Issue 10026001: Cross-platform thread primitives (Closed)
|
Created:
March 29, 2013, 5:26 a.m. by Felix Dahlke Modified:
Nov. 12, 2013, 10:09 a.m. Visibility:
Public. |
DescriptionI've added some thread primitives (threads, mutices and condition variables) we'll need for moving JS execution to a separate thread.
I'm not a fan of adding something we don't use yet, but I'm quite certain we'll need this. The alternative would be using an event loop library like libevent, which I looked into yesterday, but I don't think it's the way to go for us, at least not yet.
I wasn't able to run the tests on Windows, as that's not yet possible there. I already pushed the changes as they're not used yet, and to compile them on Windows.
As for the design, I've considered making something more similar to C++11's std::thread, but that isn't really usable without a general purpose functor, so I went with an inheritance-based approach.
Patch Set 1 #
Total comments: 28
Patch Set 2 : Renamed thread to nativeThread, made the tests more robust #Patch Set 3 : Don't unlock/signal mutices/conditions, move Mutex and Condition to the top level #
Total comments: 8
Patch Set 4 : Improve mutex test #MessagesTotal messages: 8
http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp File src/Thread.cpp (right): http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp#newcode24 src/Thread.cpp:24: Unlock(); That's a race condition - you unlock the mutex so some thread waiting on it can grab it before you delete it. Just delete it, do not unlock before you do. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp#newcode48 src/Thread.cpp:48: } I would be in favor of an AutoLock helper class that will unlock the mutex automatically when the scope is exited: { AutoLock al(mutex); // Do something here } http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp#newcode62 src/Thread.cpp:62: Signal(); This is a race condition again. Why would we want to signal before destroying anyway? This will only introduce an inconsistency compared to Windows. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp#newcode92 src/Thread.cpp:92: CreateThread(0, 0, &CallRun, 0, this, &thread); Wrong order of parameters? |this| should passed as fourth parameter. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.h File src/Thread.h (right): http://codereview.adblockplus.org/10026001/diff/1/src/Thread.h#newcode15 src/Thread.h:15: class Mutex Why not make that class public? I think it will become necessary on its own as well. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.h#newcode39 src/Thread.h:39: #ifdef WIN32 CONDITION_VARIABLE nativeCondition; missing here? http://codereview.adblockplus.org/10026001/diff/1/src/Thread.h#newcode52 src/Thread.h:52: HANDLE thread; Rename this into nativeThread for consistency? http://codereview.adblockplus.org/10026001/diff/1/test/Thread.cpp File test/Thread.cpp (right): http://codereview.adblockplus.org/10026001/diff/1/test/Thread.cpp#newcode26 test/Thread.cpp:26: timesCalled++; This statement isn't thread-safe, you need a mutex to ensure that this variable isn't incremented twice in parallel. http://codereview.adblockplus.org/10026001/diff/1/test/Thread.cpp#newcode109 test/Thread.cpp:109: ASSERT_EQ(0, mock.timesCalled); This is a race condition - by the time you check the call count might have been incremented already. http://codereview.adblockplus.org/10026001/diff/1/test/Thread.cpp#newcode123 test/Thread.cpp:123: ASSERT_FALSE(mock2.working); Please note that Sleep() (both the Windows and the POSIX variants) merely guarantees that a thread will sleep *at least* a given number of milliseconds. It does not guarantee that it will be exactly that number of milliseconds. In other words, the assert here can fail if the main thread gets delayed (e.g. because some other heavy action is going on). It's better to have the threads push their states to some variable and later verify that the order of these states is correct. http://codereview.adblockplus.org/10026001/diff/1/test/Thread.cpp#newcode141 test/Thread.cpp:141: ASSERT_EQ(1, queue.size()); Same flaw here as above.
http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp File src/Thread.cpp (right): http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp#newcode24 src/Thread.cpp:24: Unlock(); On 2013/04/03 13:14:47, Wladimir Palant wrote: > That's a race condition - you unlock the mutex so some thread waiting on it can > grab it before you delete it. Just delete it, do not unlock before you do. Both DeleteCriticalSection and pthread_mutex_destroy lead to undefined behaviour when called on a locked mutex, so... But you're right, this may cause trouble. Then again, it'd be quite the bug if one thread would try to lock the mutex while it's being destructed. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp#newcode48 src/Thread.cpp:48: } On 2013/04/03 13:14:47, Wladimir Palant wrote: > I would be in favor of an AutoLock helper class that will unlock the mutex > automatically when the scope is exited: > > { > AutoLock al(mutex); > // Do something here > } Agreed, that's neat. I'll add one later with a new review. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp#newcode62 src/Thread.cpp:62: Signal(); On 2013/04/03 13:14:47, Wladimir Palant wrote: > This is a race condition again. Why would we want to signal before destroying > anyway? This will only introduce an inconsistency compared to Windows. As above, to avoid undefined behaviour. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp#newcode92 src/Thread.cpp:92: CreateThread(0, 0, &CallRun, 0, this, &thread); On 2013/04/03 13:14:47, Wladimir Palant wrote: > Wrong order of parameters? |this| should passed as fourth parameter. Yep, I somehow mixed that up and didn't manage to pull properly before compiling my changes on Windows, so it seemed fine... Oleksandr fixed this. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.h File src/Thread.h (right): http://codereview.adblockplus.org/10026001/diff/1/src/Thread.h#newcode15 src/Thread.h:15: class Mutex On 2013/04/03 13:14:47, Wladimir Palant wrote: > Why not make that class public? I think it will become necessary on its own as > well. It is public, just nested in Thread. Would you prefer to have it on the top level? Don't have a strong opinion myself. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.h#newcode39 src/Thread.h:39: #ifdef WIN32 On 2013/04/03 13:14:47, Wladimir Palant wrote: > CONDITION_VARIABLE nativeCondition; missing here? Yes, somehow messed up the patch it seems, Oleksandr fixed that. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.h#newcode52 src/Thread.h:52: HANDLE thread; On 2013/04/03 13:14:47, Wladimir Palant wrote: > Rename this into nativeThread for consistency? Absolutely, done. http://codereview.adblockplus.org/10026001/diff/1/test/Thread.cpp File test/Thread.cpp (right): http://codereview.adblockplus.org/10026001/diff/1/test/Thread.cpp#newcode26 test/Thread.cpp:26: timesCalled++; On 2013/04/03 13:14:47, Wladimir Palant wrote: > This statement isn't thread-safe, you need a mutex to ensure that this variable > isn't incremented twice in parallel. I thought that's pretty unlikely here, but fair enough, fixed it. http://codereview.adblockplus.org/10026001/diff/1/test/Thread.cpp#newcode109 test/Thread.cpp:109: ASSERT_EQ(0, mock.timesCalled); On 2013/04/03 13:14:47, Wladimir Palant wrote: > This is a race condition - by the time you check the call count might have been > incremented already. Yes indeed, made it a bit more robust. It's still a kind of silly test though, any idea for a better one? http://codereview.adblockplus.org/10026001/diff/1/test/Thread.cpp#newcode123 test/Thread.cpp:123: ASSERT_FALSE(mock2.working); On 2013/04/03 13:14:47, Wladimir Palant wrote: > Please note that Sleep() (both the Windows and the POSIX variants) merely > guarantees that a thread will sleep *at least* a given number of milliseconds. > It does not guarantee that it will be exactly that number of milliseconds. In > other words, the assert here can fail if the main thread gets delayed (e.g. > because some other heavy action is going on). It's better to have the threads > push their states to some variable and later verify that the order of these > states is correct. Agreed, much more robust that way. http://codereview.adblockplus.org/10026001/diff/1/test/Thread.cpp#newcode141 test/Thread.cpp:141: ASSERT_EQ(1, queue.size()); On 2013/04/03 13:14:47, Wladimir Palant wrote: > Same flaw here as above. Fixed as well.
http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp File src/Thread.cpp (right): http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp#newcode24 src/Thread.cpp:24: Unlock(); To me it sounds like AutoLock is a better solution to ensure that a mutex is definitely unlocked when it is being destroyed... http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp#newcode62 src/Thread.cpp:62: Signal(); I think that not destroying conditions that are still in use is a better way to avoid undefined behavior - otherwise we are looking for trouble no matter what we do. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.h File src/Thread.h (right): http://codereview.adblockplus.org/10026001/diff/1/src/Thread.h#newcode15 src/Thread.h:15: class Mutex AdblockPlus::Thread::Mutex just sounds wrong - it isn't directly related to threads. AdblockPlus::Threading::Mutex on the other hand would be fine but then you would need AdblockPlus::Threading::Thread as well - way too much to write. Better top level I guess, there is little potential for conflict anyway. Condition will have to be renamed into ConditionVariable however.
http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp File src/Thread.cpp (right): http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp#newcode24 src/Thread.cpp:24: Unlock(); On 2013/04/03 19:30:16, Wladimir Palant wrote: > To me it sounds like AutoLock is a better solution to ensure that a mutex is > definitely unlocked when it is being destroyed... Yeah, you're probably right. I was planning to wrap pthreads/Win32 threads thinly after all, so it's better not to have any magic here. Removed the Unlock() call. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.cpp#newcode62 src/Thread.cpp:62: Signal(); On 2013/04/03 19:30:16, Wladimir Palant wrote: > I think that not destroying conditions that are still in use is a better way to > avoid undefined behavior - otherwise we are looking for trouble no matter what > we do. Yeah, same as above, removed the Signal() call. http://codereview.adblockplus.org/10026001/diff/1/src/Thread.h File src/Thread.h (right): http://codereview.adblockplus.org/10026001/diff/1/src/Thread.h#newcode15 src/Thread.h:15: class Mutex On 2013/04/03 19:30:16, Wladimir Palant wrote: > AdblockPlus::Thread::Mutex just sounds wrong - it isn't directly related to > threads. AdblockPlus::Threading::Mutex on the other hand would be fine but then > you would need AdblockPlus::Threading::Thread as well - way too much to write. > Better top level I guess, there is little potential for conflict anyway. > Condition will have to be renamed into ConditionVariable however. Sure, moved them to the top level.
http://codereview.adblockplus.org/10026001/diff/12001/test/Thread.cpp File test/Thread.cpp (right): http://codereview.adblockplus.org/10026001/diff/12001/test/Thread.cpp#newcode28 test/Thread.cpp:28: Sleep(5); Why is this necessary? Seems random. http://codereview.adblockplus.org/10026001/diff/12001/test/Thread.cpp#newcode112 test/Thread.cpp:112: mock.mutex.Lock(); This should be done before mock.Start(), otherwise the thread might lock the mutex before you do. http://codereview.adblockplus.org/10026001/diff/12001/test/Thread.cpp#newcode127 test/Thread.cpp:127: mock2.Start(); This test doesn't make sense - mock1 will already be finished by the time mock2 starts, locking or not. Should LockingMock have Sleep(10) before pushing its name to the log? Actually, there are ordering assumptions here regardless - these will be correct most of the time but there is no guarantee. How about starting both mock1 and mock2 immediately? They could log "name started" when they start, sleep a bit and log "name done" before releasing the lock. There will be two valid results: "mock1 started", "mock1 done", "mock2 started", "mock2 done" and "mock2 started", "mock2 done", "mock1 started", "mock1 done".
http://codereview.adblockplus.org/10026001/diff/12001/test/Thread.cpp File test/Thread.cpp (right): http://codereview.adblockplus.org/10026001/diff/12001/test/Thread.cpp#newcode28 test/Thread.cpp:28: Sleep(5); On 2013/04/04 07:09:00, Wladimir Palant wrote: > Why is this necessary? Seems random. See below. http://codereview.adblockplus.org/10026001/diff/12001/test/Thread.cpp#newcode112 test/Thread.cpp:112: mock.mutex.Lock(); On 2013/04/04 07:09:00, Wladimir Palant wrote: > This should be done before mock.Start(), otherwise the thread might lock the > mutex before you do. That's what the 5ms sleep above is for. It's not brilliant, but that way it will at least test that there's actually a separate thread of execution. http://codereview.adblockplus.org/10026001/diff/12001/test/Thread.cpp#newcode127 test/Thread.cpp:127: mock2.Start(); On 2013/04/04 07:09:00, Wladimir Palant wrote: > This test doesn't make sense - mock1 will already be finished by the time mock2 > starts, locking or not. Should LockingMock have Sleep(10) before pushing its > name to the log? > > Actually, there are ordering assumptions here regardless - these will be correct > most of the time but there is no guarantee. How about starting both mock1 and > mock2 immediately? They could log "name started" when they start, sleep a bit > and log "name done" before releasing the lock. There will be two valid results: > "mock1 started", "mock1 done", "mock2 started", "mock2 done" and "mock2 > started", "mock2 done", "mock1 started", "mock1 done". Yeah, that's a much better test. Did that.
LGTM with that compile issue fixed. http://codereview.adblockplus.org/10026001/diff/12001/test/Thread.cpp File test/Thread.cpp (right): http://codereview.adblockplus.org/10026001/diff/12001/test/Thread.cpp#newcode112 test/Thread.cpp:112: mock.mutex.Lock(); Oh well :-( http://codereview.adblockplus.org/10026001/diff/12001/test/Thread.cpp#newcode146 test/Thread.cpp:146: ASSERT_EQ(0, queue.size()); This fails to compile for me once again, 0u please. 
|
