Repository navigation
test(runtime): make the JS block dispose race specs prove the interleaving - #505
Draft
edusperoni wants to merge 2 commits into
Draft
edusperoni wants to merge 2 commits into
edusperoni wants to merge 2 commits into
Conversation
The specs for a JS block whose dispose waits on the isolate relied on a 1 ms release landing inside a 30 ms sleep. When the release slipped past the turn, the block disposed normally and the specs passed without exercising the race. The fixture now keeps blocks until the test releases them. The stranding helper drops the last reference on a background queue and waits for libclosure's deallocating flag on the block, so the spec fails instead of passing vacuously when the dispose never started. The handleof-while-held spec releases its block before it ends, and the worker spec releases the block only after the worker's nsworkerended event, once its runtime has been deleted.
State the JSBlock dispose's ownership rule, keep the Block_copy rationale next to TryRetainJSBlock's definition only, describe the handleof reference as lasting until the autorelease pool drains, and drop the bug narration from DisposeValue's Block case.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Follows up #500 with its review findings.
Specs that could pass without exercising the race
The specs for a JS block whose dispose waits on the isolate released the block 1 ms after marshalling, inside a 30 ms sleep. When that release slipped past the turn (loaded or ASan simulator), the block disposed normally.
handleofthen threw and the re-marshal built a fresh block anyway, so the specs passed vacuously.keepBlockUntilReleased:,releaseKeptBlocks).releaseKeptBlocksAwaitingDispose:drops the last reference on a global queue. It then polls libclosure's deallocating flag in the block header until it is set, and returns NO on a timeout._Block_releaseimmediately before it calls the dispose helper, so this is a deterministic "dispose has started" signal with no production hook and no sleep.strandDisposeasserts that it returned true.nsworkerendedlistener. That event is posted after the worker's runtime has been deleted, so the release lands after~Runtime, once the isolate is gone.Leftover state
The handleof-while-held spec held its block for 1 s, so the release fired during a later spec. It now releases the block synchronously before it ends and expects one release. The unused
keepBlock:forMilliseconds:andsleepMilliseconds:fixtures are removed.Comment nits
ObjectManager::DisposeValueleaves it alone), whether or not the isolate is alive.TryRetainJSBlockdocuments only its contract. The Block_copy rationale stays next to its definition in Interop.mm.Discrimination check
TryRetainJSBlockwas temporarily made toBlock_copyunconditionally and return true, which matches #500 being reverted at both the cache hit and handleof. The hammer specs were disabled for this check. The revert was not committed.Under ASan:
Expected function to throw an exception.The stranding assertion itself passed, so the dispose had started.IsolateWrapper::Isolate()fromArgConverter::MethodCallback, invoked fromNSBlockOperation. The revived block ran after its dispose freed the callback wrapper.Suite
Both match main.