Skip to content
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

src: add public API for managing NodePlatform #16981

Closed
wants to merge 1 commit into from

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Nov 13, 2017

In embedders like Electron, we need to control when to use v8::Platform and when not, and to make Node work correctly we have to create NodePlatform manually sometimes. So this requires the ability to manage NodePlatform in embedders, while currently the interface of NodePlatform is not exported.

This pull request add public APIs for creating/destroying NodePlatform, which follows the pattern of the APIs managing node::Environment.

/cc @refack @bnoordhuis

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 13, 2017
@addaleax addaleax self-requested a review November 13, 2017 03:02
src/node.h Outdated
@@ -244,6 +250,12 @@ NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data,
NODE_EXTERN void LoadEnvironment(Environment* env);
NODE_EXTERN void FreeEnvironment(Environment* env);

NODE_EXTERN NodePlatform* CreatePlatform(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just use the MultiIsolatePlatform class declared right above, right? No need to explicitly reference NodePlatform in the API.

(Plus I think embedders want to call DrainBackgroundTasks() + CancelPendingDelayedTasks() at the end of an Isolate’s lifetime anyway?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize there is MultiIsolatePlatform, I was working on Node v8.7. Will do the change.

And do you think we should export MultiIsolatePlatform?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean, with NODE_EXTERN? I guess … didn’t really think of that. It should only be relevant to embedders, though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm it is impossible to export MultiIsolatePlatform anyway, the v8::Platform is not exported.

@addaleax addaleax added embedding Issues and PRs related to embedding Node.js in another project. semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 13, 2017
@bnoordhuis
Copy link
Member

@zcbenz Can you explain why/when exactly a NodePlatform is needed? Are there things you can't do with CreateIsolateData() + a custom MultiIsolatePlatform?

@zcbenz
Copy link
Contributor Author

zcbenz commented Nov 14, 2017

@bnoordhuis We need NodePlatform when we want to enable v8 inspector for the main process in Electron, it is not needed in renderer process where we have Chromium's devtools.

While it is possible to reimplement NodePlatform in Electron, it seems an unnecessary task when we can just reuse Node's code, and we don't want to have too much code relying on Node's C++ interface, it changes a lot and makes it harder to upgrade Node in Electron.

@bnoordhuis
Copy link
Member

@zcbenz I don't quite understand. NodePlatform isn't coupled to the V8 inspector. What does it do that V8's DefaultPlatform doesn't do and is difficult to write from scratch?

@zcbenz
Copy link
Contributor Author

zcbenz commented Nov 15, 2017

@bnoordhuis We use the inspector agent shipped with node, the default v8 platform used to work, but after upgrading to recent versions of node, we have to use NodePlatform to make things work with electron. I did not look into the root cause.

@bnoordhuis
Copy link
Member

@zcbenz Okay, I think I understand now. The behavior you see (or rather: don't see) probably isn't so much the introduction of NodePlatform but the fact that we stopped calling v8::platform::PumpMessageLoop(). We can probably fix that.

Do you call node::Start() or node::Init() or does electron control the event loop in the setup you describe?

@addaleax
Copy link
Member

@bnoordhuis Do you object to this landing? I think it’s a good idea regardless of whether it’s possible to do this or not; it certainly makes embedding Node easier.

@bnoordhuis
Copy link
Member

The PR was opened as a workaround for a bug. From that perspective: we should fix the bug, not merge the workaround.

Whether it makes embedding easier, I don't know. I assume most "serious" embedders will want more control over the v8::Platform so this PR would only be useful for simple embeddings.

No one requested that, AFAIK, and I don't like landing features that don't have users.

@addaleax
Copy link
Member

No one requested that, AFAIK, and I don't like landing features that don't have users.

Well, that is in part because our embedding story isn’t great to begin with, and issues like this are part of that … ;)

Basically what I’m thinking is, we’re going to support this anyway, so we might as well expose it to embedders, even if it’s just helping lower the barrier to get reasonable code running. And as an extra, using our platform will yield a bit more isolation from requirement changes in upstream V8.

(But, yes, we obviously should fix that bug.)

@addaleax
Copy link
Member

@bnoordhuis
Copy link
Member

even if it’s just helping lower the barrier to get reasonable code running

I don't disagree but can you think of a plausible scenario where rolling your own Platform is a step too far but v8::platform::CreateDefaultPlatform() doesn't cut it?

That's why I asked questions. I figured there must be more to it than ease of use.

@zcbenz
Copy link
Contributor Author

zcbenz commented Nov 20, 2017

Do you call node::Start() or node::Init() or does electron control the event loop in the setup you describe?

@bnoordhuis node::Init() is called in Electron, but we explicitly the ParseArgs() call, this is how Electron started the node inspector agent:
https://github.com/electron/electron/blob/master/atom/browser/node_debugger.cc

@joyeecheung
Copy link
Member

jasnell pushed a commit that referenced this pull request Nov 22, 2017
PR-URL: #16981
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member

jasnell commented Nov 22, 2017

Landed in 2728112

@jasnell jasnell closed this Nov 22, 2017
@bnoordhuis
Copy link
Member

@jasnell That's a little premature, wouldn't you say? This PR isn't an actual fix for the issue that electron is experiencing.

@jasnell
Copy link
Member

jasnell commented Nov 22, 2017

Possibly, definitely my apologies if so. I did not see any hard objections, it passed CI, it had sign off so I made an assumption. We can certainly revisit if necessary. :-)

@zcbenz
Copy link
Contributor Author

zcbenz commented Jan 17, 2018

@gibfahn Yeah it would be helpful, we did backport this patch ourselves for electron's node fork.

@gibfahn
Copy link
Member

gibfahn commented Jan 18, 2018

Okay, I'll take off the baking label, we'll look at including it in the next minor.

@gibfahn gibfahn removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Jan 18, 2018
@MylesBorins
Copy link
Contributor

@codebytere could you backport this to v8.x?

MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
PR-URL: #16981
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
PR-URL: #16981
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 9, 2018
PR-URL: #16981
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
MylesBorins pushed a commit that referenced this pull request Aug 16, 2018
PR-URL: #16981
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins added a commit that referenced this pull request Aug 17, 2018
Notable Changes:

* async_hooks:
  - rename PromiseWrap.parentId (Ali Ijaz Sheikh)
    #18633
  - remove runtime deprecation (Ali Ijaz Sheikh)
    #19517
  - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh)
    #18513
