src: add public API for managing NodePlatform

[zcbenz]

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

[addaleax]

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?)

[zcbenz]

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?

[addaleax]

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

[zcbenz]

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

[bnoordhuis]

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

[zcbenz]

@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]

@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]

@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]

@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]

@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]

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]

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]

CI: https://ci.nodejs.org/job/node-test-commit/14143/

[bnoordhuis]

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]

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

@bnoordhuisnode::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]

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

[jasnell]

Landed in 2728112

[bnoordhuis]

@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]

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]

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

[gibfahn]

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

[MylesBorins]

@codebytere could you backport this to v8.x?