src: split up InitializeContext #30067
Conversation
nodejs-github-bot
added
the
c++
|
InitializeContextRuntime is separate from InitializeContext on purpose. It should only be called at runtime, but InitializeContext might be called at snapshot time. |
|
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. |
|
@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. |
|
@devsnek We can't call Based on a quick code search (and knowing the history of I'm not sure I understand the snapshot reasoning when 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 |
|
@MarshallOfSound it should be called at runtime whenever a node context is created or loaded from a snapshot. |
|
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 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. |
|
@addaleax fwiw, the Atomics global isn't present at all during snapshot capture. |
|
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? |
|
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 From the perspective of Node.js internals I think it makes sense to rename the internal |
|
@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... |
|
@devsnek AFAICT |
|
@joyeecheung so you mean pull the existing code in |
|
@codebytere Yes |
codebytere
force-pushed
the
move-initializecontextruntime
branch
from
80cd88f
to
b0d43e5
Compare
|
Should be set! Thanks for the guidance :) |
codebytere
changed the title
codebytere
force-pushed
the
move-initializecontextruntime
branch
from
b0d43e5
to
5a8156f
Compare
|
Re-run of failing node-test-commit-windows-fanned (✔️) |
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
|
Landed in b086e38 |
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Refs electron/electron#20684.
Refs 1ec4154.
This PR splits out code from
InitializeContextinto a new functionInitializeContextForSnapshotand moves the callsite ofInitializeContextRuntimefromNewContexttoInitializeContext- embedders don't necessarily callNewContextand so we experienced a primordial-related crash:InitializeContextRuntimewasn't a public method, so we couldn't call it otherwise. To resolve this,any initialization logic should be performed in
InitializeContextto minimize surface area of potential breakage for embedders like Electron.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes