Rendezvous after receiving "exit" notification #159
Conversation
MaskRay
force-pushed
the
quit
branch
3 times, most recently
from
396bc0a to
62aba26
Compare
| @@ -91,6 +92,10 @@ struct Index_Request { | |||
| int64_t ts = tick++; | |||
| }; | |||
|
|
|||
| std::mutex thread_mtx; | |||
| std::condition_variable no_pending_threads; | |||
| int pending_threads; | |||
There was a problem hiding this comment.
pending_threads is protected by thread_mtx. It doesn't need to be atomic.
| manager.Quit(); | ||
|
|
||
| { std::lock_guard lock(index_request->mutex_); } | ||
| indexer_waiter->cv.notify_all(); |
There was a problem hiding this comment.
It’s better to protect the notify with the mutex. If the thread is woken up for any reason, the signal would be lost
There was a problem hiding this comment.
The only execution sequence which can lead to lost signal is:
waiter lock
waiter test
signaler change pipeline::quit to true
signaler notify
waiter unlock
By having a lock_guard between pipeline::quit=true; and indexer_waiter->cv.notify_all();, we guarantee that this scenario will not happen.
There was a problem hiding this comment.
The code is correct. It is a bit obscure, however.
src/sema_manager.cc
Outdated
| @@ -432,6 +436,8 @@ void *CompletionMain(void *manager_) { | |||
| set_thread_name("comp"); | |||
| while (true) { | |||
| std::unique_ptr<SemaManager::CompTask> task = manager->comp_tasks.Dequeue(); | |||
| if (pipeline::quit) | |||
There was a problem hiding this comment.
A full memory barrier is probably not necessary here?
There was a problem hiding this comment.
ThreadedQueue::{Dequeue,PushBack} use a mutex internally, the access of pipeline::quit is guaranteed to be synchronized.
There was a problem hiding this comment.
Yes a seq_cst barrier is an overkill. Consider the following:
if(pipeline::quit.load(std::memory_order_relaxed)
...(or memory_order_acquire if it wasn't protected by a mutex).
There was a problem hiding this comment.
It seems everything is protected by a mutex. I can change every load of quit to use std::memory_order_relaxed
@lhmouse