* cluster:
  - add cwd to cluster.settings (cjihrig)
    #18399
  - support windowsHide option for workers (Todd Wong)
    #17412
* crypto:
  - allow passing null as IV unless required (Tobias Nießen)
    #18644
* deps:
  - upgrade npm to 6.2.0 (Kat Marchán)
    #21592
  - upgrade libuv to 1.19.2 (cjihrig)
    #18918
  - Upgrade node-inspect to 1.11.5 (Jan Krems)
    #21055
* fs,net:
  - support as and as+ flags in stringToFlags() (Sarat Addepalli)
    #18801
  - emit 'ready' for fs streams and sockets (Sameer Srivastava)
    #19408
* http, http2:
  - add options to http.createServer() (Peter Marton)
    #15752
  - add 103 Early Hints status code (Yosuke Furukawa)
    #16644
  - add http fallback options to .createServer (Peter Marton)
    #15752
* n-api:
  - take n-api out of experimental (Michael Dawson)
    #19262
* perf_hooks:
  - add warning when too many entries in the timeline (James M Snell)
    #18087
* src:
  - add public API for managing NodePlatform (Cheng Zhao)
    #16981
  - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko)
    #17600
  - node internals' postmortem metadata (Matheus Marchini)
    #14901
* tls:
  - expose Finished messages in TLSSocket (Anton Salikhmetov)
    #19102
* **trace_events**:
  - add file pattern cli option (Andreas Madsen)
    #18480
* util:
  - implement util.getSystemErrorName() (Joyee Cheung)
    #18186

PR-URL: #21593
BethGriggs pushed a commit to BethGriggs/node that referenced this pull request Aug 29, 2018
Notable Changes:

* async_hooks:
  - rename PromiseWrap.parentId (Ali Ijaz Sheikh)
    nodejs#18633
  - remove runtime deprecation (Ali Ijaz Sheikh)
    nodejs#19517
  - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh)
    nodejs#18513
* cluster:
  - add cwd to cluster.settings (cjihrig)
    nodejs#18399
  - support windowsHide option for workers (Todd Wong)
    nodejs#17412
* crypto:
  - allow passing null as IV unless required (Tobias Nießen)
    nodejs#18644
* deps:
  - upgrade npm to 6.2.0 (Kat Marchán)
    nodejs#21592
  - upgrade libuv to 1.19.2 (cjihrig)
    nodejs#18918
  - Upgrade node-inspect to 1.11.5 (Jan Krems)
    nodejs#21055
