search

progschj / ThreadPool

A simple C++11 Thread Pool implementation
zlib License
7.64k stars 2.21k forks source link

perfect thread pool #47

Closed ywg1013 closed 6 years ago

ywg1013 commented 6 years ago

if one thread cost many many time , other thread would be blocked. all thread can not execute on same time .

threadpool.h should be modified. insert "lock.unlock()" before line 55.

bye the way, i have a question. why do you add lock in the line 99 when destroy threadpool? because this lock. application can not exit when destroy threadpool.

progschj commented 6 years ago

I'm not sure I understand the issue. In line 55 the lock is already out of scope (and thus implicitly unlocked).

The lock in the destructor protects the stop variable since it might otherwise be concurrently read by the other threads. What actually makes the destructor wait (and potentiall prevent a program to exit) are the joins though. This way the pool makes sure all tasks that are queued actually get executed.

ywg1013 commented 6 years ago

@progschj sorry ,you are right for the first problem. thanks. but the second, the stop variable in the destructor only be read by main thread, why do you say it might be read by other threads? can you explain it?

Persixty commented 6 years ago

The stop member is read by each worker in the wait clause (line 48). Consequently it needs to be modified while holding the same lock. Otherwise there could be sequencing issues where workers going into the wait state but not yet added to the notify collection and as a result 'miss' a notification and sleep forever.

ywg1013 commented 6 years ago

@Persixty thanks for your answer.
the actual reason is that prevent the thread missing a notification and sleep forever.

i think , whether you add lock in the destructor or not , all tasks actually get executed finally.
do you think it is right?

i think that all tasks are queued actually get executed because of this condition in line49 ("&& this->tasks.empty()"), if missing this condition, the tasks that not yet executed will not executed any more.

ywg1013 commented 6 years ago

@progschj i think that all tasks are queued actually get executed because of this condition in line49 ("&& this->tasks.empty()"), if missing this condition, the tasks that not yet executed will not executed any more.