Thread: Cancel FileCopy Operation

1. Non-indented code is a crime against nature.

2. You need to do the big work in a background thread, because in the main thread of the program, only one message at a time can be processed. If you put the work directly inside an event handler, the next message can't be processed until all your work is done.

Originally Posted by dsarrafi

I agree with the first point. This is my very first post ever! So give me a break.

Get in the habit of making your code readable. The reason we even have source code, as opposed to just writing binary programs with a hex editor, is for human readability and understanding.

As for doing the deleting/copying in a thread, I have never done that. Can you suggest the function and how to user-abort it. Thx!

For how to abort it, you could use a static boolean and change its value from the main thread. Note - normally there is good reason not to access the same data from your main and worker threads. It's very easy to get in a race condition. In this very specific example, it doesn't need thread synchronization.


As to how I'd do threading, here's one way. First, you create a type that contains all the data that is shared between your main class and the thread. This isn't strictly necessary, you can pass a pointer to your main class object, but this makes it a lot more obvious which parts of your code might need to consider thread safety:

Code:

// Create a data object to give to the thread
struct ThreadData
{
      bool run;
      // Your other data for the thread
};

You'd make a function outside of the class that would do your work:

Code:

UINT ThreadMain (void * data)
{
      ThreadData * threadData = static_cast<ThreadData*>(data);
      while (threadData->run)
      {
      // Do stuff
      }
      return 0;
}

You'd add a private member of the ThreadData type, and set it up when the button was clicked to begin the thread:

Code:

      // Inside your begin button's event handler
      // Set all the variables in your data object
      m_threadData.run = true;
      AfxBeginThread(ThreadMain, &m_threadData);

And to ask the thread to stop:

Code:

      // Inside your cancel button's event handler
      m_threadData.run = false;

This is the very basics. It won't do things like report progress, report when it's finished, and initially you'd have problems if, say, you clicked begin while a thread was still running. All of that is solvable, but this should be a place to start from. You can read more about CWinThread on MSDN if you'd like to learn more about how MFC does threading.

If you moved to .NET, it actually has a more robust background worker threading system that handles a lot of that messaging for you, so the pieces about knowing if your worker is still running become a lot easier.

Originally Posted by dsarrafi

I am new to C++/MFC.

You may be using C++, but your code is largely MFC (i.e. windows specific), so is off-topic in a C++ forum. You've asked your question in the wrong forum (a windows programming forum would be more likely to get useful answers).

As to your problem, I wouldn't necessarily use a static boolean, but the technique is for your main thread to set some indicator (an int, a bool, or something testable) when the cancel button is hit, and for the worker thread to periodically check that indicator, and finish up as needed. If you have a loop checking FindNextFile(), an obvious place to check is in that loop.

Since that indicator/variable is actually shared between threads, it is a good idea to protect it (using a critical section or mutex), to prevent race conditions. Yes, even with simple types like bool that is needed - operations on basic types are not usually atomic (i.e. a thread doing something to such a variable can be preempted part way through most operations).

Originally Posted by grumpy

Since that indicator/variable is actually shared between threads, it is a good idea to protect it (using a critical section or mutex), to prevent race conditions. Yes, even with simple types like bool that is needed - operations on basic types are not usually atomic (i.e. a thread doing something to such a variable can be preempted part way through most operations).

But in this particular case, even if it was pre-empted, there isn't a race condition. It's not just that it's a primitive type, but that it's a boolean that is only set by one thread and only read by another, and there's no relationship between its state and the state of anything else in the struct.

Even with pre-empting, the worker thread is going to read the value as either 'true' or 'false', so it will either act like it was atomically set before the pre-emption or like it was atomically set after.

I suppose you could argue that maybe a boolean could transition from true to false by passing through an intermediate invalid state, so the thread could read a bit pattern that is not a possible value for a bool.

If that was a concern you could always use an integer (which is guaranteed not to have any invalid states) and check zero/nonzero. You may well read a partially updated integer, but again, you'll either interpret it as true or false. Essentially, with or without synchronization, you will always read a long set of 'true' values that ultimately becomes 'false'. The transition from true to false will occur sometime between when the main thread begins updating the variable and when it ends updating the variable, and it won't pass through any invalid states along the way.

Originally Posted by dsarrafi

Again, my code was formatted.

There's no "again", you never said that before. All you said was that it was your first post ever. If you're expecting me to infer from that statement that your source code was actually well formatted and the formatting was lost somewhere along the way, well, that's certainly not obvious.

So why is it that you feel compelled to patronize me?

I wasn't being patronizing. All you'd given me to go on is that you were new to C++/MFC and new to the forum. I made the obvious, if incorrect, inference that you're a newbie who hasn't yet learned good formatting skills - which is far, far more common to see here than the occasional glitches in the forum's code parsing.

Originally Posted by Cat

But in this particular case, even if it was pre-empted, there isn't a race condition. It's not just that it's a primitive type, but that it's a boolean that is only set by one thread and only read by another, and there's no relationship between its state and the state of anything else in the struct.

Even with pre-empting, the worker thread is going to read the value as either 'true' or 'false', so it will either act like it was atomically set before the pre-emption or like it was atomically set after.

