4 ms·
The code in listings 3 and 5 is not thread safe. The list could easily become empty between the call to empty() and acquiring the mutex, leading to a race condi
by jey 15y ago
The code in listings 3 and 5 is not thread safe. The list could easily become empty between the call to empty() and acquiring the mutex, leading to a race condition. I see two other subtle race conditions as well. I haven't looked at the rest but this density of bugs is enough to convince me that this author doesn't understand multithreaded programming well enough to give advice on the topic.
Other bugs:
1. Assumes that std::list<T>::empty() is an atomic operation.
2. Assumes it's safe to use separate locks for reading and writing, which std::list says nothing about. It probably screws up even in practice for at least the case where you have only one element in the list and two threads execute push() and pop() at the same time.
- mentat 15y agoI agree. It looks like their read implementation means that every thread pops the same item off as well, since they clone to a temp variable.
- jey 15y agoThreads have separate stacks and the call to front() and pop_front() is protected by the mutex, so that part is fine as far as I can see.
- mentat 15y agoI'm not familiar with the STL implementation, but it seems the pop would need to be coherent with the push in an empty stack. Like if a thread was trying to pop the only entry while another was trying to push, 'intuitively' there'd be a race condition there if STL isn't thread safe (which is the whole point of the article I think).
- mkup 15y agoAgreed. It's better idea to use mutex + 2 semaphores to implement queue with blocking: http://en.wikipedia.org/wiki/Producer-consumer_problem#Using_semaphores http://en.wikipedia.org/wiki/Producer-consumer_problem#Using... (it's a classical computer science problem with classical solution)
- dgottlieb 15y agoIn fairness to the rest of the article, in Listing 9 the author does protect the call to empty() with (the same) lock and also moves the condition inside of a while loop.
- Jabbles 15y agoI agree. Listing 11 won't even compile as was_empty is declared const.