test: give each async launch an owned callback context - #49
Open
owenthcarey wants to merge 1 commit into
Open
owenthcarey wants to merge 1 commit into
owenthcarey wants to merge 1 commit into
Conversation
The async tests passed the completion callback a raw pointer to a boxed mpsc::Sender, then freed that box on the test thread right after recv returned. recv unblocks as soon as the message is queued, before the producer thread has finished send, so the test could free the Sender and then the whole channel out from under an in-flight send. The window is tiny and load dependent, which is why it surfaced as an intermittent SIGSEGV in the weaveffi runtime tests on macOS CI and nowhere else. Every launch now gets its own boxed clone of the sender and the callback adopts it with Box::from_raw, so the channel is torn down by whichever side releases last and no borrowed reference outlives the callback. This is the ownership the ABI's "consumer owns context" rule intends, and it replaces the deliberate leak async-demo used to sidestep the same race. The generated launchers were already correct: they carry context as an opaque usize and never touch it after the callback fires.
This branch has not been deployed
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
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.
Fixes the intermittent
SIGSEGVincrates/weaveffi/tests/runtimethat hit the macOSBuild and testjob on #48 (the same tree passed onmainand on re-run).Root cause
The async tests handed the completion callback a raw pointer to a
Box<mpsc::Sender>and freed that box on the test thread right afterrecvreturned:recvunblocks as soon as the message is queued, before the producer thread has finishedsend(it still notifies waiters afterward). If the test thread frees theSenderand then theReceiverin that window, the channel is deallocated under the producer thread. The window is tiny and load dependent, which is why it only ever showed up undercargo insta test --workspaceon a busy runner.samples/async-demohad already met this race and worked around it by deliberately leaking the context (leak_ctx).crates/weaveffi/tests/runtime.rsandsamples/kvstorenever got that treatment.Fix
Test code only. Each async launch gets its own boxed clone of the sender via
ctx_for(&tx), and the callback adopts it withadopt_ctx(ctx)(Box::from_raw) and drops it when done. The channel is then torn down by whichever side releases last, through the channel's own refcount, and no borrowed reference outlives the callback. This is the ownership the ABI's "consumer ownscontext" rule intends. Theleak_ctxworkaround inasync-demois removed in favour of the same pattern.The generated launchers were already correct:
async_fns.rscarriescontextas an opaqueusize, never dereferences it, and fires the callback exactly once. No runtime or generator changes.Verification
cargo clippy -p weaveffi -p kvstore -p async-demo --all-targets -- -D warnings: cleancargo test -p weaveffi --test runtime(42),-p kvstore(26),-p async-demo(10): all pass--test-threads=16under 8 busy-loop threads: 0 failures in 300 runs