fix three runtime races the green default exposed - #625
Merged
Conversation
the last strong release wrote the dead flag with a plain u32 store and pith_struct_weak_load read it back with a plain load. structs cross task boundaries, and under the green backend tasks run concurrently on a fixed worker pool, so the writer and the reader can genuinely be on different threads: a plain read/write pair on shared memory is a data race, and the load had no ordering edge to the destructor's writes either. the flag is now an AtomicU32 like its strong/weak neighbors. the release store in pith_struct_release pairs with the acquire load in pith_struct_weak_load, so a reader that observes the target dead also observes everything its destructor did. a reader that still sees it alive gets the borrowed pointer exactly as before — the weak-read semantics are unchanged, only the flag's cross-thread visibility is now defined. adds a test that releases the last strong owner on another thread and checks the weak load here sees the death (the join provides the happens-after edge the assertion relies on).
process.rs opens with the claim that no green worker is parked on a child anywhere in the runtime, and for spawn/read/write/wait that is true — the pipes and the pidfd all go through the epoll reactor. the run-to-completion calls broke it: pith_process_output_argv, pith_exec_output, and pith_exec all drove Command::output()/status() inline, holding the calling worker for the child's whole lifetime. the default worker pool is two threads, so two concurrent process.output calls froze every other task in the program until the children chose to exit. these now go through a small dedicated pool, the same shape dns (#598) and file i/o (#601) use: the Command crosses to the pool thread as plain owned data, the Output/ExitStatus comes back the same way, the task parks in the meantime, and every pith-visible value is still built on the calling task's thread. off a green task blocking::run takes its inline arm, so the os-thread backend and main-thread callers are byte-for-byte unaffected. the pool is separate from file/dns so an unbounded child cannot queue in front of a write or a dial. tested: new unit tests drive command_output/command_status through the inline arm (exit codes, both pipes, a missing program); the pool arm is the same closure the existing blocking-pool tests already cover.
pith_random_float advanced the shared seed with a relaxed load followed by a relaxed store. that pair is not one step: two tasks drawing at once on different workers could both read the same seed, both compute the same successor, and both return the identical draw — a lost update the atomics made race-free in the rust sense but still wrong as a sequence. fetch_update makes each draw claim a distinct step of the LCG chain. relaxed ordering is all it needs — the returned value is the only payload, nothing else is published through the seed, and pith_random_seed remains a plain store since seeding is a deliberate overwrite of the sequence. tested: a reproducibility test pins the seeded single-thread sequence, and a four-thread draw asserts no two concurrent draws return the same value (the lost-update shape produced duplicates by construction; the interleaving is timing-dependent on a two-core host, so this is a regression gate rather than a reliable reproduction).
a pass over the runtime's shared state now that green is the linux default turned up three things worth fixing (previous commits) and three things whose fix is a semantics decision, recorded here instead: - os.set_env after tasks exist races glibc's own getenv on the pool threads (getaddrinfo reads the environment per lookup); both candidate fixes change observable behavior. - an fd number recycled between a handle lookup and the syscall can land the call on the wrong fd — the raw-fd hazard, on both backends. - sleep holds a green worker, and so do the loops built on it: select's probe wait, the timer workers, a context's deadline watcher. the fix is a pure-timeout wait in the reactor, which already keeps the deadline heap it would need. the green sections of both docs also catch up with the previous commit: process.output and friends no longer hold a worker, they run on a process pool while the caller parks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
summary
a concurrency audit of
cranelift/runtime/src/after green became the linuxdefault. before the flip most runtime state saw one thread, or one thread per
task; now a fixed worker pool runs many tasks, three blocking pools exist, and
a reactor wakes tasks across threads. three genuine defects came out of it.
the struct weak-reference dead flag was a plain
u32, written by the laststrong release on one thread and read by
pith_struct_weak_loadon another —a data race, and the reader could also miss the destructor's writes. it is an
atomic now, with a release/acquire pair giving the reader an ordering edge to
the destructor.
process.outputand theexecfamily held the calling green worker for thechild's entire lifetime, which contradicted the module's own documentation.
measured on a two-core box: two concurrent
process.outputcalls starved acpu-bound task for 2012 ms at one worker and 1007 ms at two — one child's
lifetime per worker. they run on a small process pool now, the same shape dns
and file i/o use, and only plain owned data crosses to a pool thread so the
handle rule still holds.
output_shellwas already fine and is untouched.the rng stepped its seed with a relaxed load/store pair, so two tasks could
step from the same seed and return identical "random" values. it is a
compare-exchange now.
what was tested
cargo test --release -p pith-runtime134 pass, five new — including athreaded test that pins the weak-reference ordering.
make teston bothbackends, both corpora at 285/285,
make memcheck,make leak-check, and thegrpc examples. the starvation fix was verified by measurement on both sides:
2012 ms to 8 ms at one worker, with the children still completing correctly.
notes
three findings are documented in
docs/limitations.mdrather than fixed,because each is a design decision rather than a repair:
os.set_envracinglibc's
getenvfrom pool threads, the raw-fd recycle window between aregistry lookup and its syscall, and
sleepholding a green worker — whichalso affects
select's probe loop, so two idle selects can occupy bothworkers. that last one wants a pure-timeout wait in the reactor, which is a
scheduling change worth deciding deliberately.
the audit's cleared list, recorded so it need not be re-derived:
blocking.rsqueue, slot and counters;
netpoll.rsregistration andon_closecoverageacross every close path including the pidfd;
SendCoroutine's pinninginvariant against all three new wakers; the handle-construction rule on pool
threads; the refcount paths; and the perf and stats counters, which are all
atomics already.