std::condition_variable::notify_one/all() should be called after unlocking mutex

[jimmy-park]

The notifying thread doesn't need to hold the lock because the notified thread may immediately block again in some implementations. (see description and example in cppreference)
There is another caveat about notifying while holding a lock, but I think it isn't relevant to current implementation of ThreadPool.

I also recommend the implementation of task system which presented in Sean Parent's talk.

[yhirose]

@jimmy-park thank you for the reply. Could you add at least one unit test case to verify your modification works? Then, I'll do code review. Thanks!

[jimmy-park]

@yhirose I added a simple test which increases an atomic counter using TaskQueue . Since TaskQueue is only used in listen_internal(), I think it acts like a SPMC (single producer multiple consumer) queue and test code follows this behavior. I know this is an oversimplified example. Do we need more test cases for concurrency?

[yhirose]

@jimmy-park, the reason why I need the unit test is that I would like to reproduce the problem you mentioned above on my machine and confirm that your modification fixes it. Otherwise, there is no way for me to verify your fix is acceptable. :)

[jimmy-park]

@yhirose Understood 😄

[yhirose]

@jimmy-park, I just tried to run TaskQueueTest.IncreaseAtomicInteger with the current httplib.h without your change, but it worked fine with no problem. How did you find the current implementation of TaskQueue has a problem?

[jimmy-park]

@yhirose The unit test I added was a simple check of the basic functionality of TaskQueue. It works in current implementation because all waiting threads will wake up on TaskQueue::shutdown(). I think it maybe a performance issue in some platforms as cppreference says, not a functionality issue. It is hard to point out which platform has this issue, so I recommend to follow cppreference's example.

[yhirose]

@jimmy-park, thanks for your replay. I read the cppreference and now understand what you were trying to do. Your change looks good to me, and I just merged it. Thanks a lot!