Skip to content

Commit a601b89

Browse files
committed
merge revision(s) 0837263: [Backport #21926]
Fix race condition right after ubf registration Fixes [Bug #21926]
1 parent f808ff5 commit a601b89

File tree

3 files changed

+39
-10
lines changed

3 files changed

+39
-10
lines changed

test/ruby/test_thread.rb

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1649,4 +1649,32 @@ def test_mutexes_locked_in_fiber_dont_have_aba_issue_with_new_fibers
16491649
end
16501650
end;
16511651
end
1652+
1653+
# [Bug #21926]
1654+
def test_thread_join_during_finalizers
1655+
assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}", timeout: 30)
1656+
begin;
1657+
require 'open3'
1658+
1659+
class ProcessWrapper
1660+
def initialize
1661+
@stdin, @stdout, @stderr, @wait_thread = Open3.popen3("cat") # hangs until we close our stdin side
1662+
ObjectSpace.define_finalizer(self, self.class.make_finalizer(@stdin, @stdout, @stderr, @wait_thread))
1663+
end
1664+
1665+
def self.make_finalizer(stdin, stdout, stderr, wait_thread)
1666+
proc do
1667+
stdin.close rescue nil
1668+
stdout.close rescue nil
1669+
stderr.close rescue nil
1670+
wait_thread.value
1671+
end
1672+
end
1673+
end
1674+
1675+
50.times { ProcessWrapper.new }
1676+
GC.stress = true
1677+
1000.times { Object.new }
1678+
end;
1679+
end
16521680
end

thread_pthread.c

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1099,23 +1099,21 @@ thread_sched_to_waiting_until_wakeup(struct rb_thread_sched *sched, rb_thread_t
10991099

11001100
RB_VM_SAVE_MACHINE_CONTEXT(th);
11011101

1102-
if (ubf_set(th, ubf_waiting, (void *)th)) {
1103-
return;
1104-
}
11051102

11061103
RB_INTERNAL_THREAD_HOOK(RUBY_INTERNAL_THREAD_EVENT_SUSPENDED, th);
11071104

11081105
thread_sched_lock(sched, th);
11091106
{
1110-
if (!RUBY_VM_INTERRUPTED(th->ec)) {
1107+
// NOTE: there's a lock ordering inversion here with the ubf call, but it's benign.
1108+
if (ubf_set(th, ubf_waiting, (void *)th)) {
1109+
RUBY_DEBUG_LOG("th:%u interrupted", rb_th_serial(th));
1110+
}
1111+
else {
11111112
bool can_direct_transfer = !th_has_dedicated_nt(th);
11121113
// NOTE: th->status is set before and after this sleep outside of this function in `sleep_forever`
11131114
thread_sched_wakeup_next_thread(sched, th, can_direct_transfer);
11141115
thread_sched_wait_running_turn(sched, th, can_direct_transfer);
11151116
}
1116-
else {
1117-
RUBY_DEBUG_LOG("th:%u interrupted", rb_th_serial(th));
1118-
}
11191117
}
11201118
thread_sched_unlock(sched, th);
11211119

thread_pthread_mn.c

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,12 +67,15 @@ thread_sched_wait_events(struct rb_thread_sched *sched, rb_thread_t *th, int fd,
6767

6868
uint32_t event_serial = ++th->sched.event_serial; // overflow is okay
6969

70-
if (ubf_set(th, ubf_event_waiting, (void *)th)) {
71-
return false;
72-
}
7370

7471
thread_sched_lock(sched, th);
7572
{
73+
// NOTE: there's a lock ordering inversion here with the ubf call, but it's benign.
74+
if (ubf_set(th, ubf_event_waiting, (void *)th)) {
75+
thread_sched_unlock(sched, th);
76+
return false;
77+
}
78+
7679
if (timer_thread_register_waiting(th, fd, events, rel, event_serial)) {
7780
RUBY_DEBUG_LOG("wait fd:%d", fd);
7881

0 commit comments

Comments
 (0)