Skip to content

Commit 7e81bf5

Browse files
authored
Fix sleep spurious wakeup from sigchld (ruby#15802)
When sleeping with `sleep`, currently the main thread can get woken up from sigchld from any thread (subprocess exited). The timer thread wakes up the main thread when this happens, as it checks for signals. The main thread then executes the ruby sigchld handler if one is registered and is supposed to go back to sleep immediately. This is not ideal but it's the way it's worked for a while. In commit 8d8159e I added writes to `th->status` before and after `wait_running_turn` in `thread_sched_to_waiting_until_wakeup`, which is called from `sleep`. This is usually the right way to set the thread's status, but `sleep` is an exception because the writes to `th->status` are done in `sleep_forever`. There's a loop that checks `th->status` in `sleep_forever`. When the main thread got woken up from sigchld it saw the changed `th->status` and continued to run the main thread instead of going back to sleep. The following script shows the error. It was returning instead of sleeping forever. ```ruby t = Thread.new do sleep 0.3 `echo hello` # Spawns subprocess puts "Subprocess exited" end puts "Main thread sleeping..." result = sleep # Should block forever puts "sleep returned: #{result.inspect}" ``` Fixes [Bug #21812]
1 parent c65a554 commit 7e81bf5

File tree

2 files changed

+19
-2
lines changed

2 files changed

+19
-2
lines changed

test/ruby/test_sleep.rb

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# frozen_string_literal: false
22
require 'test/unit'
33
require 'etc'
4+
require 'timeout'
45

56
class TestSleep < Test::Unit::TestCase
67
def test_sleep_5sec
@@ -13,4 +14,21 @@ def test_sleep_5sec
1314
assert_operator(slept, :<=, 6.0, "[ruby-core:18015]: longer than expected")
1415
end
1516
end
17+
18+
def test_sleep_forever_not_woken_by_sigchld
19+
begin
20+
t = Thread.new do
21+
sleep 0.5
22+
`echo hello`
23+
end
24+
25+
assert_raise Timeout::Error do
26+
Timeout.timeout 2 do
27+
sleep # Should block forever
28+
end
29+
end
30+
ensure
31+
t.join
32+
end
33+
end
1634
end

thread_pthread.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,10 +1109,9 @@ thread_sched_to_waiting_until_wakeup(struct rb_thread_sched *sched, rb_thread_t
11091109
{
11101110
if (!RUBY_VM_INTERRUPTED(th->ec)) {
11111111
bool can_direct_transfer = !th_has_dedicated_nt(th);
1112-
th->status = THREAD_STOPPED_FOREVER;
1112+
// NOTE: th->status is set before and after this sleep outside of this function in `sleep_forever`
11131113
thread_sched_wakeup_next_thread(sched, th, can_direct_transfer);
11141114
thread_sched_wait_running_turn(sched, th, can_direct_transfer);
1115-
th->status = THREAD_RUNNABLE;
11161115
}
11171116
else {
11181117
RUBY_DEBUG_LOG("th:%u interrupted", rb_th_serial(th));

0 commit comments

Comments
 (0)