From f0cf4dce65ba7bf3dc6787e10b88e08b411e2c92 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Fri, 6 Jun 2025 09:38:57 +0900 Subject: [PATCH] Handle spurious wakeups in `Thread#join`. (#13532) --- test/fiber/scheduler.rb | 2 +- test/fiber/test_thread.rb | 41 +++++++++++++++++++++++++++++++++++++++ thread.c | 28 +++++++++++++++----------- 3 files changed, 59 insertions(+), 12 deletions(-) diff --git a/test/fiber/scheduler.rb b/test/fiber/scheduler.rb index 5782efd0d1..26a807c8c5 100644 --- a/test/fiber/scheduler.rb +++ b/test/fiber/scheduler.rb @@ -126,7 +126,7 @@ class Scheduler end ready.each do |fiber| - fiber.transfer + fiber.transfer if fiber.alive? end end end diff --git a/test/fiber/test_thread.rb b/test/fiber/test_thread.rb index 5e3cc6d0e1..0247f330d9 100644 --- a/test/fiber/test_thread.rb +++ b/test/fiber/test_thread.rb @@ -90,6 +90,47 @@ class TestFiberThread < Test::Unit::TestCase assert_equal :done, thread.value end + def test_spurious_unblock_during_thread_join + ready = Thread::Queue.new + + target_thread = Thread.new do + ready.pop + :success + end + + Thread.pass until target_thread.status == "sleep" + + result = nil + + thread = Thread.new do + scheduler = Scheduler.new + Fiber.set_scheduler scheduler + + # Create a fiber that will join a long-running thread: + joining_fiber = Fiber.schedule do + result = target_thread.value + end + + # Create another fiber that spuriously unblocks the joining fiber: + Fiber.schedule do + # This interrupts the join in joining_fiber: + scheduler.unblock(:spurious_wakeup, joining_fiber) + + # This allows the unblock to be processed: + sleep(0) + + # This allows the target thread to finish: + ready.push(:done) + end + + scheduler.run + end + + thread.join + + assert_equal :success, result + end + def test_broken_unblock thread = Thread.new do Thread.current.report_on_exception = false diff --git a/thread.c b/thread.c index 4ab36c6cff..3530173c79 100644 --- a/thread.c +++ b/thread.c @@ -1052,23 +1052,28 @@ thread_join_sleep(VALUE arg) while (!thread_finished(target_th)) { VALUE scheduler = rb_fiber_scheduler_current(); - if (scheduler != Qnil) { - rb_fiber_scheduler_block(scheduler, target_th->self, p->timeout); - // Check if the target thread is finished after blocking: - if (thread_finished(target_th)) break; - // Otherwise, a timeout occurred: - else return Qfalse; - } - else if (!limit) { - sleep_forever(th, SLEEP_DEADLOCKABLE | SLEEP_ALLOW_SPURIOUS | SLEEP_NO_CHECKINTS); + if (!limit) { + if (scheduler != Qnil) { + rb_fiber_scheduler_block(scheduler, target_th->self, Qnil); + } + else { + sleep_forever(th, SLEEP_DEADLOCKABLE | SLEEP_ALLOW_SPURIOUS | SLEEP_NO_CHECKINTS); + } } else { if (hrtime_update_expire(limit, end)) { RUBY_DEBUG_LOG("timeout target_th:%u", rb_th_serial(target_th)); return Qfalse; } - th->status = THREAD_STOPPED; - native_sleep(th, limit); + + if (scheduler != Qnil) { + VALUE timeout = rb_float_new(hrtime2double(*limit)); + rb_fiber_scheduler_block(scheduler, target_th->self, timeout); + } + else { + th->status = THREAD_STOPPED; + native_sleep(th, limit); + } } RUBY_VM_CHECK_INTS_BLOCKING(th->ec); th->status = THREAD_RUNNABLE; @@ -1749,6 +1754,7 @@ io_blocking_operation_exit(VALUE _arguments) rb_fiber_t *fiber = io->closing_ec->fiber_ptr; if (thread->scheduler != Qnil) { + // This can cause spurious wakeups... rb_fiber_scheduler_unblock(thread->scheduler, io->self, rb_fiberptr_self(fiber)); } else {