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

[v12.x backport] src: improve embedder API #35241

Closed
wants to merge 37 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 17, 2020

It’s been a while since these have landed on v14.x, so we should have had enough baking time to backport these:

There were no major conflicts to be resolved, mostly neighbouring-line conflicts. (Non-conflict changes that were necessary for backporting are in the fixup! commits.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. embedding Issues and PRs related to embedding Node.js in another project. v12.x labels Sep 17, 2020
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 17, 2020
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member Author

The CI failed on debian-8 because its GCC is too old, and that warning is printed on the command line. Is it intentional that we have GCC 4.9.2 in our CI here?

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 17, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 17, 2020

@richardlau
Copy link
Member

The CI failed on debian-8 because its GCC is too old, and that warning is printed on the command line. Is it intentional that we have GCC 4.9.2 in our CI here?

We dropped Debian 8 for Node.js 13 and above in nodejs/build#1974 based on the reasoning in nodejs/build#1970 (comment). Debian 8 went out of support in June 2020: https://www.debian.org/releases/jessie/ and we state in https://github.com/nodejs/node/blob/v12.x/BUILDING.md that we do not support running Node.js on End-of-Life platforms. I'd be open to dropping Debian 8 from testing on Node.js 12 -- maybe dropping it altogether?

cc @nodejs/build @nodejs/release

@mhdawson
Copy link
Member

looks like the minimum for 12.x is GCC 6.x https://github.com/nodejs/node/blob/v12.x/BUILDING.md#official-binary-platforms-and-toolchains, even the kernel level seems to say Debian 9 is the minimum https://github.com/nodejs/node/blob/v12.x/BUILDING.md#platform-list

Debian 8 also went EOL on June 30 2020 , so I think dropping it from 12.x testing should be ok? @rvagg any concern?

@rvagg
Copy link
Member

rvagg commented Sep 21, 2020

no, debian is always hard to maintain over time, no objections to removing it

@addaleax addaleax force-pushed the v12.x-staging branch 4 times, most recently from 55fe022 to 65b7bf4 Compare September 22, 2020 17:57
Make the calls `stop_sub_worker_contexts()`, `RunCleanup()`
part of the public API for easier embedding.

(Note that calling `RunAtExit()` is idempotent because the
at-exit callback queue is cleared after each call.)

PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Workers are fully in control of their Isolates, and this helps
avoid a problem with later changes to `CreateEnvironment()`
because now we can run the boostrapping code inside the latter.

PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
In our codebase, the assumption generally is that `!is_main_thread()`
means that the current Environment belongs to a Node.js Worker thread.

PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This addresses some long-standing TODOs by Joyee and me about
making the embedder API more powerful and us less reliant on
internal APIs for creating the main thread and Workers.

PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This allows embedders to flexibly control how they start JS code
rather than using `third_party_main`.

PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Allow passing a string as the main module rather than using
the callback variant.

PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Refs: nodejs#31910

PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This makes this bit of the embedder situation a bit easier to use.

PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This is a decent replacement for the to-be-deprecated Init() API.

PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
This should eventually remove any necessity to use the global-state
`GetMainThreadMultiIsolatePlatform()`.

PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This addresses some long-standing TODOs by Joyee and me about
making the embedder API more powerful and us less reliant on
internal APIs for creating the main thread and Workers.

Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This allows embedders to flexibly control how they start JS code
rather than using `third_party_main`.

Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
Allow passing a string as the main module rather than using
the callback variant.

Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
Refs: #31910

Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This makes this bit of the embedder situation a bit easier to use.

Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This is a decent replacement for the to-be-deprecated Init() API.

Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This should eventually remove any necessity to use the global-state
`GetMainThreadMultiIsolatePlatform()`.

Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
We do not need a Node.js-provided `v8::TracingController`, generally.
Loosen that restriction in order to make it easier for embedders
to provide their own subclass of `v8::TracingController`,
or none at all.

Refs: electron/electron@9c36576

Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
If created platform with CreatePlatform, the crash occurs because
it does not check if it was initialized to v8_platform
when DisposePlatform was called.

Backport-PR-URL: #35241
Refs: #31260
Co-authored-by: Anna Henningsen <anna@addaleax.net>
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
There is currently no way to properly do this.

Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
Add an embedder cctest that also covers a multi-Environment situation,
including worker_threads-style inspector support.

Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>

Backport-PR-URL: #35241
PR-URL: #30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
I’ve seen this fail a few times in CI, presumably because the
inspector commmand did not reach the child thread in time.
Explicitly waiting seems to solve that.

Refs: #30467

Backport-PR-URL: #35241
PR-URL: #32563
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
Embedders may not want to terminate the process when `process.exit()`
is called. This provides a hook for more flexible handling of that
situation.

Refs: #30467 (comment)

Backport-PR-URL: #35241
PR-URL: #32531
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This makes sense given that terminating execution of the parent thread
this way likely also is supposed to stop all running Worker threads
spawned by it.

Backport-PR-URL: #35241
PR-URL: #32531
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
Refs: d7f1107
Fixes: #30257

Backport-PR-URL: #35241
PR-URL: #32406
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This un-breaks testing in the case of `./configure --debug-node`.

Backport-PR-URL: #35241
PR-URL: #32422
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This is necessary for `--inspect-brk-node` to work, and for the
inspector to be aware of scripts created before bootstrapping.

Fixes: #32648
Refs: #30467 (comment)

Backport-PR-URL: #35241
PR-URL: #32672
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
Mostly, this introduces a pattern that makes sure that if a custom
error is reported, `stopped_` will be set to `true` correctly in
every cast, which was previously missing for the
`NewContext().IsEmpty()` case (which led to a hard crash from the
`Worker` destructor).

This also leaves TODO comments for a few cases in which
`ERR_WORKER_OUT_OF_MEMORY` was not used in accordance with the
documentation for that error code (or according to its intention).
Fixing that is semver-major.

Backport-PR-URL: #35241
PR-URL: #33084
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
Otherwise this potentially leads to race conditions when used in a
multi-threaded environment (i.e. when used for its very purpose).

Backport-PR-URL: #35241
PR-URL: #32523
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
Fix this to account for the fact that `Stop()` may already have been
called from a cleanup hook when the `inspector::Agent` is deleted
along with the `Environment`, at which point cleanup hooks are no
longer available.

Backport-PR-URL: #35241
PR-URL: #32523
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This cleans up `Agent::RequestIoThreadStart()` significantly.

Backport-PR-URL: #35241
PR-URL: #32523
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This simplifies the code significantly, and removes the dependency of
the inspector code on the availability of a `MultiIsolatePlatform`
(by removing the dependency on a platform altogether).

It also fixes a memory leak that occurs when `RequestInterrupt()`
is used, but the interrupt handler is never called before the Isolate
is destroyed.

One downside is that this leads to a slight change in timing, because
inspector messages are not dispatched in a re-entrant way. This means
having to harden one test to account for that possibility by making
sure that the stack is always clear through a `setImmediate()`.
This does not affect the assertion made by the test, which is that
messages will not be delivered synchronously while other code is
executing.

#32415

Backport-PR-URL: #35241
PR-URL: #32523
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This avoids an edge-case memory leak.

Refs: #32523 (comment)

Backport-PR-URL: #35241
PR-URL: #32523
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This ensures that microtasks scheduled by native immediates are run
after the tasks are done. In particular, this affects the inspector
integration since 6f9f546.

Fixes: #33002
Refs: #32523

Backport-PR-URL: #35241
PR-URL: #34366
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 23, 2020
libuv 1.39.0 will begin requiring uv_setup_args() to be called
before attempting to access the process title. This commit adds
uv_setup_args() calls that were missing in order for the test
suite to pass (and updates the documentation).

Backport-PR-URL: #35241
PR-URL: #34751
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@addaleax
Copy link
Member Author

Landed in 1d3638b...26ede7f

@addaleax addaleax closed this Sep 23, 2020
@addaleax addaleax deleted the backport-v12.x-30467 branch September 23, 2020 18:29
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. lib / src Issues and PRs related to general changes in the lib or src directory. 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.

6 participants