Author of another thread pool says mtrebi's thread pool fails tests

[Yzrsah]

From:
https://github.com/Fdhvdu/ThreadPool/tree/master/comparison

First, I will not compare nbsdx, philipphenkel, tghosgor and mtrebi. Because their works has potential bug.

In the mtrebi test directory:
https://github.com/Fdhvdu/ThreadPool/tree/master/comparison/mtrebi

Warning
Do not use this.
It gets stuck when runing out_100 sometimes.

[mtrebi]

Hey,

Thanks for notifying me. I had no idea someone performed tests on my thread pool and it failed. I actually never thought someone would use this. I did this project to learn how threads worked and never expected someone to use it in a professional environment. I did everything for learning purposes.

Nowadays, I am really busy with my current job but I keep this in mind and I'll try to fix it in the future.

Thanks,

Mariano.

[jwang01]

Looks like there is deadlock which cause cpu to go 0 and app freezes.

[yvoinov]

Deadlock can occurs due to race/spurious wakeup.

[jwang01]

It seems that it is missing the lockguard/unique_lock in two places:
m_conditional_lock.notify_all()
and
m_conditional_lock.notify_one()

[yvoinov]

It doesn't work that way. To protect against spurious wakeup, you should do something like this: https://github.com/yvoinov/thread-pool-cpp/blob/thread-pool-cpp-round-robin-stealing/include/thread_pool/worker.hpp. Mutexes do not play here. (If we're really have spurious wakeup case here.)

[jwang01]

The code seems a bit complicated. What's spurious wakeup case, do you mind elaborating a little bit on it? https://stackoverflow.com/questions/17101922/do-i-have-to-acquire-lock-before-calling-condition-variable-notify-one
also:
https://thispointer.com/c11-multithreading-part-7-condition-variables-explained/

[yvoinov]

https://stackoverflow.com/questions/17101922/do-i-have-to-acquire-lock-before-calling-condition-variable-notify-one

This about different thing. Of course, you always can make your own tests to prove your own theory :)

[jwang01]

You can also refer to this example: https://github.com/vit-vit/ctpl, which is working, but a bit slower.

[yvoinov]

I can cite a bunch of links proving that there is no need to block the notification of conditional variables. But I will not. :) You can continue to stay with your own delusions. :) But I advise you to better examine the issue of spurious wakeup.

[yvoinov]

In addition: Mutexes is not slow. Slow only lock contention. So, think more, does you really want to slow down thread pool.

[jwang01]

It looks to me the bug is in line 32: m_pool->m_conditional_lock.wait(lock); Here it needs to check certain condition is met or not to tell whether it is spurious wake up or not.

m_pool->m_conditional_lock.wait(lock, [this]{return ! m_pool->m_queue.empty();});

[C8LUKA]

It looks to me the bug is in line 32: m_pool->m_conditional_lock.wait(lock); Here it needs to check certain condition is met or not to tell whether it is spurious wake up or not.

m_pool->m_conditional_lock.wait(lock, [this]{return ! m_pool->m_queue.empty();});

I think you right, you can set up cpu core to thread to avoid that, or trying to wait_for certain time