What you describe, at best, is "sort of" thread-safe. It's dangerous in practice, because compilers and modern CPUs have a fetish for getting in the way of making it work like you describe.

What you are missing is that thread safety depends on context. Updating a bool is always thread safe, if you never read from it. But, if you do read from it, then the answer depends on when you read from it, and what that read signifies.

On some CPUs, but not all, writes to an object of type bool will be atomic. x86 CPUs will generally make it atomic, but others do NOT. If the update or the read are not atomic, then they need to be synchronised.

The next problem, even if the update and read are atomic, is reordering. The compiler (and CPU) are not guaranteed to carry out reads/writes to variables in the order specified by your program. People often claim that can be addressed by making the variable volatile .... which brings us to the next problem.

Declaring a variable volatile only guarantees that operations on it (and other volatile variables) are not reordered. But it provides no guarantee about reordering one volatile memory access relative to any non-volatile ones, particularly those local to either the worker thread or the main thread. A common example involves defining some kind of flag to protect access to a resource, make the flag volatile, and then the compiler moves the resource access up so it happens before you check the flag. It's allowed to do that, because it's not reordering the internal ordering of two volatile accesses, but merely a volatile and a non-volatile one.

The only way to prevent that is to use explicit synchronisation: a critical section, a mutex, etc ..... i.e. a facility supported by the host system (or executable process) that exists in a context which is more than the functions which make use of that variable. That facility also explicitly prevents (unless there is a serious compiler bug) reordering between volatile and non-volatile accesses.

Note I referred to memory accesses in the above: it is no accident that folks talk about "memory barriers" - places (or markers) in a sequence of instructions that ensure instructions which act on memory within a barrier cannot be reordered with instructions that act on memory outside the barrier. [Being grossly over-simplistic here].

Originally Posted by Cat

I suppose you could argue that maybe a boolean could transition from true to false by passing through an intermediate invalid state, so the thread could read a bit pattern that is not a possible value for a bool.

There are multiprocessor systems where one thread reading a register that is being updated by an instruction being executed by another processor will generate a signal that must be trapped (otherwise the offending processor is prevented - in hardware - from doing certain normal operations).

With multicore processors, an analogous behaviour can occur when multiple cores access a cache or set of variables that are shared between them. It is all part of how multicore and multiprocessor systems negotiate access to shared resources (cache, registers, etc)

Originally Posted by Cat

If that was a concern you could always use an integer (which is guaranteed not to have any invalid states) and check zero/nonzero.

You are aware that the C++ standard specifies that accessing the value of an uninitialised integer (or any variable or memory) has undefined behaviour?

One reason for that (rather than simply requiring that uninitialised variables always be zero) is processors where an uninitialised variable can have an "invalid" state - so reading such a variable before it has ever been written to will cause a processor signal or the like.

Originally Posted by Cat

You may well read a partially updated integer, but again, you'll either interpret it as true or false. Essentially, with or without synchronization, you will always read a long set of 'true' values that ultimately becomes 'false'. The transition from true to false will occur sometime between when the main thread begins updating the variable and when it ends updating the variable, and it won't pass through any invalid states along the way.

Changing the type of variable does not change the concerns.

The basic problem is that, even if memory accesses aren't atomic, you're assuming that the compiler will fix things up so they seem to be atomic as far as code in your program is concerned. That is simply not the case. There are too many lower level details that prevent compilers being able to do that (no matter what strategy they employ, there is always some variability in how processors work that introduces a non-zero chance of that strategy failing).

Hence the primary rule of multithreading: the programmer, and not the compiler, is responsible for protecting those variables which are used by multiple threads. Tools for that include mutexes, critical sections, memory barriers, and the like.

>> But in this particular case, even if it was pre-empted, there isn't a race condition.
In addition to grumpy's points - it's undefined behavior in both C++11 and Posix. C++11 calls it a "data race".

So that's a good reason not to do it.

gg

Originally Posted by dsarrafi

Why do we geeks like to get into ........ing contests? If it's respect we're seeking, then it is best earned by giving people who asking for help simple terse concrete help instead of theoretical pontification!

Cat's suggestion was simple, terse and concerete. I haven't implemented it yet but I'm pretty sure it's going to work. I will figure out the synchronization if necessary for my project.

Cat's advice also is not guaranteed to work despite his and your unsubstantiated belief otherwise. That is the reason I made the comments I did in my first post.

True, that "simple, terse, and concrete" suggestion will seem to work sometimes - and might even do so consistently in some settings - but that is just blind luck. It is not guaranteed to work, and that is bad technique. Theoretically and practically.

Multithreaded programming is littered with the bodies of over-confident programmers. You and Cat can look forward to joining them.

Originally Posted by grumpy

Cat's advice also is not guaranteed to work despite his and your unsubstantiated belief otherwise.

Guaranteed to work on every hypothetical system that is standards compliant? No.

Guaranteed to work on every actual system that has been built or is likely to ever be built? That's the more important question.

In most cases I don't see a benefit in taking a performance hit on every iteration of a loop to guard against a problem that isn't realistically going to be possible. Maybe if this code were used in nuclear launch software or something that level of paranoia is justified, but I generally don't care if the code will operate on every hypothetical system that could be created, I care if it's going to run on the actual systems it will be deployed to.