fix(swingset): don't un-expose 'gc', to stop intermittent test failures #6029
Conversation
This file contains 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
michaelfig
approved these changes
Aug 22, 2022
warner
added
automerge:squash
|
I figured out that the CI failure was another instance of #5575 (and I have an idea on how to deal with that), so I'll just re-kick the CI. |
mhofman
reviewed
Aug 23, 2022
Comment on lines
10
to
11
| // we leave --expose_gc turned on, otherwise AVA's shared workers | ||
| // may race and disable it before we manage to extract the binding |
There was a problem hiding this comment.
Should we leave a comment that it's safe since at worse it would just leave a global gc in new realms (which we don't create in non-test paths), and that would get removed in Compartments anyway.
warner
force-pushed
the
6028-gc-not-defined
branch
from
10ce5d7 to
49131da
Compare
engine-gc.js acquires the `gc()` function (to force garbage collection) on Node.js by turning on the (global) `--expose_gc` V8 flag, then evaluating code within a new context (because the original one might not have had `--expose_gc` turned on), stashing the `gc` global, then uses `--no-expose_gc` to hide it again. It seems that AVA's worker-based parallelism approach causes multiple calls to this routine within the same process, sharing a V8 instance. So there's a race between one worker doing enable/stash/disable and another. If they interleave, a "stash" might happen while the symbol is disabled, causing a Reference Error and a crash. This won't cause `gc` to be visible to new Compartments, because SES strips out all globals that it doesn't recognize. closes #6028
warner
force-pushed
the
6028-gc-not-defined
branch
from
49131da to
c4f7038
Compare
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.
engine-gc.js acquires the
gc()function (to force garbagecollection) on Node.js by turning on the (global)
--expose_gcV8flag, then evaluating code within a new context (because the original
one might not have had
--expose_gcturned on), stashing thegcglobal, then uses
--no-expose_gcto hide it again.It seems that AVA's worker-based parallelism approach causes multiple
calls to this routine within the same process, sharing a V8
instance. So there's a race between one worker doing
enable/stash/disable and another. If they interleave, a "stash" might
happen while the symbol is disabled, causing a Reference Error and a
crash.
closes #6028