Repository navigation
worker: make terminate() abort a worker running synchronous JS - #1197
Conversation
Reaping a worker (terminate(), the Worker finalizer, parent teardown) only sent its stop async and joined the thread, so a worker stuck in synchronous JS never returned to its loop and the parent blocked forever. Worker runtimes now install a QuickJS interrupt handler that returns a cross-thread 'terminating' flag, set by the reap path (only; self.close() must let the current task finish) before it stops the loop. The resulting uncatchable interrupt is not reported: no error event in the worker or on the parent Worker, and nothing on stderr. The handler is removed after the join, before TJS_FreeRuntime runs teardown JS on the parent thread. Atomics.wait() without a timeout is still not interruptible (#1196).
|
Deep review (round 1) @ 930f571 — changes requested. The design, memory ordering and reap → join → handler removal → free ordering all check out, and a 14-scenario adversarial set (nested busy workers, interrupts inside One defect: the top-level synchronous abort is not silent. quickjs-ng's |
Module evaluation turns the interrupt that aborts a busy module body into a rejection of the module promise, so the abort reached the unhandledrejection path, which was not gated on the terminating flag: a worker listener could observe 'interrupted' after terminate(), and an interrupt landing inside the event construction or dispatch was printed to stderr. Drop pending rejections (and don't dump errors raised while dispatching them) once the worker is being terminated. The test helper now installs self.onunhandledrejection so the empty-stderr assertion covers this path.
|
Deep review round 2 (deep-reviewer) — approved at head d90dfe3. The round-1 finding (the top-level abort reaching the What I ran on this exact head (macOS x86_64; Release, Debug w/ JS_DUMP_LEAKS, ASan builds, all confirmed up to date with the checkout):
Design decisions 1–7 and every acceptance criterion on the card are met; the PR body (squash message) describes the fourth gate and says |
… drain (#1201) When a Worker stops, the parent reaps it in `worker_reap()`: `tjs__runtime_terminate()`, `uv_thread_join()`, then `TJS_FreeRuntime()`. The drain inside `TJS_FreeRuntime()` runs **on the parent thread** and can run the worker's JS. For example, it waits for in-flight threadpool requests and then runs their promise continuations. `worker_reap()` removed the worker's interrupt handler just before the drain, so a continuation that never returned hung the parent for good: - **`terminate()`**: `w.terminate()` never returned. - **`self.close()`**: the reap runs from `worker_exit_cb` with no `terminate()` call. The parent's whole event loop froze, and its timers never fired. This predates #1197: v26.11.0 hangs on the `terminate()` case too. ### Fix `worker_reap()` no longer calls `JS_SetInterruptHandler(rt, NULL, NULL)`. `terminating` is already set on every reap path, so JS the drain runs is aborted at its next interrupt poll. The handler only reads the atomic flag, so it is safe to call from the parent thread. The comments in `worker.c` and on the handler in `vm.c` now say so. The old rationale (an error path would `TJS_Stop` the already-closed `stop` handle) no longer holds, because `TJS_Stop()` checks `uv_is_closing()`. I checked that the drain-time abort stays silent. The interrupt surfaces in `tjs__drain_microtasks()` / `tjs_call_handler()` as a failed job: `tjs__worker_handle_uncaught()` returns early, because `error_event_ctor` is cleared at the start of `TJS_FreeRuntime` and `terminating` is set. `tjs__worker_post_error()` returns early because `internal_message_pipe` is cleared. The rejection tracker is removed and the check handle that runs `tjs__execute_jobs` is closed, so nothing dispatches `unhandledrejection` or prints. `tjs_dump_error` is only reached from `TJS_Run`, `tjs__execute_jobs` (already gated) and module eval, and none of those runs in the drain. So no new gating was needed. ### Behaviour change After either `terminate()` or `self.close()`, JS that the teardown drain runs for a reaped worker is cut off at the next interrupt poll instead of running to completion. Short continuations normally still finish, because QuickJS polls only every N ops. ### Tests New modes in `tests/test-worker-terminate-busy.js` / `tests/helpers/worker-terminate-busy.js`. They use the existing subprocess plus 10 s watchdog, so a hang fails the test instead of hitting the runner timeout: - `drain-terminate`: the worker starts a 64 MiB SHA-512 digest whose `.then` busy-loops and posts `'busy'`. The parent calls `terminate()`. The test expects exit 0, `terminate returned` and empty stderr. - `drain-close`: the same worker, plus `self.close()`. The parent schedules a timer on `'busy'`. The test expects exit 0, exactly `parent alive` and empty stderr, which shows the parent loop recovered. With the one-line fix reverted, both modes hang until the watchdog kills them. ### Verification (macOS x86_64) - Repro from the card, against a Release build of master ac97a67 (before) and of this branch (after). It runs `tjs run mk.js <mode> <src>` under a 10 s watchdog: - before: terminate BUSY rc=124, terminate IDLE rc=0 `terminate returned`, close BUSY rc=124, close IDLE rc=0 `parent timer fired` - after: all four rc=0 with the expected output and empty stderr - `./build/tjs test tests/` (Release): 476/476 OK - Every `tests/test-worker*.js` plus `tests/test-dispose-worker.js`, run with `tjs run`, passes on a Debug build (no `JS_DUMP_LEAKS` output) and on an ASan build (RelWithDebInfo, mimalloc off) with no reports. This includes `test-worker-terminate-threadpool-*`. - `clang-format --dry-run --Werror src/worker.c src/vm.c` (18.1.8) is clean, and so is `make lint`. Refs #1196, #1200 Devteam card: t_8c9af946
What
Worker.terminate()(and every other path that reaps a worker: the Worker finalizer and parent-runtime teardown) now aborts a worker that is running synchronous JavaScript, instead of blocking the calling thread forever.Why
worker_reap()only sent the worker'sstopasync and then joined its thread. A worker stuck in JS that never returns to its event loop (e.g.for (;;) {}) never processes that async, so the join never returns. Bothterminate()and a parent that dies with an uncaught error while such a worker runs hang forever. The Web Workers spec saysterminate()aborts the worker's script even mid-execution.How
struct TJSRuntimegets anatomic_bool terminating(C11<stdatomic.h>, asmod_channel.calready uses), initialised to false.tjs__runtime_terminate()storesterminating = trueand then callsTJS_Stop(), so the worker sees the flag no later than the stop.worker_reap()uses it instead ofTJS_Stop(). The public API (src/tjs.h,TJS_Stop()) is unchanged.self.close()and the worker's own uncaught-error paths keep calling plainTJS_Stop(): per spec,close()lets the current task finish, so it must not abort running JS.atomic_load(&terminating): no allocation, JS or locks. QuickJS polls it every ~10k bytecode ops and during regexp execution, and throws an uncatchableInternalError: interrupted.terminatingis set,tjs__worker_handle_uncaught()andtjs__worker_post_error()return early (noerrorevent in the worker, none on the parentWorker), andTJS_Run()frees a pending exception on a terminating worker without dumping it. Fourth gate:tjs__execute_jobs()drops pending unhandled rejections of a terminating worker (nounhandledrejectionevent, notjs_dump_error), and doesn't dump an error raised while dispatching one. This is needed because quickjs-ng's module evaluation turns the uncatchable interrupt in a busy module body into a rejection of the module promise, so a top-level abort arrives as an unhandled rejection rather than a thrown exception. The gate is the flag, notJS_IsUncatchableError, so a genuine error racing withterminate()may be dropped; either outcome of that race is fine.uv_thread_join()and beforeTJS_FreeRuntime(),worker_reap()uninstalls the handler.TJS_FreeRuntime()can run JS on the parent thread (the handle drain); with the flag still set, that JS would be interrupted halfway and an error path couldTJS_Stop()the already-closedstophandle. Teardown therefore behaves exactly as before.Out of scope: a worker blocked in
Atomics.wait()with no timeout still hangsterminate(), because QuickJS's wait is a condition-variable wait that never polls interrupts. Refs #1196.How verified (macOS x86_64)
timeout 10 ./build/tjs run hang.js: before this change printsterminating, exit 124; after it printsterminating/terminate returned, exit 0.throw new Error('boom')instead of the terminate): before, the error prints and the process hangs (exit 124); after, it prints the error and exits 1 promptly.tests/test-worker-terminate-busy.js(helpertests/helpers/worker-terminate-busy.js, spawn-with-watchdog liketest-worker-self-stop-exit.js) covers a busy loop at module top level, inonmessage, and after anawait, each followed byterminate()(expectsterminate returned, exit 0, empty stderr, noerrorevent on either side), plus a fatal parent error while the worker busy-loops (expects exit 1, nointerruptedon stderr). The worker prelude installsself.onerrorandself.onunhandledrejectionloggers, so the empty-stderr assertion also catches an abort leaking through either path. It fails (watchdog) on the pre-change binary, and passed 20/20 in a loop on Release../build/tjs test tests/on Release: all 472 tests OK.test-worker*.jsplustest-dispose-worker.js(24 files) pass on a Debug build (no JS_DUMP_LEAKS output) and an ASan build (no reports). The helper's four modes run clean on both.pnpm run lintclean.Devteam card: t_2450795f