* fs,net:
  - support as and as+ flags in stringToFlags() (Sarat Addepalli)
    nodejs#18801
  - emit 'ready' for fs streams and sockets (Sameer Srivastava)
    nodejs#19408
* http, http2:
  - add options to http.createServer() (Peter Marton)
    nodejs#15752
  - add 103 Early Hints status code (Yosuke Furukawa)
    nodejs#16644
  - add http fallback options to .createServer (Peter Marton)
    nodejs#15752
* n-api:
  - take n-api out of experimental (Michael Dawson)
    nodejs#19262
* perf_hooks:
  - add warning when too many entries in the timeline (James M Snell)
    nodejs#18087
* src:
  - add public API for managing NodePlatform (Cheng Zhao)
    nodejs#16981
  - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko)
    nodejs#17600
  - node internals' postmortem metadata (Matheus Marchini)
    nodejs#14901
* tls:
  - expose Finished messages in TLSSocket (Anton Salikhmetov)
    nodejs#19102
* **trace_events**:
  - add file pattern cli option (Andreas Madsen)
    nodejs#18480
* util:
  - implement util.getSystemErrorName() (Joyee Cheung)
    nodejs#18186

PR-URL: nodejs#21593
MylesBorins added a commit that referenced this pull request Sep 3, 2018
Notable Changes:

* async_hooks:
  - rename PromiseWrap.parentId (Ali Ijaz Sheikh)
    #18633
  - remove runtime deprecation (Ali Ijaz Sheikh)
    #19517
  - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh)
    #18513
* cluster:
  - add cwd to cluster.settings (cjihrig)
    #18399
  - support windowsHide option for workers (Todd Wong)
    #17412
* crypto:
  - allow passing null as IV unless required (Tobias Nießen)
    #18644
* deps:
  - upgrade npm to 6.4.1 (Kat Marchán)
    #22591
  - upgrade libuv to 1.19.2 (cjihrig)
    #18918
  - Upgrade node-inspect to 1.11.5 (Jan Krems)
    #21055
* fs,net:
  - support as and as+ flags in stringToFlags() (Sarat Addepalli)
    #18801
  - emit 'ready' for fs streams and sockets (Sameer Srivastava)
    #19408
* http, http2:
  - add options to http.createServer() (Peter Marton)
    #15752
  - add 103 Early Hints status code (Yosuke Furukawa)
    #16644
  - add http fallback options to .createServer (Peter Marton)
    #15752
* n-api:
  - take n-api out of experimental (Michael Dawson)
    #19262
* perf_hooks:
  - add warning when too many entries in the timeline (James M Snell)
    #18087
* src:
  - add public API for managing NodePlatform (Cheng Zhao)
    #16981
  - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko)
    #17600
  - node internals' postmortem metadata (Matheus Marchini)
    #14901
* tls:
  - expose Finished messages in TLSSocket (Anton Salikhmetov)
    #19102
* **trace_events**:
  - add file pattern cli option (Andreas Madsen)
    #18480
* util:
  - implement util.getSystemErrorName() (Joyee Cheung)
    #18186

PR-URL: #21593
@davisjam davisjam mentioned this pull request Sep 6, 2018
4 tasks
MylesBorins added a commit that referenced this pull request Sep 6, 2018
Notable Changes:

* async_hooks:
  - rename PromiseWrap.parentId (Ali Ijaz Sheikh)
    #18633
  - remove runtime deprecation (Ali Ijaz Sheikh)
    #19517
  - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh)
    #18513
* cluster:
  - add cwd to cluster.settings (cjihrig)
    #18399
  - support windowsHide option for workers (Todd Wong)
    #17412
* crypto:
  - allow passing null as IV unless required (Tobias Nießen)
    #18644
* deps:
  - upgrade npm to 6.2.0 (Kat Marchán)
    #21592
  - upgrade libuv to 1.19.2 (cjihrig)
    #18918
  - Upgrade node-inspect to 1.11.5 (Jan Krems)
    #21055
* fs,net:
  - support as and as+ flags in stringToFlags() (Sarat Addepalli)
    #18801
  - emit 'ready' for fs streams and sockets (Sameer Srivastava)
    #19408
* http, http2:
  - add options to http.createServer() (Peter Marton)
    #15752
  - add 103 Early Hints status code (Yosuke Furukawa)
    #16644
  - add http fallback options to .createServer (Peter Marton)
    #15752
