test: cover Environments sharing an embedder-owned isolate - #66239
Open
codebytere wants to merge 5 commits into
Open
codebytere wants to merge 5 commits into
codebytere wants to merge 5 commits into
Conversation
codebytere
added
c++
Collaborator
|
Review requested:
|
nodejs-github-bot
added
lib / src
codebytere
marked this pull request as ready for review
September 23, 2026 12:59
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #66239 +/- ##
==========================================
+ Coverage 90.28% 90.31% +0.02%
==========================================
Files 789 789
Lines 272878 272880 +2
Branches 52109 52104 -5
==========================================
+ Hits 246363 246442 +79
+ Misses 16979 16903 -76
+ Partials 9536 9535 -1
🚀 New features to boost your workflow:
|
jasnell
approved these changes
Sep 23, 2026
The FreeEnvironment() fix for sibling Environments keeps the depth of nested Environment::CleanupHandles() calls on the IsolateData, so that InternalCallbackScope can re-allow JavaScript for sibling Environments while one of them is being freed. Environments that each have their own IsolateData on the same isolate and loop never see that counter and still fail with "illegal access". Environments that share a loop share a thread, so keep the depth in a thread_local instead. Refs: nodejs#65977 Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
…out one An Environment created with kNoCreateInspector threw a bare string from inspector.Session#connect(), inspector.open() and the other Agent entry points, so callers could not tell the condition apart by error code. Use the ERR_INSPECTOR_NOT_AVAILABLE code that connectToMainThread() already throws for the same situation. Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
FreeIsolateData() while an Environment created from it is still alive left that Environment with a dangling pointer and failed later in unrelated code. Count the Environments using an IsolateData and CHECK in its destructor that none are left. Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere
force-pushed
the
test-multi-env-shared-isolate
branch
from
September 23, 2026 15:41
f55fe32 to
df2ae81
Compare
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
3 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.
Refs: #65977
Node.js supports running several Environments on one isolate and event loop, including an isolate the embedder created itself, but no test builds that setup. The crashes fixed in it over the past month were all found by embedders.
This adds a cctest that does: it creates the isolate, CppHeap and contexts the way an embedder would, starts a few Environments that keep timers running, and then frees, stops and inspects them one at a time while checking that the others keep working. It runs once with a shared
IsolateDataand once with one per Environment. Reverting any of the recent fixes makes it fail.Writing it turned up three small issues, fixed in separate commits:
IsolateData; its counter is now per thread.node:inspector; it now throwsERR_INSPECTOR_NOT_AVAILABLE.IsolateDatabefore its Environments now fails aCHECKinstead of leaving a dangling pointer.It also rewrites the multiple-Environments section of embedding.md to describe the supported setup.
Disclosure: the code, tests, docs and this description were written by Claude Code, directed and reviewed by @codebytere.