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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
vm: add experimental NodeRealm implementation #47855
vm: add experimental NodeRealm implementation #47855
Changes from 5 commits
5aebfda
a624c7e
b04d52c
156fad5
8ff9d0a
1050cb2
1b5978b
4211750
8929ecd
7f56539
db8bf72
62e534b
bb0a04a
75618e6
1b8059f
6184855
0ffaba0
aa46778
c3f4321
39e58b3
8474e51
8240d61
f371400
9f1d1a5
57409e6
db0c89b
3988738
db395ac
7ba9bbd
4209518
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think the docs should clarify the difference between this and a
ShadowRealm
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also like to understand the differences (and similarities) between this and a worker. Because they look very similar. For example, does a realm have an event loop? Does it share globals? (I'm assuming yes and no?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we create the NodeRealm with a specifier?
In this way, we can get rid of the higher order function
createImport
and makes the class method more aligned withShadowRealm.prototype.importValue
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be clarified whether this value is mutable. e.g. is it possible to
localworker.globalThis.foo = 1
and have that value reflected within the local worker.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should NodeRealm inherit from BaseObject? This seems pretty similar to BaseObject's work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Only reason I didn't do this in synchronous-worker is because it was built outside of Node.js core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be a good idea - I think so far we've been assuming in the code base that there is a 1:1 relationship between the environment and the isolate, things can be subtly broken once we have n:1. It would be cleaner if we follow the Realm approach and just split out states in the Environment that are unique to each context/realm to a subclass of
node::Realm
(or maybe it is already justnode::PrincipalRealm
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite a few of the crashes I've been fighting with this approach are due to that. I'm using the reference to
env
to clean up all the handles. Otherwise, we will have bad crashes afterstop()
is called (which is why I'm doing this in core). Using anode::PrincipalReam
would require removing the deliberatestop()
method: how would we shut it down?The other question I have if we dith the
CreateEnvironment
call, how do we guarantee some level of isolation?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just move the handles into the Realm instead. Basically, just move things that should belong to individual realms instead of being shared across them to the realm, and do the setup/cleanup on a per-realm basis, which is what we've been trying to do with the ShadowRealm integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you do that?
As an example
HandleWrap
add things to theEnvironment
here:node/src/handle_wrap.cc
Line 110 in c542d3a
Should I try to move that list from the Env to the Realm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just make that
realm()->handle_wrap_queue()->PushBack(this)
. That's what we've been doing to support other BaseObjects in ShadowRealms, we just haven't got toHandleWrap
yet. If our intent is to make individual realms have their own sets of handles etc., we'd have to move them into realms and stop mixing them in one giantEnvironment
that contains per-thread information anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, tracking handle wraps and request wraps by realms is the plan for shadow realm integration. It's not the current focus yet.
However, moving the list from the Environment to the Realm breaks the postmortem diagnostics since tools like llnode are built on top of the
Environment::handle_wrap_queue
structure: https://github.com/nodejs/node/blob/main/src/node_postmortem_metadata.cc#L23An option can be tracking the handle wraps and request wraps by both the Env and its creation Realm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to worry much about the postmortem diagnostics data, we only need to leave the offset of the queues within Realms and the tools would then figure out how to adapt to newer versions of Node.js. It's never guaranteed that we'd never change the layout of our internals, only that when we do, we still leave some information in the binary (the offsets) for these tools to figure out how to extract information from a core dump / process memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that we won't be able to have a
process
object without a new Node.js Environment, right?May we land this PR and work to remove
CreateEnvironment
in follow-on work?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That that's not the case is precisely the difference between
Environment
andIsolateData
, though. Just because the Node.js CLI doesn't create multipleEnvironment
instances perIsolate
doesn't mean that it's not semantically correct to do so.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think in practice we have really been writing the code that way. There are some places where we configure V8 isolate hooks (e.g. the ones in
Environment::InitializeDiagnostics
) with the current Environment as data to be used in the callback. The async hooks where things likeEnvironment::GetCurrent(isolate)->trigger_async_id()
is used a lot don't look particularly robust against a multi-Environment architecture either, and there are probably more places than I can think of off the top of my head..