mirror of
https://github.com/ruby/ruby.git
synced 2026-10-28 16:36:20 +08:00
MN threads: reclaim a terminated coroutine via nt->dead_co, not the transfer return value
The nt scheduling loop reclaimed a terminated coroutine thread's context using the return value of coroutine_transfer() (through thread_sched_switch0), assuming it was the resuming context (the dead coroutine). That holds for the amd64 asm backend, but the ucontext backend returns the transfer *target* instead, so the loop never recognized the terminal transfer: the reclaim always saw a live thread, and the loop always unlocked the sched lock -- which the dying thread had already released in coroutine_thread_terminated -- and double-unlocked it. Benign on glibc, but FreeBSD's pthread_mutex_unlock returns EPERM (crash), and it corrupts the scheduler (hang) elsewhere. Have the terminating coroutine record its own context in nt->dead_co (per native thread) right before its final transfer; the loop reads it after switch0 returns, independent of coroutine_transfer()'s backend-dependent return value. It is set and read on the same native thread with nothing running in between, so it cannot be overwritten before it is consumed. coroutine_transfer0() and thread_sched_switch0() no longer need to return the resuming context, so revert them to void (the ASan-only local remains). Reproduced on Linux with --with-coroutine=ucontext (100% hang before, clean after); amd64 and ucontext both pass bootstraptest thread/ractor/fiber. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
1 parent
e807ba452c
commit
bcd9129c21
3 files changed
+32
-22
Notes:
git
2026-07-09 16:36:08 +00:00
No files matched your search
+24
-22
@@ -346,9 +346,9 @@ struct rb_thread_context {
|
||||
bool dead;
|
||||
};
|
||||
|
||||
static bool thread_sched_reclaim_from(struct coroutine_context *returning_from);
|
||||
static bool thread_sched_reclaim(struct coroutine_context *dead_co);
|
||||
#endif
|
||||
static struct coroutine_context *coroutine_transfer0(struct coroutine_context *transfer_from,
|
||||
static void coroutine_transfer0(struct coroutine_context *transfer_from,
|
||||
struct coroutine_context *transfer_to, bool to_dead);
|
||||
|
||||
#define thread_sched_dump(s) thread_sched_dump_(__FILE__, __LINE__, s)
|
||||
@@ -1264,7 +1264,7 @@ rb_thread_sched_init(struct rb_thread_sched *sched, bool atfork)
|
||||
#endif
|
||||
}
|
||||
|
||||
static struct coroutine_context *
|
||||
static void
|
||||
coroutine_transfer0(struct coroutine_context *transfer_from, struct coroutine_context *transfer_to, bool to_dead)
|
||||
{
|
||||
#ifdef RUBY_ASAN_ENABLED
|
||||
@@ -1279,6 +1279,7 @@ coroutine_transfer0(struct coroutine_context *transfer_from, struct coroutine_co
|
||||
__tsan_switch_to_fiber(transfer_to->tsan_fiber, 0);
|
||||
#endif
|
||||
|
||||
RBIMPL_ATTR_MAYBE_UNUSED()
|
||||
struct coroutine_context *returning_from = coroutine_transfer(transfer_from, transfer_to);
|
||||
|
||||
/* if to_dead was passed, the caller is promising that this coroutine is finished and it should
|
||||
@@ -1288,11 +1289,9 @@ coroutine_transfer0(struct coroutine_context *transfer_from, struct coroutine_co
|
||||
__sanitizer_finish_switch_fiber(transfer_from->fake_stack,
|
||||
(const void**)&returning_from->stack_base, &returning_from->stack_size);
|
||||
#endif
|
||||
|
||||
return returning_from;
|
||||
}
|
||||
|
||||
static struct coroutine_context *
|
||||
static void
|
||||
thread_sched_switch0(struct coroutine_context *current_cont, rb_thread_t *next_th, struct rb_native_thread *nt, bool to_dead)
|
||||
{
|
||||
VM_ASSERT(!nt->dedicated);
|
||||
@@ -1307,7 +1306,7 @@ thread_sched_switch0(struct coroutine_context *current_cont, rb_thread_t *next_t
|
||||
ruby_thread_set_native(next_th);
|
||||
native_thread_assign(nt, next_th);
|
||||
|
||||
return coroutine_transfer0(current_cont, next_th->sched.context, to_dead);
|
||||
coroutine_transfer0(current_cont, next_th->sched.context, to_dead);
|
||||
}
|
||||
|
||||
static void
|
||||
@@ -2436,12 +2435,16 @@ nt_start(void *ptr)
|
||||
if (next_th && next_th->nt == NULL) {
|
||||
RUBY_DEBUG_LOG("nt:%d next_th:%d", (int)nt->serial, (int)next_th->serial);
|
||||
#if USE_MN_THREADS
|
||||
struct coroutine_context *from = thread_sched_switch0(nt->nt_context, next_th, nt, false);
|
||||
if (thread_sched_reclaim_from(from)) {
|
||||
// A terminal transfer: the dying thread released
|
||||
// the sched lock before transferring (its Ractor
|
||||
// may be gone by now); we resumed with NO lock
|
||||
// held and must not touch the sched again.
|
||||
thread_sched_switch0(nt->nt_context, next_th, nt, false);
|
||||
|
||||
// If a coroutine terminated during the transfer, co_start
|
||||
// recorded it in nt->dead_co (switch0's return value is
|
||||
// backend-dependent, unusable; see thread_pthread.h).
|
||||
struct coroutine_context *dead_co = nt->dead_co;
|
||||
nt->dead_co = NULL;
|
||||
if (thread_sched_reclaim(dead_co)) {
|
||||
// it already released the sched lock before its
|
||||
// transfer (its Ractor may be gone): leave sched be.
|
||||
locked = false;
|
||||
}
|
||||
#else
|
||||
@@ -2477,16 +2480,15 @@ static int native_thread_create_shared(rb_thread_t *th);
|
||||
static void nt_free_stack(void *mstack);
|
||||
|
||||
|
||||
// Reclaim the coroutine context the nt scheduling loop just resumed from,
|
||||
// if its thread terminated. A dying thread always returns to its native
|
||||
// thread's own context (co_start's epilogue), so this is the only place
|
||||
// terminal transfers arrive -- and our running here proves the transfer's
|
||||
// register save into the block has completed. Returns true for a terminal
|
||||
// transfer, whose dying thread RELEASED the sched lock before transferring.
|
||||
// Reclaim the context a coroutine thread recorded in nt->dead_co before its
|
||||
// final transfer (co_start's epilogue). Our running here proves that transfer's
|
||||
// register save into the block completed. Returns true when a thread did
|
||||
// terminate -- it RELEASED the sched lock before transferring; NULL/false means
|
||||
// a live yield, where the loop still owns the lock.
|
||||
static bool
|
||||
thread_sched_reclaim_from(struct coroutine_context *returning_from)
|
||||
thread_sched_reclaim(struct coroutine_context *dead_co)
|
||||
{
|
||||
struct rb_thread_context *tctx = (struct rb_thread_context *)returning_from;
|
||||
struct rb_thread_context *tctx = (struct rb_thread_context *)dead_co;
|
||||
|
||||
if (tctx != NULL && tctx->dead) {
|
||||
nt_free_stack(tctx->stack);
|
||||
@@ -2512,7 +2514,7 @@ rb_threadptr_sched_free(rb_thread_t *th)
|
||||
else if (th->sched.context != NULL) {
|
||||
// a coroutine thread that never reached its epilogue (never started);
|
||||
// a terminated one is reclaimed by whoever resumed from its final
|
||||
// transfer (thread_sched_reclaim_from), and cleared this pointer.
|
||||
// transfer (thread_sched_reclaim), and cleared this pointer.
|
||||
struct rb_thread_context *tctx = (struct rb_thread_context *)th->sched.context;
|
||||
nt_free_stack(tctx->stack);
|
||||
SIZED_FREE(tctx);
|
||||
|
||||
@@ -112,6 +112,11 @@ struct rb_native_thread {
|
||||
|
||||
struct coroutine_context *nt_context;
|
||||
int dedicated;
|
||||
|
||||
// A terminating coroutine records its context here before its final
|
||||
// transfer; this nt's loop reclaims it. (Not via coroutine_transfer()'s
|
||||
// return value: its meaning differs between the amd64 asm and ucontext.)
|
||||
struct coroutine_context *dead_co;
|
||||
};
|
||||
|
||||
#undef except
|
||||
|
||||
@@ -549,6 +549,9 @@ co_start(struct coroutine_context *from, struct coroutine_context *self)
|
||||
// this native thread's own context, where the nt loop reclaims tctx.
|
||||
struct rb_thread_context *tctx = (struct rb_thread_context *)self;
|
||||
tctx->dead = true;
|
||||
// Hand this context to the nt's loop, which reclaims it right after the
|
||||
// final transfer returns from switch0.
|
||||
tctx->nt->dead_co = &tctx->co;
|
||||
coroutine_transfer0(&tctx->co, tctx->nt->nt_context, true);
|
||||
|
||||
rb_bug("unreachable");
|
||||
|
||||
Reference in new issue
Block a user