1

This shouldn't be too hard to understand:

bool ThreadIsRunning = false;

void Thread1(void *nothing){
    ThreadIsRunning = true;
    cout << "The thread is running!\n";
    Sleep(1000);
    cout << "Ending thread!\n");
    ThreadIsRunning = false;
    _endthread();
    return;
}


int main(){

    std::cout << "The thread is starting!\n";

    _beginthread(Thread1, 0, 0);

    std::cout << "Waiting for thread to end!\n";

    while(ThreadIsRunning);

    std::cout << "The thread is ended!\n";

    return 0;
}

So the main thread wait's for the Thread1 to set ThreadIsRunning to false, right?

Yeah, but it does not. Nothing happens when it goes to false. Shouldn't it check the value for eternity until it's changed?

It works if I put while(ThreadIsRunning) Sleep(10); but I don't think it should be necessary for my code to work.

while(ThreadIsRunning) void(); does not work either.

Im using Visual Studio Ultimate 2012.

C/C++ command line options:

/GS /GL /analyze- /W3 /Gy /Zc:wchar_t /Zi /Gm- /O2 /sdl /Fd"Release\vc110.pdb" /fp:precise /D "WIN32" /D "NDEBUG" /D "_CONSOLE" /D "_UNICODE" /D "UNICODE" /errorReport:prompt /WX- /Zc:forScope /Gd /Oy- /Oi /MD /Fa"Release\" /EHsc /nologo /Fo"Release\" /Fp"Release\Test1.pch"

Linker command line options:

/OUT:"<projectsfolder>\Test1\Release\Test1.exe" /MANIFEST /LTCG /NXCOMPAT /PDB:"<projectsfolder>\Test1\Release\Test1.pdb" /DYNAMICBASE "kernel32.lib" "user32.lib" "gdi32.lib" "winspool.lib" "comdlg32.lib" "advapi32.lib" "shell32.lib" "ole32.lib" "oleaut32.lib" "uuid.lib" "odbc32.lib" "odbccp32.lib" /DEBUG /MACHINE:X86 /OPT:REF /SAFESEH /INCREMENTAL:NO /PGD:"<projectsfolder>\Test1\Release\Test1.pgd" /SUBSYSTEM:CONSOLE /MANIFESTUAC:"level='asInvoker' uiAccess='false'" /ManifestFile:"Release\Test1.exe.intermediate.manifest" /OPT:ICF /ERRORREPORT:PROMPT /NOLOGO /TLBID:1

Edit: There is NOT a schedule/race/time problem. Even if I add Sleep(1000); after _beginthread(Thread1, 0, 0), the problem is still that nothing happens when ThreadIsRunning goes to false

Christopher Janzon
  • 1,039
  • 11
  • 22
  • Do you see "The thread is ended!" in your output? Does your program end? – Hamlet Hakobyan Dec 26 '13 at 22:09
  • 1
    Are you really looking for something like this? http://stackoverflow.com/q/11779504/10077 – Fred Larson Dec 26 '13 at 22:12
  • You have a race condition here. It's quite possible that the main thread will check `ThreadIsRunning` before the other thread ever starts. You also have a potential problem with the compiler optimizing away the variable access altogether because it doesn't take multithreading into account. Those, among others, are the reasons you should use synchronization primitives (such as wait handles) when communicating between threads. The link that Fred Larson posted in his comment is the way you want to go. – Jim Mischel Dec 26 '13 at 22:18
  • @HamletHakobyan No. The program does not end. – Christopher Janzon Dec 26 '13 at 22:20
  • @JimMischel No there's no race condition. That is not the problem. I could just add another ´Sleep´ function and it would still not end. And yes, I think so too that the compiler is optimizing in a way it's not checking the variable in the loop. But if i don't want to use any wait handlers? Why can I not use my method? What is wrong with a variable checking loop? – Christopher Janzon Dec 26 '13 at 22:23
  • The race condition exists. That you apparently don't suffer any ill effects from it is just the luck of the draw. Run on some other hardware or built with some other compiler, it could be the cause of the problem. You can't use your method because the compiler defaults to assuming single-threaded code. And because the compiler determines that `ThreadIsRunning` can never be modified within the loop, it just silently optimizes out the check. You'll probably have to use [volatile](http://msdn.microsoft.com/en-us/library/x13ttww7.aspx) if you really want to do things this way. – Jim Mischel Dec 26 '13 at 22:48

5 Answers5

4

This can be down to the thread scheduling - it is perfectly possible that before the thread even starts that the thread running main() reaches your while() statement and breaks out of it before the thread has had chance to change your global boolean to true. That is probably why you see that adding the Sleep call give the thread chance to change from false to true and you see the behaviour you expect.

mathematician1975
  • 21,161
  • 6
  • 59
  • 101
2

C++03 is not a thread-aware language and the optimizer can "clearly see" (having no knowledge that another thread could be changing it) that ThreadIsRunning is not changing during the loop body. As soon as you add the call to sleep the compiler has to assume that sleep accesses an aliased copy of ThreadIsRunning and has to check its value each iteration through the loop.

The solution to your problem is to not use a standard variable and a wait-loop (which will pet a CPU core). Instead use a condition variable and signal between thread when appropriate, as that's the standard way to convey such information between threads.

Mark B
  • 95,107
  • 10
  • 109
  • 188
0

You may have to declare ThreadIsRunning as volatile. The compiler probably optimized it out since it did not see anything modifying it in main().

