Permalink
Browse files

Use explicit pausing around fork() for signal handler and finalizer

This prevents deadlocks in situations where fork() is called from
multiple threads. The problem is that it keeps spawning and killing
threads that causes deadlocks in for example starting the thread in
Thread::in_new_thread when the thread becomes GC dependent.

The tricky interaction here between GC dependent and the worker_lock
makes this solution much easier. We now define explicit pause points
where it's fine that we can fork because these auxiliary threads aren't
running any code at that point.

These with all previous commits fixes #2138. This issues not only
exposed the thread safety issue with the C-API constants but also caused
the deadlocks that this commit and previous changes to the signal
handler fixes.
  • Loading branch information...
dbussink committed Feb 1, 2013
1 parent 305c70a commit 4b66fcb52cbba5f3fdd467122d7bf48bf8192204
Showing with 66 additions and 23 deletions.
  1. +33 −13 vm/gc/finalize.cpp
  2. +2 −0 vm/gc/finalize.hpp
  3. +29 −10 vm/signal.cpp
  4. +2 −0 vm/signal.hpp
View
@@ -80,6 +80,7 @@ namespace rubinius {
, process_list_(NULL)
, iterator_(NULL)
, process_item_kind_(eRuby)
+ , paused_(false)
, exit_(false)
, finishing_(false)
{
@@ -92,6 +93,8 @@ namespace rubinius {
live_guard_.init();
worker_lock_.init();
worker_cond_.init();
+ pause_cond_.init();
+
supervisor_lock_.init();
supervisor_cond_.init();
}
@@ -114,6 +117,7 @@ namespace rubinius {
if(self_) return;
utilities::thread::Mutex::LockGuard lg(worker_lock_);
self_ = state->shared().new_vm();
+ paused_ = false;
exit_ = false;
thread_.set(Thread::create(state, self_, G(thread), finalizer_handler_tramp, false, true));
run(state);
@@ -150,20 +154,28 @@ namespace rubinius {
}
void FinalizerHandler::before_fork(STATE) {
- stop_thread(state);
+ utilities::thread::Mutex::LockGuard lg(worker_lock_);
+ while(!paused_) {
+ pause_cond_.wait(worker_lock_);
+ }
}
void FinalizerHandler::after_fork_parent(STATE) {
- start_thread(state);
+ utilities::thread::Mutex::LockGuard lg(worker_lock_);
+ pause_cond_.signal();
}
void FinalizerHandler::after_fork_child(STATE) {
live_guard_.init();
worker_lock_.init();
worker_cond_.init();
+ pause_cond_.init();
supervisor_lock_.init();
supervisor_cond_.init();
+ VM::discard(state, self_);
+ self_ = NULL;
+
start_thread(state);
}
@@ -182,18 +194,26 @@ namespace rubinius {
if(!process_list_) first_process_item();
if(!process_list_) {
- utilities::thread::Mutex::LockGuard lg(worker_lock_);
-
- if(finishing_) supervisor_signal();
-
- // exit_ might have been set in the mean while after
- // we grabbed the worker_lock
- if(exit_) break;
- state->gc_independent(gct);
- worker_wait();
- if(exit_) break;
+ {
+ utilities::thread::Mutex::LockGuard lg(worker_lock_);
+
+ if(finishing_) supervisor_signal();
+
+ // exit_ might have been set in the mean while after
+ // we grabbed the worker_lock
+ if(exit_) break;
+ state->gc_independent(gct);
+ paused_ = true;
+ pause_cond_.signal();
+ worker_wait();
+ if(exit_) break;
+ }
state->gc_dependent();
- if(exit_) break;
+ {
+ utilities::thread::Mutex::LockGuard lg(worker_lock_);
+ paused_ = false;
+ if(exit_) break;
+ }
continue;
}
View
@@ -95,8 +95,10 @@ namespace rubinius {
utilities::thread::Mutex live_guard_;
utilities::thread::Mutex worker_lock_;
utilities::thread::Condition worker_cond_;
+ utilities::thread::Condition pause_cond_;
utilities::thread::Mutex supervisor_lock_;
utilities::thread::Condition supervisor_cond_;
+ bool paused_;
bool exit_;
bool finishing_;
View
@@ -35,6 +35,7 @@ namespace rubinius {
, target_(state->vm())
, self_(NULL)
, queued_signals_(0)
+ , paused_(false)
, exit_(false)
, thread_(state)
{
@@ -50,6 +51,7 @@ namespace rubinius {
worker_lock_.init();
worker_cond_.init();
+ pause_cond_.init();
start_thread(state);
}
@@ -63,6 +65,7 @@ namespace rubinius {
if(self_) return;
utilities::thread::Mutex::LockGuard lg(worker_lock_);
self_ = state->shared().new_vm();
+ paused_ = false;
exit_ = false;
thread_.set(Thread::create(state, self_, G(thread), signal_handler_tramp, false, true));
run(state);
@@ -105,16 +108,24 @@ namespace rubinius {
}
void SignalHandler::before_fork(STATE) {
- stop_thread(state);
+ utilities::thread::Mutex::LockGuard lg(worker_lock_);
+ while(!paused_) {
+ pause_cond_.wait(worker_lock_);
+ }
}
void SignalHandler::after_fork_parent(STATE) {
- start_thread(state);
+ utilities::thread::Mutex::LockGuard lg(worker_lock_);
+ pause_cond_.signal();
}
void SignalHandler::after_fork_child(STATE) {
worker_lock_.init();
worker_cond_.init();
+ pause_cond_.init();
+
+ VM::discard(state, self_);
+ self_ = NULL;
start_thread(state);
}
@@ -137,15 +148,23 @@ namespace rubinius {
state->vm()->thread->hard_unlock(state, gct);
while(!exit_) {
- utilities::thread::Mutex::LockGuard lg(worker_lock_);
- if(exit_) break;
- state->gc_independent(gct);
- worker_cond_.wait(worker_lock_);
- // If we should exit now, don't try to become
- // dependent first but break and exit the thread
- if(exit_) break;
+ {
+ utilities::thread::Mutex::LockGuard lg(worker_lock_);
+ if(exit_) break;
+ state->gc_independent(gct);
+ paused_ = true;
+ pause_cond_.signal();
+ worker_cond_.wait(worker_lock_);
+ // If we should exit now, don't try to become
+ // dependent first but break and exit the thread
+ if(exit_) break;
+ }
state->gc_dependent();
- if(exit_) break;
+ {
+ utilities::thread::Mutex::LockGuard lg(worker_lock_);
+ if(exit_) break;
+ paused_ = false;
+ }
target_->set_check_local_interrupts();
target_->wakeup(state, gct);
View
@@ -24,13 +24,15 @@ namespace rubinius {
int pending_signals_[NSIG];
int queued_signals_;
+ bool paused_;
bool exit_;
TypedRoot<Thread*> thread_;
std::list<int> watched_signals_;
utilities::thread::Condition worker_cond_;
+ utilities::thread::Condition pause_cond_;
utilities::thread::Mutex worker_lock_;
public:

0 comments on commit 4b66fcb

Please sign in to comment.