src,test: fix and test Environments sharing an isolate by codebytere · Pull Request #66239 · nodejs/node

codebytere · 2026-09-23T12:17:32Z

embedding.md lets several Environments share an IsolateData, and with it an isolate and event loop, 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 IsolateData and 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:

The src: fix FreeEnvironment() breaking JS in sibling Environments #65977 fix didn't cover Environments with their own IsolateData; its counter is now per thread.
An Environment without an inspector threw a plain string from node:inspector; it now throws ERR_INSPECTOR_NOT_AVAILABLE.
Freeing an IsolateData before its Environments now fails a CHECK instead of leaving a dangling pointer.

It also corrects the FreeEnvironment() notes in embedding.md and node.h, which said JavaScript is disallowed on the whole isolate; only the Environment being freed loses it.

Disclosure: the code, tests, docs and this description were written by Claude Code, directed and reviewed by @codebytere.

nodejs-github-bot · 2026-09-23T12:17:38Z

Review requested:

@nodejs/inspector

codecov · 2026-09-23T14:52:36Z

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.37%. Comparing base (6dfe4eb) to head (65b3b9e).
⚠️ Report is 165 commits behind head on main.

Files with missing lines	Patch %	Lines
src/env.cc	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #66239      +/-   ##
==========================================
+ Coverage   90.28%   90.37%   +0.09%     
==========================================
  Files         789      790       +1     
  Lines      272878   273858     +980     
  Branches    52109    52395     +286     
==========================================
+ Hits       246363   247502    +1139     
+ Misses      16979    16872     -107     
+ Partials     9536     9484      -52

Files with missing lines	Coverage Δ
src/api/callback.cc	`83.25% <100.00%> (+0.47%)`	⬆️
src/env.h	`97.33% <100.00%> (+0.07%)`	⬆️
src/inspector_agent.cc	`83.07% <100.00%> (+1.33%)`	⬆️
src/node.h	`91.66% <ø> (ø)`
src/node_internals.h	`80.35% <ø> (ø)`
src/env.cc	`82.12% <85.71%> (+<0.01%)`	⬆️

... and 87 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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>

joyeecheung · 2026-09-24T23:37:21Z

hmm, I am not quite sure if this is something we are ready to support, IIRC we have a some places that simply assume "on thread, one isolate, one Environment" so making "multiple Environments sharing the same isolate" means these code now have to use something more sophisticated than thread_locals to retrieve states that are really per-Environment. I think some mechanism needs to be provided to organize these better before we can officially call it supported?

codebytere · 2026-09-25T13:19:50Z

@joyeecheung fair - i went through the thread_locals in src/ and it looks like two of them hold per-Environment state:

the root cert store in crypto_context.cc: tls.setDefaultCACertificates() in one Environment changes it for the others
quic_alloc_state in quic/bindingdata.cc: binding points at the last BindingData created on the thread, so one Environment's QUIC session frees against another's counter

The rest are re-entrancy guards and debug counters that are fine per thread.

Would something like this cover it?

a typed slot for native state on Environment (e.g. env->GetOrCreateNativeState<T>()), destroyed in FreeEnvironment()
callbacks reach it through their user_data or Environment::GetCurrent(isolate) instead of a thread_local
a lint that rejects new thread_locals in src/ unless they're marked as intentionally per-thread

i'll drop the docs commit so this PR doesn't call the setup supported, and can open the slot as its own PR first?

With the FreeEnvironment() fix for sibling Environments and the handle cleanup depth tracked per thread, only the Environment being freed loses JavaScript while FreeEnvironment() runs the shared loop. Callbacks of the other Environments on that loop run their JavaScript as usual. Update embedding.md and the comment in node.h, which still describe JavaScript as disallowed on the whole isolate. Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>

legendecas · 2026-09-25T13:21:18Z

btw the PR title looks like this is a test only change.

nodejs-github-bot · 2026-09-25T13:25:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/77896/

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> PR-URL: #66239 Refs: #65977 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com> PR-URL: #66239 Refs: #65977 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

With the FreeEnvironment() fix for sibling Environments and the handle cleanup depth tracked per thread, only the Environment being freed loses JavaScript while FreeEnvironment() runs the shared loop. Callbacks of the other Environments on that loop run their JavaScript as usual. Update embedding.md and the comment in node.h, which still describe JavaScript as disallowed on the whole isolate. Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com> PR-URL: #66239 Refs: #65977 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

codebytere · 2026-09-30T09:32:22Z

Landed in 524eb24...8bf7793

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: #65977 Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com> PR-URL: #66239 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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> PR-URL: #66239 Refs: #65977 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>