src: split up InitializeContext - #30067
Conversation
Sorry, something went wrong.
|
InitializeContextRuntime is separate from InitializeContext on purpose. It should only be called at runtime, but InitializeContext might be called at snapshot time. |
Sorry, something went wrong.
|
Then would you prefer i expose this as a new embedder API? Either way, we'll need to find a reusable solution for embedders here I would say. |
Sorry, something went wrong.
|
@codebytere i'm not really sure what the constraints here are. If electron can't call NewContext for some reason it should duplicate the logic I guess. |
Sorry, something went wrong.
|
@devsnek We can't call NewContext because we are not responsible for creating the context, we are given an existing context and we want to give that to node. If there is logic that node must run on a context to make it work, that logic must either be run on the exposed APIs that allow embedders to initialize a context or a new API must be exposed and a breaking change declared for embedders. Based on a quick code search (and knowing the history of InitializeContext) that method is called in exactly one place. The exact same place that InitializeContextRuntime is called. The initialize method was exposed in #28544 for the sole use case of embedders initializing an already created context with node's special logic. I'm not sure I understand the snapshot reasoning when InitializeContextRuntime is called whenever InitializeContext is called. Duplicating the logic doesn't make much sense either as that is again a breaking change for embedders and it means that all embedders that use the Initialize API suddenly need to start duplicating logic (some of which may use node internals that can't be called without further duplication). |
Sorry, something went wrong.
|
@MarshallOfSound it should be called at runtime whenever a node context is created or loaded from a snapshot. |
Sorry, something went wrong.
|
Maybe for context – I have wondered the same thing as you, @MarshallOfSound; the reason for this separation is, as I understand the explanation that @devsnek gave me, that snapshot dons’t actually reflect removals from e.g. builtin objects, like the one we perform with Atomics.wake. I feel like a comment in the code about that would be a good idea. And if necessary, exposing this as an embedder method seems fine to me. |
Sorry, something went wrong.
|
@addaleax fwiw, the Atomics global isn't present at all during snapshot capture. |
Sorry, something went wrong.
|
Sorry if I'm missing something, but this separation still makes no sense. From nodes perspective, there is 0 difference from before this PR and after. The same code runs, in the same order. From Electron (Embedders) perspective, what was broken, now works again 🤷♂ What am I missing? This PR fixes embedders, and has no impact on node? |
Sorry, something went wrong.
|
From the perspective of an embedder I don't think any new API needs to be introduced for now - we don't provide any proper hooks for embedders to do anything about the snapshots and that need to be well thought-out than just an ad-hoc InitializeContextRuntime. Having just InitializeContext which runs through an existing context to do everything is sufficient and backwards compatible. From the perspective of Node.js internals I think it makes sense to rename the internal InitializeContext to something like InitializeContextForSnapshot, and then call both InitializeContextForSnapshot and InitializeContextRuntime in the public InitializeContext. So then NewContext is essentially Context::New + InitializeContext, and embedders can choose to just call InitializeContext on an existing context as usual. |
Sorry, something went wrong.
|
@joyeecheung i think that sounds okay. the only thing i want to make sure of is that InitializeContextRuntime isn't called twice on a context, not because i think it will break anything at the moment, but it could cause trouble in the future... |
Sorry, something went wrong.
|
@devsnek AFAICT InitializeContextRuntime is just a temporary ad-hoc internal helper which contains two tasks that are also temporary ad-hoc fixes (it's hard to tell who may disappear first, the helper or the fixes) so I would not worry too much about future footguns. |
Sorry, something went wrong.
|
@joyeecheung so you mean pull the existing code in InitializeContext into a new method InitializeContextForSnapshot and then have the public one call into that as well as the existing InitializeContextRuntime? |
Sorry, something went wrong.
|
Should be set! Thanks for the guidance :) |
Sorry, something went wrong.
Sorry, something went wrong.
|
Re-run of failing node-test-commit-windows-fanned (✔️) |
Sorry, something went wrong.
This splits out code from InitializeContext into a new function InitializeContextForSnapshot and moves the callsite of InitializeContextRuntime from NewContext to InitializeContext - embedders don't necessarily call NewContext and so need to be able to guarantee these functions are called regardless. PR-URL: #30067 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This splits out code from InitializeContext into a new function InitializeContextForSnapshot and moves the callsite of InitializeContextRuntime from NewContext to InitializeContext - embedders don't necessarily call NewContext and so need to be able to guarantee these functions are called regardless. PR-URL: #30067 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This splits out code from InitializeContext into a new function InitializeContextForSnapshot and moves the callsite of InitializeContextRuntime from NewContext to InitializeContext - embedders don't necessarily call NewContext and so need to be able to guarantee these functions are called regardless. PR-URL: #30067 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This splits out code from InitializeContext into a new function InitializeContextForSnapshot and moves the callsite of InitializeContextRuntime from NewContext to InitializeContext - embedders don't necessarily call NewContext and so need to be able to guarantee these functions are called regardless. PR-URL: #30067 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This splits out code from InitializeContext into a new function InitializeContextForSnapshot and moves the callsite of InitializeContextRuntime from NewContext to InitializeContext - embedders don't necessarily call NewContext and so need to be able to guarantee these functions are called regardless. PR-URL: #30067 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This splits out code from InitializeContext into a new function InitializeContextForSnapshot and moves the callsite of InitializeContextRuntime from NewContext to InitializeContext - embedders don't necessarily call NewContext and so need to be able to guarantee these functions are called regardless. PR-URL: #30067 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Refs electron/electron#20684.
Refs 1ec4154.
This PR splits out code from InitializeContext into a new function InitializeContextForSnapshot and moves the callsite of InitializeContextRuntime from NewContext to InitializeContext - embedders don't necessarily call NewContext and so we experienced a primordial-related crash:
DetailsInitializeContextRuntime wasn't a public method, so we couldn't call it otherwise. To resolve this,
any initialization logic should be performed in InitializeContext to minimize surface area of potential breakage for embedders like Electron.
Checklist