src: apply IsolateSettings when using a snapshot - #65407
Open
codebytere wants to merge 1 commit into
Open
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
`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>
Copy link
Copy Markdown
Codecov Report✅ All modified and coverable lines are covered by tests. @@ 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
... and 35 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
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
Reviewers
At least 1 approving review is required to merge this pull request.
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 of IsolateSettings and 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's fatal_error_callback, oom_error_callback, should_abort_on_uncaught_exception_callback and prepare_stack_trace_callback are dropped, and the message listener is added even if MESSAGE_LISTENER_WITH_ERROR_LEVEL was cleared. The only workaround is calling SetIsolateUpForNode() again after CreateEnvironment().
This makes NewIsolate() apply all of the settings whether or not there is snapshot data, as its comment in node.h already says, and CreateEnvironment() stops touching isolate handlers. The deferral came in with #27321, but every handler copes with there being no Environment yet (without a snapshot they have always been installed before any context exists), and workers already call SetIsolateUpForNode() straight after a snapshot NewIsolate(), so that call goes too.
Tests: embedtest gets an --embedder-isolate-settings mode that goes through NewIsolate()/CreateIsolateData()/CreateEnvironment() with a custom prepare_stack_trace_callback, since CommonEnvironmentSetup takes no settings; the new test-embedding-snapshot-isolate-settings.js checks that callback still formats new Error().stack once the Environment is created from a snapshot; it fails on main and 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.