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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 37 commits into from
Closed

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

wants to merge 37 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 17, 2020 •
edited
Loading

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

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

@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 •
edited by addaleax
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/33158/ (:green_heart:)

@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
@addaleax
src: make FreeEnvironment() perform all necessary cleanup
ebdf84a 
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>
@addaleax
src: fix memory leak in CreateEnvironment when bootstrap fails
3f9fa28 
PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@addaleax
src: move worker_context from Environment to IsolateData
459973c 
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>
@addaleax
src: associate is_main_thread() with worker_context()
2c87b17 
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>
@addaleax
src: align worker and main thread code with embedder API
0e9f4a9 
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>
@addaleax
src: provide a variant of LoadEnvironment taking a callback
c694f08 
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>
@addaleax
src: add LoadEnvironment() variant taking a string
8a1255c 
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>
@addaleax
test: re-enable cctest that was commented out
bb689cf 
Refs: nodejs#31910

PR-URL: nodejs#30467
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@addaleax
src: add unique_ptr equivalent of CreatePlatform
ad95b85 
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>
@addaleax
src: make InitializeNodeWithArgs() official public API
33ecd3b 
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>
@addaleax
src: add ability to look up platform based on Environment*
31528f3 
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
@addaleax
src: align worker and main thread code with embedder API
808dedc 
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
@addaleax
src: provide a variant of LoadEnvironment taking a callback
c5aa3f4 
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
@addaleax
src: add LoadEnvironment() variant taking a string
4a6748d 
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
@addaleax
test: re-enable cctest that was commented out
f325c95 
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
@addaleax
src: add unique_ptr equivalent of CreatePlatform
8005e63 
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
@addaleax
src: make InitializeNodeWithArgs() official public API
a770a35 
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
@addaleax
src: add ability to look up platform based on Environment*
9b84ee6 
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
@addaleax
src: allow non-Node.js TracingControllers
2f8f767 
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
@addaleax
src: fix what a dispose without checking
a28c990 
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
@addaleax
src: shutdown platform from FreePlatform()
b8c9048 
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
@addaleax
src,test: add full-featured embedder API test
4af336d 
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
@addaleax
doc: add basic embedding example documentation
1a70820 
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
@addaleax
test: add extended embedder cctest
cb635c2 
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
@addaleax
test: wait for message from parent in embedding cctest
61eec0c 
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
@addaleax
embedding: provide hook for custom process.exit() behaviour
ff0a036 
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
@addaleax
embedding: make Stop() stop Workers
7a8f59f 
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
@addaleax
test: use InitializeNodeWithArgs in cctest
bdf6d41 
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
@addaleax
test: use common.buildType in embedding test
bd71cdf 
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
@addaleax
src: initialize inspector before RunBootstrapping()
1066341 
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
@addaleax
worker: unify custom error creation
18ecaeb 
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
@addaleax
src: make Environment::interrupt_data_ atomic
4513b6a 
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
@addaleax
src: fix cleanup hook removal for InspectorTimer
4704e58 
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
@addaleax
src: use env->RequestInterrupt() for inspector io thread start
2e4536e 
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
@addaleax
src: use env->RequestInterrupt() for inspector MainThreadInterface
29620c4 
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
@addaleax
src: flush V8 interrupts from Environment dtor
1474405 
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
@addaleax
src: add callback scope for native immediates
7e2cd72 
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
@cjihrig @addaleax
test,doc: add missing uv_setup_args() calls
26ede7f 
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
Reviewers
No reviews
Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@addaleax @nodejs-github-bot @richardlau @mhdawson @rvagg @cjihrig