Skip to content

feat: add global MiniRacer pause gate - #430

Open
SamSaffron wants to merge 4 commits into
mainfrom
fork-safe
Open

feat: add global MiniRacer pause gate#430
SamSaffron wants to merge 4 commits into
mainfrom
fork-safe

Conversation

@SamSaffron

Copy link
Copy Markdown
Collaborator

Add MiniRacer.pause/resume to quiesce operations process-wide with timeout handling and nested pauses. Expose PauseTimeoutError and opt-in Process._fork hooks so fork can wait for MiniRacer to drain before parent and child continue.

Document the fork coordination APIs and cover pause, timeout, hook, and single-threaded fork behavior with tests.

@natevick

Copy link
Copy Markdown

Thanks for building this — the pause gate is a nice primitive, and install_fork_hooks! is exactly
the ergonomic I was hoping for. We gave it a run on a production-shaped workload and wanted to share
the result, since I think it turns up a case the gate can't reach.

Setup: Rails app rendering pages through execjs/mini_racer, puma --preload --fork-worker,
Ruby 3.3.12, mini_racer at ba7a72e with install_fork_hooks!(timeout: 5.0). Over 3,124 renders we
got 18 crashes (13 SIGSEGV, 5 SIGABRT) against ~23 expected untreated — a reduction, but not a fix.
Zero PauseTimeoutError, so the gate was draining cleanly every time.

The fault is in V8's platform rather than in a mini_racer operation:

absl::Mutex::Fer <- absl::CondVar::SignalAll
  <- v8::platform::DefaultWorkerThreadsTaskRunner::PostTaskImpl
  <- v8::platform::DefaultPlatform::PostTaskOnWorkerThreadImpl
  <- v8::internal::BaselineBatchCompiler::CompileBatchConcurrent
  <- v8::internal::TieringManager::OnInterruptTick

--fork-worker forks from worker 0, which has already created a Context — so v8_once_init's
pthread_once has fired and NewDefaultPlatform()'s worker threads are sitting idle on that
condvar. The child inherits waiter entries for threads that don't exist in it, and since the
pthread_once reads as already-done it never rebuilds the platform. pause waiting on
pause_state.active == 0 is satisfied throughout: the hazard isn't an in-flight operation, it's the
idle pool.

Two suggestions, both smaller than a V8 change:

1. Extend the fork guard to default-mode contexts. You already handle this for the single-threaded
runner — single_threaded_recover_after_fork reasons about it exactly ("the runner thread and any
waiters from the parent no longer exist"), and context_free_do deliberately leaks rather than hang
on inherited state. Default-mode contexts have no equivalent: the struct carries
single_threaded_pid but no pid for the v8-thread mode, so an inherited context walks into V8
instead of failing. Recording the pid and refusing cross-pid use would make that catchable:

 typedef struct Context {
     ...
+    pid_t pid;   // process that created this context; guards post-fork use
     pthread_t single_threaded_thr;
     if (!a->started) {
         if (single_threaded && (r = single_threaded_recover_after_fork(c))) {
             rendezvous_release(a);
             return (void *)(intptr_t)r;
         }
+        if (!single_threaded && c->pid != getpid()) {
+            rendezvous_release(a);
+            return (void *)(intptr_t)ESRCH;  // inherited context; v8 thread is gone
+        }

2. Say something at install_fork_hooks! time. With the default platform I don't think the gate
can be made sufficient, since the child has no way to rebuild the platform it inherited.
NewSingleThreadedDefaultPlatform() has no worker pool to inherit, and your single_threaded_test.rb
already covers fork in that mode — so recommending single-threaded for apps that fork, or warning
when hooks are installed without it, may be the more honest guidance.

Happy to share the full backtrace, or to test a patch against the same load. We're dropping
--fork-worker on our side in the meantime.

@SamSaffron

Copy link
Copy Markdown
Collaborator Author

Thanks for testing @natevick ... how is latest feeling?

Add MiniRacer.pause/resume to quiesce operations process-wide with timeout handling and nested pauses. Expose PauseTimeoutError and opt-in Process._fork hooks so fork can wait for MiniRacer to drain before parent and child continue.

Document the fork coordination APIs and cover pause, timeout, hook, and single-threaded fork behavior with tests.
Track V8 platform ownership and context state across forks, and raise
MiniRacer::ForkError when a child attempts to use unrecoverable inherited
state. Keep quiescent single-threaded contexts usable while rejecting busy
ones.

Warn when fork hooks are used with the default platform, and document why
pause hooks cannot restore its missing worker threads. Add coverage for
platform initialization races, inherited context cleanup, and safe child
initialization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants