diff --git a/test/ruby/test_thread.rb b/test/ruby/test_thread.rb index ac37c43af7..6620ccbf33 100644 --- a/test/ruby/test_thread.rb +++ b/test/ruby/test_thread.rb @@ -1557,97 +1557,4 @@ q.pop assert_equal(true, t.pending_interrupt?(Exception)) assert_equal(false, t.pending_interrupt?(ArgumentError)) end - - def test_deadlock_backtrace - bug21127 = '[ruby-core:120930] [Bug #21127]' - - expected_stderr = [ - /-:12:in 'Thread#join': No live threads left. Deadlock\? \(fatal\)\n/, - /2 threads, 2 sleeps current:\w+ main thread:\w+\n/, - /\* #\n/, - :*, - /^\s*-:6:in 'Object#frame_for_deadlock_test_2'/, - :*, - /\* #\n/, - :*, - /^\s*-:2:in 'Object#frame_for_deadlock_test_1'/, - :*, - ] - - assert_in_out_err([], <<-INPUT, [], expected_stderr, bug21127) - def frame_for_deadlock_test_1 - yield - end - - def frame_for_deadlock_test_2 - yield - end - - q = Thread::Queue.new - t = Thread.new { frame_for_deadlock_test_1 { q.pop } } - - frame_for_deadlock_test_2 { t.join } - INPUT - end - - # [Bug #21342] - def test_unlock_locked_mutex_with_collected_fiber - bug21127 = '[ruby-core:120930] [Bug #21127]' - assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}") - begin; - 5.times do - m = Mutex.new - Thread.new do - m.synchronize do - end - end.join - Fiber.new do - GC.start - m.lock - end.resume - end - end; - end - - def test_unlock_locked_mutex_with_collected_fiber2 - assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}") - begin; - MUTEXES = [] - 5.times do - m = Mutex.new - Fiber.new do - GC.start - m.lock - end.resume - MUTEXES << m - end - 10.times do - MUTEXES.clear - GC.start - end - end; - end - - def test_mutexes_locked_in_fiber_dont_have_aba_issue_with_new_fibers - assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}") - begin; - mutexes = 1000.times.map do - Mutex.new - end - - mutexes.map do |m| - Fiber.new do - m.lock - end.resume - end - - GC.start - - 1000.times.map do - Fiber.new do - raise "FAILED!" if mutexes.any?(&:owned?) - end.resume - end - end; - end end diff --git a/thread.c b/thread.c index 7b7d38ec8a..9ada69efd3 100644 --- a/thread.c +++ b/thread.c @@ -445,7 +445,7 @@ rb_threadptr_unlock_all_locking_mutexes(rb_thread_t *th) th->keeping_mutexes = mutex->next_mutex; // rb_warn("mutex #<%p> was not unlocked by thread #<%p>", (void *)mutex, (void*)th); - VM_ASSERT(mutex->fiber); + const char *error_message = rb_mutex_unlock_th(mutex, th, mutex->fiber); if (error_message) rb_bug("invalid keeping_mutexes: %s", error_message); } diff --git a/thread_sync.c b/thread_sync.c index cc86889c1f..462506b20b 100644 --- a/thread_sync.c +++ b/thread_sync.c @@ -8,7 +8,6 @@ static VALUE rb_eClosedQueueError; /* Mutex */ typedef struct rb_mutex_struct { rb_fiber_t *fiber; - VALUE thread; // even if the fiber is collected, we might need access to the thread in mutex_free struct rb_mutex_struct *next_mutex; struct ccan_list_head waitq; /* protected by GVL */ } rb_mutex_t; @@ -107,6 +106,8 @@ static const char* rb_mutex_unlock_th(rb_mutex_t *mutex, rb_thread_t *th, rb_fib * */ +#define mutex_mark ((void(*)(void*))0) + static size_t rb_mutex_num_waiting(rb_mutex_t *mutex) { @@ -122,39 +123,13 @@ rb_mutex_num_waiting(rb_mutex_t *mutex) rb_thread_t* rb_fiber_threadptr(const rb_fiber_t *fiber); -static bool -locked_p(rb_mutex_t *mutex) -{ - return mutex->fiber != 0; -} - -static void -mutex_mark(void *ptr) -{ - rb_mutex_t *mutex = ptr; - VALUE fiber; - if (locked_p(mutex)) { - fiber = rb_fiberptr_self(mutex->fiber); // rb_fiber_t* doesn't move along with fiber object - if (fiber) rb_gc_mark_movable(fiber); - rb_gc_mark_movable(mutex->thread); - } -} - -static void -mutex_compact(void *ptr) -{ - rb_mutex_t *mutex = ptr; - if (locked_p(mutex)) { - mutex->thread = rb_gc_location(mutex->thread); - } -} - static void mutex_free(void *ptr) { rb_mutex_t *mutex = ptr; - if (locked_p(mutex)) { - const char *err = rb_mutex_unlock_th(mutex, rb_thread_ptr(mutex->thread), mutex->fiber); + if (mutex->fiber) { + /* rb_warn("free locked mutex"); */ + const char *err = rb_mutex_unlock_th(mutex, rb_fiber_threadptr(mutex->fiber), mutex->fiber); if (err) rb_bug("%s", err); } ruby_xfree(ptr); @@ -168,7 +143,7 @@ mutex_memsize(const void *ptr) static const rb_data_type_t mutex_data_type = { "mutex", - {mutex_mark, mutex_free, mutex_memsize, mutex_compact,}, + {mutex_mark, mutex_free, mutex_memsize,}, 0, 0, RUBY_TYPED_WB_PROTECTED | RUBY_TYPED_FREE_IMMEDIATELY }; @@ -229,13 +204,12 @@ rb_mutex_locked_p(VALUE self) { rb_mutex_t *mutex = mutex_ptr(self); - return RBOOL(locked_p(mutex)); + return RBOOL(mutex->fiber); } static void thread_mutex_insert(rb_thread_t *thread, rb_mutex_t *mutex) { - RUBY_ASSERT(!mutex->next_mutex); if (thread->keeping_mutexes) { mutex->next_mutex = thread->keeping_mutexes; } @@ -260,24 +234,10 @@ thread_mutex_remove(rb_thread_t *thread, rb_mutex_t *mutex) } static void -mutex_set_owner(VALUE self, rb_thread_t *th, rb_fiber_t *fiber) +mutex_locked(rb_thread_t *th, VALUE self) { rb_mutex_t *mutex = mutex_ptr(self); - mutex->thread = th->self; - mutex->fiber = fiber; - RB_OBJ_WRITTEN(self, Qundef, th->self); - if (fiber) { - RB_OBJ_WRITTEN(self, Qundef, rb_fiberptr_self(fiber)); - } -} - -static void -mutex_locked(rb_thread_t *th, rb_fiber_t *fiber, VALUE self) -{ - rb_mutex_t *mutex = mutex_ptr(self); - - mutex_set_owner(self, th, fiber); thread_mutex_insert(th, mutex); } @@ -298,8 +258,9 @@ rb_mutex_trylock(VALUE self) rb_fiber_t *fiber = GET_EC()->fiber_ptr; rb_thread_t *th = GET_THREAD(); + mutex->fiber = fiber; - mutex_locked(th, fiber, self); + mutex_locked(th, self); return Qtrue; } else { @@ -367,7 +328,7 @@ do_mutex_lock(VALUE self, int interruptible_p) rb_ensure(call_rb_fiber_scheduler_block, self, delete_from_waitq, (VALUE)&sync_waiter); if (!mutex->fiber) { - mutex_set_owner(self, th, fiber); + mutex->fiber = fiber; } } else { @@ -397,7 +358,6 @@ do_mutex_lock(VALUE self, int interruptible_p) rb_ractor_sleeper_threads_inc(th->ractor); rb_check_deadlock(th->ractor); - RUBY_ASSERT(!th->locking_mutex); th->locking_mutex = self; ccan_list_add_tail(&mutex->waitq, &sync_waiter.node); @@ -408,7 +368,7 @@ do_mutex_lock(VALUE self, int interruptible_p) // unlocked by another thread while sleeping if (!mutex->fiber) { - mutex_set_owner(self, th, fiber); + mutex->fiber = fiber; } rb_ractor_sleeper_threads_dec(th->ractor); @@ -422,13 +382,10 @@ do_mutex_lock(VALUE self, int interruptible_p) if (interruptible_p) { /* release mutex before checking for interrupts...as interrupt checking * code might call rb_raise() */ - if (mutex->fiber == fiber) { - mutex->thread = Qfalse; - mutex->fiber = NULL; - } + if (mutex->fiber == fiber) mutex->fiber = 0; RUBY_VM_CHECK_INTS_BLOCKING(th->ec); /* may release mutex */ if (!mutex->fiber) { - mutex_set_owner(self, th, fiber); + mutex->fiber = fiber; } } else { @@ -447,7 +404,7 @@ do_mutex_lock(VALUE self, int interruptible_p) } if (saved_ints) th->ec->interrupt_flag = saved_ints; - if (mutex->fiber == fiber) mutex_locked(th, fiber, self); + if (mutex->fiber == fiber) mutex_locked(th, self); } RUBY_DEBUG_LOG("%p locked", mutex);