Uses a GCLockGuard for the contention_lock

This prevents a possible deadlock described in issue #1372. Fixes #1372
rubinius · Oct 28, 2011 · e5b8bb3 · e5b8bb3
1 parent 466fcb3
commit e5b8bb3
Show file tree

Hide file tree

Showing 8 changed files with 22 additions and 19 deletions.
diff --git a/vm/builtin/thread.cpp b/vm/builtin/thread.cpp
@@ -206,7 +206,7 @@ namespace rubinius {
     return Qnil;
   }
 
-  Object* Thread::raise(STATE, Exception* exc) {
+  Object* Thread::raise(STATE, GCToken gct, Exception* exc) {
     OnStack<1> os(state, exc);
 
     thread::SpinLock::LockGuard lg(init_lock_);
@@ -216,27 +216,30 @@ namespace rubinius {
 
     vm->register_raise(state, exc);
 
-    vm->wakeup(state);
+    vm->wakeup(state, gct);
     return exc;
   }
 
   Object* Thread::set_priority(STATE, Fixnum* new_priority) {
     return new_priority;
   }
 
-  Thread* Thread::wakeup(STATE) {
+  Thread* Thread::wakeup(STATE, GCToken gct) {
     thread::SpinLock::LockGuard lg(init_lock_);
 
     if(alive() == Qfalse || !vm_) {
       return reinterpret_cast<Thread*>(kPrimitiveFailed);
     }
 
     VM* vm = vm_;
-    if(!vm) return this;
+    Thread* self = this;
+    OnStack<1> os(state, self);
 
-    vm->wakeup(state);
+    if(!vm) return self;
 
-    return this;
+    vm->wakeup(state, gct);
+
+    return self;
   }
 
   Tuple* Thread::context(STATE) {

diff --git a/vm/builtin/thread.hpp b/vm/builtin/thread.hpp
@@ -142,7 +142,7 @@ namespace rubinius {
      *  Process an exception raised for this Thread.
      */
     // Rubinius.primitive :thread_raise
-    Object* raise(STATE, Exception* exc);
+    Object* raise(STATE, GCToken gct, Exception* exc);
 
     /**
      *  Set the priority for this Thread.
@@ -162,7 +162,7 @@ namespace rubinius {
      *  is queued to be run, although not necessarily immediately.
      */
     // Rubinius.primitive :thread_wakeup
-    Thread* wakeup(STATE);
+    Thread* wakeup(STATE, GCToken gct);
 
     // Rubinius.primitive :thread_context
     Tuple* context(STATE);

diff --git a/vm/objectmemory.cpp b/vm/objectmemory.cpp
@@ -164,7 +164,7 @@ namespace rubinius {
     struct timespec ts = {0,0};
 
     {
-      thread::Mutex::LockGuard lg(contention_lock_);
+      GCLockGuard lg(state, gct, contention_lock_);
 
       // We want to lock obj, but someone else has it locked.
       //
@@ -281,8 +281,8 @@ namespace rubinius {
     }
   }
 
-  void ObjectMemory::release_contention(STATE) {
-    thread::Mutex::LockGuard lg(contention_lock_);
+  void ObjectMemory::release_contention(STATE, GCToken gct) {
+    GCLockGuard lg(state, gct, contention_lock_);
     contention_var_.broadcast();
   }
 

diff --git a/vm/objectmemory.hpp b/vm/objectmemory.hpp
@@ -312,7 +312,7 @@ namespace rubinius {
     Integer* assign_object_id_ivar(STATE, Object* obj);
     bool inflate_lock_count_overflow(STATE, ObjectHeader* obj, int count);
     LockStatus contend_for_lock(STATE, GCToken gct, ObjectHeader* obj, bool* error, size_t us=0);
-    void release_contention(STATE);
+    void release_contention(STATE, GCToken gct);
     bool inflate_and_lock(STATE, ObjectHeader* obj);
     bool inflate_for_contention(STATE, ObjectHeader* obj);
 

diff --git a/vm/oop.cpp b/vm/oop.cpp
@@ -399,7 +399,7 @@ namespace rubinius {
             if(!state->om->inflate_for_contention(state, this)) continue;
 
             state->del_locked_object(this);
-            state->om->release_contention(state);
+            state->om->release_contention(state, gct);
 
             return eUnlocked;
           }
@@ -487,7 +487,7 @@ namespace rubinius {
         if(new_val.f.LockContended == 1) {
           // If we couldn't inflate for contention, redo.
           if(!state->om->inflate_for_contention(state, this)) continue;
-          state->om->release_contention(state);
+          state->om->release_contention(state, gct);
         }
 
         return;

diff --git a/vm/signal.cpp b/vm/signal.cpp
@@ -106,6 +106,7 @@ namespace rubinius {
     pthread_sigmask(SIG_BLOCK, &set, NULL);
 #endif
 
+    GCTokenImpl gct;
     thread::Thread::set_os_name("rbx.signal-dispatch");
 
     for(;;) {
@@ -137,7 +138,7 @@ namespace rubinius {
         {
           target_->check_local_interrupts = true;
           target_->get_attention();
-          target_->wakeup(state);
+          target_->wakeup(state, gct);
 
         }
       }

diff --git a/vm/vm.cpp b/vm/vm.cpp
@@ -375,13 +375,13 @@ namespace rubinius {
     interrupt_with_signal_ = true;
   }
 
-  bool VM::wakeup(STATE) {
+  bool VM::wakeup(STATE, GCToken gct) {
     SYNC(state);
 
     check_local_interrupts = true;
 
     // Wakeup any locks hanging around with contention
-    om->release_contention(this);
+    om->release_contention(this, gct);
 
     if(interrupt_with_signal_) {
 #ifdef RBX_WINDOWS
@@ -401,7 +401,6 @@ namespace rubinius {
 
       if(!chan->nil_p()) {
         UNSYNC;
-        GCTokenImpl gct;
         chan->send(state, gct, Qnil);
         return true;
       } else if(custom_wakeup_) {

diff --git a/vm/vm.hpp b/vm/vm.hpp
@@ -384,7 +384,7 @@ namespace rubinius {
     void wait_on_inflated_lock(InflatedHeader* ih);
     void wait_on_custom_function(void (*func)(void*), void* data);
     void clear_waiter();
-    bool wakeup(STATE);
+    bool wakeup(STATE, GCToken gct);
     bool waiting_p();
 
     void set_sleeping();