TimDave
  • 652
  • 3
  • 6
  • While it is true that `volatile` is often assumed to mean things it does not, in my test it actually does make this particular example work. That said, `WaitForSingleObject(...)` is a better solution than a spin loop like this. – Retired Ninja Dec 26 '13 at 22:14
  • @RetiredNinja I had the same result. ´volatile´ made it work. But why? What should I use instead of ´volatile´? And why would i use ´WaitForSingleObject´ instead? – Christopher Janzon Dec 26 '13 at 22:18
  • The use of volatile in this example is not intended to be a synchronizing mechanism beyond preventing the compiler from optimizing the code to prevent checking the actual value of the variable. This is used and often necessary when one thread updates a variable that another thread is checking. Using a mutex, semaphore, barrier, etc. is more proper. WaitForSingleObject will wait for the thread to complete, thus reducing the possibility of a race condition. – TimDave Dec 26 '13 at 22:24
  • It's platform dependent, but `volatile` often affects how the optimizer handles code and whether or not it can assume that a value won't change due to external events. You would use `WaitForSingleObject` because it does what your spin loop does in a much more efficient and completely predictable manner. – Retired Ninja Dec 26 '13 at 22:25
  • @TimDave Okay, I think I fully understand this now! I suppose it's a optimizing problem. Why would I not use `volatile`? And why would `mutex`, `semaphore` or a `barrier` be more proper? – Christopher Janzon Dec 26 '13 at 22:38
  • @RetiredNinja Thanks! What makes `WaitForSingleObject` more efficient? Should I use another type qualifier than `volatile` for more platform-compatibility? – Christopher Janzon Dec 26 '13 at 22:40
  • Volatile still does not address the timing problems (race conditions) or synchronization issues. By using the appropriate synchronization mechanism you can guarantee that you will wait for the thread to exit. For example, if you use a condition variable the main thread will set ThreadIsRunning to true and then wait for it to be set to false by the thread. The data would be protected by a mutex so that will prevent a data race condition. A description on how to use synch. primitives in windows can be found here: http://msdn.microsoft.com/en-us/library/windows/desktop/ms686927(v=vs.85).aspx – TimDave Dec 26 '13 at 22:47
  • WaitForSingleObject, while windows only, will allow the scheduler to efficiently allow the 2 threads to run. The main thread will be suspended while the background thread is running and the main thread will then be woken up when the background thread completes. Spin looping as your example code was doing is very CPU intensive. – TimDave Dec 26 '13 at 22:52
  • volatile states that reading the variable can have side effects and should not be optimized away. What this does in every compiler and platform varies. C++11 and library synchronization primitives are a better idea, as they are well soecified in how they work. – Yakk - Adam Nevraumont Dec 26 '13 at 23:21
  • The problem of volatile is that it doesn't make any guarantees on behalf of the processor. A processor is perfectly in its right to keep the stale variable in cache and never update from memory if you use volatile. Why this will work in "practice"? Because everybody tests on x64 which has exceedingly strong ordering guarantees – Voo Dec 27 '13 at 08:54
0

Add ThreadIsRunning = true; just before starting the thread:

    std::cout << "The thread is starting!\n";

    ThreadIsRunning = true; // <============= add this

    _beginthread(Thread1, 0, 0);

    // etc.

That will tell you if it's a scheduling problem (which is likely).

egrunin
  • 24,650
  • 8
  • 50
  • 93
0

The problem is that the compiler feels free to hoist the load of the variable ThreadIsRunning out of the loop -- loading it once before the loop into a register and then checking the register (instead of the variable) each time around the loop. The register never becomes false, so the loop never exits.

The easiest and most obvious fix is to mark the variable volatile. That way, the compiler knows it can't hosit the load, and needs to reload the variable each time around the loop.

Another "fix" that makes things work is to call any externally defined function in the loop (such as Sleep). Since the compiler doesn't know what Sleep does, it must assume it might change the global var, so it needs to be reloaded. Calling a local function (one defined earlier in the same file or a header) probably won't work, because then the compiler knows what the function does (and in particular that it doesn't modify ThreadIsRunning)

Chris Dodd
  • 119,907
  • 13
  • 134
  • 226
  • Volatile is not a valid fix in vanilla c++ because it doesn't give any visibility guarantees from the processor. The correct fix is to use std::atomic or some compiler specific way to say memory barriers. For visual studio the later can indeed be done with volatile by making sure the right command-line option is set. – Voo Dec 27 '13 at 08:51
  • @Voo: `_beginthread` and `_endthread` don't exist in vanilla C++ either. `volatile` is the right solution for old C++ code that uses these instead of `std::thread`, as `std::atomic` doesn't exist on such systems, and is needed (in addition to `std::atomic`) on newer systems, as `std::atomic` just guarentees atomicness, and doesn't prevent hoisting. – Chris Dodd Dec 28 '13 at 21:28
  • I wouldn't have downvoted if you had actually mentioned all the downsides to the method and listed the limitations: Namely that it only works with VC and if you use a specific commandline flag that's not even the default in all situations. And your second solution is just plain wrong on any compiler I know of even accounting for compile flags. Anyhow, `std::atomic` obviously does give you memory ordering guarantees, matter of fact you have to specify them when loading/storing. – Voo Jan 02 '14 at 15:43
  • @Voo: `std::atomic`'s memory ordering guarantees do not prevent coalescing loads, so do not prevent the hoisting that is the problem in this example. – Chris Dodd Jan 02 '14 at 17:53
  • That's certainly not true for sequentially consistent memory orderings (or acquire/release), according to [gcc's summary of the models](http://gcc.gnu.org/wiki/Atomic/GCCMM/AtomicSync) it's also not true for the relaxed model: "There is also the presumption that relaxed stores from one thread are seen by relaxed loads in another thread within a reasonable amount of time. That means that on non-cache-coherent architectures, relaxed operations need to flush the cache (although these flushes can be merged across several relaxed operations)". – Voo Jan 02 '14 at 18:09