From 5549616628f3b8e8b74da56eb15ffbbb00943395 Mon Sep 17 00:00:00 2001 From: InauguralPhysicist Date: Wed, 29 Jul 2026 01:24:15 -0500 Subject: [PATCH] fix(tasks): a task_yield on one thread no longer suspends every other (#739 item 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cooperative scheduler lives on EigsThread — tasks are single-threaded by construction — but the suspend request that drives it was a plain global, set at four sites and polled by every vm_run on every thread at CASE(CALL). So a task_yield on the main thread drove an unrelated OS worker's next call into vm_suspend_halt. Having no scheduler of its own, the victim's task_save_slice(task_current_running()) was handed NULL and saved nothing, and vm_run returned NULL mid-evaluation with its frames deliberately undrained (the suspend path preserves them on purpose). A spawned worker therefore produced `null` instead of its result, silently. Reproduced with a control — two workers each computing 400000 while main drives a task_yield loop: workers + main yielding w1=null w2=null (3 of 3 runs) identical program, no yields w1=400000 w2=400000 (3 of 3 runs) The flag is now per-thread. It sits on EigsThread beside the task_sched it drives rather than inside TaskScheduler as the issue sketched: the poll is on the hot CASE(CALL) path, and inside the scheduler it would need `g_task_sched && ((TaskScheduler*)g_task_sched)->suspend_request` — two dependent loads plus a branch, paid by every thread that has no scheduler at all, which is nearly all of them. On EigsThread it stays a single load off the already-hot eigs_current, no NULL check. tests/test_tasks.eigs gains the case; the suite had none where cooperative tasks and OS threads ran at the same time. Validated red: "expected 400000, got null" on both workers without the fix. Gates: release 3274/3274; asan+ubsan 3278/3278, leak tally 0; TSan 11/11 including its seeded-race self-validation (the gate is live); tasks 79/79; freestanding stage 1+2 OK; jit-smoke; embed stack soak OK. Refs #739 Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 19 +++++++++++++++++++ docs/CONCURRENCY.md | 17 +++++++++++++++++ docs/SPEC.md | 8 ++++++++ src/eigenscript.h | 9 +++++++++ src/vm.c | 2 -- src/vm.h | 4 +++- tests/test_tasks.eigs | 27 +++++++++++++++++++++++++++ 7 files changed, 83 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 92c147cb..6c43f0de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -171,6 +171,25 @@ All notable changes to EigenScript are documented here. ### Fixed +- **A `task_yield` on one thread silently made every other thread return `null` + (#739 item 3).** The cooperative scheduler lives on `EigsThread` — tasks are + single-threaded by construction — but the suspend request that drives it was + a plain global, set at four sites and polled by every `vm_run` on every + thread at `CASE(CALL)`. So a `task_yield` on the main thread drove an + unrelated OS worker's next call into `vm_suspend_halt`. Having no scheduler + of its own, the victim's `task_save_slice(task_current_running())` was handed + NULL and saved nothing, and `vm_run` returned NULL mid-evaluation with its + frames deliberately undrained (the suspend path preserves them on purpose) — + so a spawned worker silently produced `null` instead of its result. Measured: + a worker computing 400000 returned `null` on 3 of 3 runs while main was + yielding, and 400000 on 3 of 3 with the identical program minus the yields. + The flag is now per-thread. It sits on `EigsThread` beside the `task_sched` + it drives rather than inside `TaskScheduler` as first sketched, so the hot + `CASE(CALL)` poll stays a single load off the already-hot `eigs_current` + with no NULL check for the (overwhelmingly common) no-scheduler thread. + `tests/test_tasks.eigs` covers it; the suite had no case where cooperative + tasks and OS threads ran at the same time. + - **One `exit of N` permanently disabled `try`/`catch` for the rest of the process (#739 item 2).** `exit` is deliberately uncatchable: `builtin_exit` sets `g_exit_requested` and `CHECK_ERROR` consults it to refuse routing the diff --git a/docs/CONCURRENCY.md b/docs/CONCURRENCY.md index 4f8f23b5..3c55941c 100644 --- a/docs/CONCURRENCY.md +++ b/docs/CONCURRENCY.md @@ -49,6 +49,23 @@ unsafe pattern — unsynchronized read-modify-write on a shared list from two workers — is caught by the ThreadSanitizer gate below (`tests/tsan_seeded_race.eigs`). +## Cooperative tasks are per-thread + +`task_spawn`/`task_yield` (#408) are a *different* model from `spawn`: +one OS thread, round-robin, deterministic by construction. The two +compose, and the scoping is what makes that safe — the scheduler, its +ready queue, and the suspend request that drives it all live on +`EigsThread`. A `task_yield` hands control to the next task **on the +calling thread only**; a `spawn`ed worker with no tasks of its own is +never suspended by someone else's yield. + +That last part was a bug until #739: the suspend request was a plain +global polled by every `vm_run` at `CASE(CALL)`, so one thread's +`task_yield` sent an unrelated worker into the suspend path. Having no +scheduler, the victim saved no slice and its `vm_run` returned `null` +mid-evaluation — a worker silently produced `null` instead of its +result. `tests/test_tasks.eigs` now runs both models at once. + ## The multithreaded performance cliff The first `spawn` in a program permanently flips the runtime into diff --git a/docs/SPEC.md b/docs/SPEC.md index 960f5283..eddddc40 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -1255,6 +1255,14 @@ result (or re-raises its uncaught error as the same `{kind, message, line}` dict — see [Error handling](#error-handling)). `task_alive of id` is 1 until the task finishes. +Tasks are scoped to the OS thread that spawned them: each thread has its +own scheduler and its own ready queue, so `task_yield` hands control to +the next task **on this thread** and never disturbs another. Mixing the +two models is therefore well-defined — a `spawn`ed worker runs its own +program while the parent interleaves tasks. (Before #739 the suspend +request was process-wide, so one thread's `task_yield` made every other +thread's next call return `null` mid-evaluation.) + ```eigenscript order is [] define step(tag) as: diff --git a/src/eigenscript.h b/src/eigenscript.h index 4dfe0f89..fce38d0e 100644 --- a/src/eigenscript.h +++ b/src/eigenscript.h @@ -725,6 +725,14 @@ struct EigsThread { * first task_spawn, freed at thread detach. Per-OS-thread because tasks * are single-threaded by construction. */ void *task_sched; + /* #739: the suspend request that drives task_sched. Per-OS-thread for the + * same reason the scheduler is — it was a plain global, so a task_yield on + * one thread drove EVERY other thread's next CALL into vm_suspend_halt: + * with no scheduler of its own the victim saved nothing and its vm_run + * silently returned NULL mid-evaluation, frames deliberately undrained. + * Lives here rather than inside TaskScheduler so the CASE(CALL) poll stays + * one load off the already-hot eigs_current, with no NULL check. */ + int task_suspend_request; /* Registry list — set by eigs_thread_attach. */ EigsThread *next; }; @@ -808,6 +816,7 @@ extern __thread EigsThread *eigs_current; #define g_json_depth (eigs_current->json_depth) #define g_native_call_depth (eigs_current->native_call_depth) #define g_task_sched (eigs_current->task_sched) +#define g_task_suspend_request (eigs_current->task_suspend_request) #define g_entry_threshold (eigs_current->state->jit_entry_threshold) #define g_iter_threshold (eigs_current->state->jit_iter_threshold) #define g_osr_threshold (eigs_current->state->jit_osr_threshold) diff --git a/src/vm.c b/src/vm.c index 456c6777..b20c42d2 100644 --- a/src/vm.c +++ b/src/vm.c @@ -5570,8 +5570,6 @@ static Value *vm_run(EigsChunk *chunk, Env *env) { * interleaving is a pure function of program order. * ======================================================================== */ -int g_task_suspend_request = 0; - #define TASK_READY_MAX HANDLE_TABLE_SIZE typedef struct { diff --git a/src/vm.h b/src/vm.h index 72bede69..214169d5 100644 --- a/src/vm.h +++ b/src/vm.h @@ -557,7 +557,9 @@ void task_free(Task *t); * above the outermost vm_execute, so C-stack depth stays flat across any * number of task switches. Suspension is deterministic-by-construction — no * tape records. */ -extern int g_task_suspend_request; /* set by a suspending builtin; actioned at CASE(CALL) */ +/* g_task_suspend_request (set by a suspending builtin, actioned at CASE(CALL)) + * is a per-thread bridge macro in eigenscript.h — #739: as a global it drove + * other threads' CALL sites into a suspend they never asked for. */ void task_sched_on_spawn(int id); /* enqueue a freshly spawned task, arm the scheduler */ void task_request_yield(void); /* current task → tail of the ready queue */ int task_request_join(int target); /* current task blocks on `target`; 0 = bad target */ diff --git a/tests/test_tasks.eigs b/tests/test_tasks.eigs index 7b347806..1c2d50f5 100644 --- a/tests/test_tasks.eigs +++ b/tests/test_tasks.eigs @@ -499,4 +499,31 @@ assert_eq of [task_join of _det_t3, null, "self-detached task is reaped at finis assert_eq of [task_detach of 0, 0, "task_detach of main is refused"] assert_eq of [task_detach of 99999, 0, "task_detach of an unknown id is refused"] +# #739 item 3: a task_yield on THIS thread must not suspend another OS thread. +# The suspend request used to be a plain global while the scheduler it drives +# is per-thread, so every other thread's next CALL saw the flag and jumped to +# vm_suspend_halt. Having no scheduler of its own, the victim saved no slice +# and its vm_run returned NULL mid-evaluation — a spawned worker silently +# produced `null` instead of its result, with its frames deliberately +# undrained. Measured before the fix: null on 3 of 3 runs; 400000 after. +# The counts are sized so main is still yielding while the workers run. +define _susp_busy(n) as: + local acc is 0 + local i is 0 + loop while i < 400000: + acc is acc + (abs of (0 - 1)) + i is i + 1 + return acc + +_susp_w1 is spawn of [_susp_busy, 1] +_susp_w2 is spawn of [_susp_busy, 2] +_susp_m is 0 +loop while _susp_m < 100000: + task_yield of null + _susp_m is _susp_m + 1 +assert_eq of [thread_join of _susp_w1, 400000, + "task_yield does not suspend another OS thread (#739)"] +assert_eq of [thread_join of _susp_w2, 400000, + "second worker also unaffected by a foreign suspend request"] + test_summary of null