* n-api:
  - take n-api out of experimental (Michael Dawson)
    #19262
* perf_hooks:
  - add warning when too many entries in the timeline (James M Snell)
    #18087
* src:
  - add public API for managing NodePlatform (Cheng Zhao)
    #16981
  - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko)
    #17600
  - node internals' postmortem metadata (Matheus Marchini)
    #14901
* tls:
  - expose Finished messages in TLSSocket (Anton Salikhmetov)
    #19102
* **trace_events**:
  - add file pattern cli option (Andreas Madsen)
    #18480
* util:
  - implement util.getSystemErrorName() (Joyee Cheung)
    #18186

PR-URL: #21593
MylesBorins added a commit that referenced this pull request Sep 10, 2018
Notable Changes:

* async_hooks:
  - rename PromiseWrap.parentId (Ali Ijaz Sheikh)
    #18633
  - remove runtime deprecation (Ali Ijaz Sheikh)
    #19517
  - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh)
    #18513
* cluster:
  - add cwd to cluster.settings (cjihrig)
    #18399
  - support windowsHide option for workers (Todd Wong)
    #17412
* crypto:
  - allow passing null as IV unless required (Tobias Nießen)
    #18644
* deps:
  - upgrade npm to 6.2.0 (Kat Marchán)
    #21592
  - upgrade libuv to 1.19.2 (cjihrig)
    #18918
  - Upgrade node-inspect to 1.11.5 (Jan Krems)
    #21055
* fs,net:
  - support as and as+ flags in stringToFlags() (Sarat Addepalli)
    #18801
  - emit 'ready' for fs streams and sockets (Sameer Srivastava)
    #19408
* http, http2:
  - add options to http.createServer() (Peter Marton)
    #15752
  - add 103 Early Hints status code (Yosuke Furukawa)
    #16644
  - add http fallback options to .createServer (Peter Marton)
    #15752
* n-api:
  - take n-api out of experimental (Michael Dawson)
    #19262
* perf_hooks:
  - add warning when too many entries in the timeline (James M Snell)
    #18087
* src:
  - add public API for managing NodePlatform (Cheng Zhao)
    #16981
  - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko)
    #17600
  - node internals' postmortem metadata (Matheus Marchini)
    #14901
* tls:
  - expose Finished messages in TLSSocket (Anton Salikhmetov)
    #19102
* **trace_events**:
  - add file pattern cli option (Andreas Madsen)
    #18480
* util:
  - implement util.getSystemErrorName() (Joyee Cheung)
    #18186

PR-URL: #21593
MylesBorins added a commit that referenced this pull request Sep 11, 2018
Notable Changes:

* async_hooks:
  - rename PromiseWrap.parentId (Ali Ijaz Sheikh)
    #18633
  - remove runtime deprecation (Ali Ijaz Sheikh)
    #19517
  - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh)
    #18513
* cluster:
  - add cwd to cluster.settings (cjihrig)
    #18399
  - support windowsHide option for workers (Todd Wong)
    #17412
* crypto:
  - allow passing null as IV unless required (Tobias Nießen)
    #18644
* deps:
  - upgrade npm to 6.2.0 (Kat Marchán)
    #21592
  - upgrade libuv to 1.19.2 (cjihrig)
    #18918
  - Upgrade node-inspect to 1.11.5 (Jan Krems)
    #21055
* fs,net:
  - support as and as+ flags in stringToFlags() (Sarat Addepalli)
    #18801
  - emit 'ready' for fs streams and sockets (Sameer Srivastava)
    #19408
* http, http2:
  - add options to http.createServer() (Peter Marton)
    #15752
  - add 103 Early Hints status code (Yosuke Furukawa)
    #16644
  - add http fallback options to .createServer (Peter Marton)
    #15752
* n-api:
  - take n-api out of experimental (Michael Dawson)
    #19262
* perf_hooks:
  - add warning when too many entries in the timeline (James M Snell)
    #18087
* src:
  - add public API for managing NodePlatform (Cheng Zhao)
    #16981
  - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko)
    #17600
  - node internals' postmortem metadata (Matheus Marchini)
    #14901
* tls:
  - expose Finished messages in TLSSocket (Anton Salikhmetov)
    #19102
* **trace_events**:
  - add file pattern cli option (Andreas Madsen)
    #18480
* util:
  - implement util.getSystemErrorName() (Joyee Cheung)
    #18186

PR-URL: #21593
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.