src: apply IsolateSettings when using a snapshot - #65407
Open
codebytere wants to merge 1 commit into
Open
Conversation
`NewIsolate()` deferred `SetIsolateErrorHandlers()` when snapshot data was passed, and `CreateEnvironment()` later installed the handlers with default `IsolateSettings` after deserializing the main context. An embedder's `fatal_error_callback`, `oom_error_callback`, `should_abort_on_uncaught_exception_callback` and `prepare_stack_trace_callback` were therefore dropped whenever a snapshot was used, and the per-isolate message listener was added even if `MESSAGE_LISTENER_WITH_ERROR_LEVEL` had been cleared. The only way to keep custom handlers was to call `SetIsolateUpForNode()` again after `CreateEnvironment()`. Install all handlers in `NewIsolate()` regardless of snapshot data, as its documentation already describes, and stop touching isolate handlers in `CreateEnvironment()`. The deferral dates from the initial isolate snapshot work; every handler already copes with a missing `Environment`, since without a snapshot they are installed before any context exists, and workers have been calling `SetIsolateUpForNode()` right after a snapshot `NewIsolate()` anyway. Refs: nodejs#27321 Refs: nodejs#45888 Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65407 +/- ##
==========================================
+ Coverage 90.10% 90.12% +0.01%
==========================================
Files 752 752
Lines 252209 252201 -8
Branches 47454 47446 -8
==========================================
+ Hits 227260 227289 +29
+ Misses 16264 16215 -49
- Partials 8685 8697 +12
🚀 New features to boost your workflow:
|
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
2 participants
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.
When
NewIsolate()is given snapshot data it applies only the misc half ofIsolateSettingsand leaves the error handlers for later;CreateEnvironment()then installs them with default settings once the main context is deserialized. So with a snapshot, an embedder'sfatal_error_callback,oom_error_callback,should_abort_on_uncaught_exception_callbackandprepare_stack_trace_callbackare dropped, and the message listener is added even ifMESSAGE_LISTENER_WITH_ERROR_LEVELwas cleared. The only workaround is callingSetIsolateUpForNode()again afterCreateEnvironment().This makes
NewIsolate()apply all of the settings whether or not there is snapshot data, as its comment innode.halready says, andCreateEnvironment()stops touching isolate handlers. The deferral came in with #27321, but every handler copes with there being noEnvironmentyet (without a snapshot they have always been installed before any context exists), and workers already callSetIsolateUpForNode()straight after a snapshotNewIsolate(), so that call goes too.Tests:
embedtestgets an--embedder-isolate-settingsmode that goes throughNewIsolate()/CreateIsolateData()/CreateEnvironment()with a customprepare_stack_trace_callback, sinceCommonEnvironmentSetuptakes no settings; the newtest-embedding-snapshot-isolate-settings.jschecks that callback still formatsnew Error().stackonce the Environment is created from a snapshot; it fails onmainand passes here. Embedding suite, cctest and the default suite pass.Refs: #27321
Refs: #45888
Disclosure: the code, test and this description were written by Claude Code, directed and reviewed by @codebytere.