src: allow optional Isolate termination in `node::Stop()` #46583

codebytere · 2023-02-09T09:01:32Z

This patch allows for node::Stop() to conditionally call v8:Isolate::TerminateExecution().

In several cases, we do not want to invoke a termination exception at exit when we're running with only_terminate_in_safe_scope set to false. Heap and coverage profilers run after environment exit and if there is a pending exception at this stage then they will fail to generate the appropriate profiles. This happens because V8's JSON logic, used by profilers here, expects that there are no pending exceptions. Node.js does not call node::Stop() on teardown and therefore does not have this issue when also running with only_terminate_in_safe_scope set to false (this is the default). Before this change, we were working around the problem by calling env->isolate()->CancelTerminateExecution(), but it would be preferable to be able to specify it here.

nodejs-github-bot · 2023-02-09T09:01:36Z

Review requested:

@nodejs/startup

addaleax · 2023-02-09T11:16:09Z

Before this change, we were working around the problem by calling env->isolate()->CancelTerminateExecution(), but it would be preferable to be able to specify it here.

That’s what I would have suggested for this use case before reading to the end of the PR description 🙂 But no strong feelings here, if this makes your life easier then I don’t see any issues with it :)

src/node.h

nodejs-github-bot · 2023-02-17T19:19:59Z

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

nodejs-github-bot · 2023-02-17T22:54:42Z

Landed in c566a04

PR-URL: #46583 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) #46673 * add ada as a dependency (Yagiz Nizipli) #46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 test_runner: * add initial code coverage support (Colin Ihrig) #46017 url: * replace url-parser with ada (Yagiz Nizipli) #46410 PR-URL: TODO

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) #46673 * add ada as a dependency (Yagiz Nizipli) #46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 test_runner: * add initial code coverage support (Colin Ihrig) #46017 url: * replace url-parser with ada (Yagiz Nizipli) #46410 PR-URL: #46725

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) nodejs#46673 * add ada as a dependency (Yagiz Nizipli) nodejs#46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) nodejs#46716 * add deokjinkim to collaborators (Deokjin Kim) nodejs#46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) nodejs#46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) nodejs#46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) nodejs#45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) nodejs#46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) nodejs#46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) nodejs#45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) nodejs#45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) nodejs#45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) nodejs#46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) nodejs#46273 test_runner: * add initial code coverage support (Colin Ihrig) nodejs#46017 url: * replace url-parser with ada (Yagiz Nizipli) nodejs#46410 PR-URL: nodejs#46725

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) #46673 * add ada as a dependency (Yagiz Nizipli) #46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 test_runner: * add initial code coverage support (Colin Ihrig) #46017 url: * replace url-parser with ada (Yagiz Nizipli) #46410 PR-URL: #46725

PR-URL: #46583 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) #46673 * add ada as a dependency (Yagiz Nizipli) #46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 test_runner: * add initial code coverage support (Colin Ihrig) #46017 url: * replace url-parser with ada (Yagiz Nizipli) #46410 PR-URL: #46725

PR-URL: #46583 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: TBD

Notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47502

Notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47502

Notable changes: Add initial support for single executable applications Compile a JavaScript file into a single executable application: ```console $ echo 'console.log(`Hello, ${process.argv[2]}!`);' > hello.js $ cp $(command -v node) hello $ npx postject hello NODE_JS_CODE hello.js \ --sentinel-fuse NODE_JS_FUSE_fce680ab2cc467b6e072b8b5df1996b2 $ npx postject hello NODE_JS_CODE hello.js \ --sentinel-fuse NODE_JS_FUSE_fce680ab2cc467b6e072b8b5df1996b2 \ --macho-segment-name NODE_JS $ ./hello world Hello, world! ``` Contributed by Darshan Sen in #45038 Replace url parser with Ada Node.js gets a new URL parser called Ada that is compliant with the WHATWG URL Specification and provides more than 100% performance improvement to the existing implementation. Contributed by Yagiz Nizipli in #46410 Other notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47502

nodejs/node#46583

* chore: bump node in DEPS to v18.16.0 * build,test: add proper support for IBM i nodejs/node#46739 * lib: enforce use of trailing commas nodejs/node#46881 * src: add initial support for single executable applications nodejs/node#45038 * lib: do not crash using workers with disabled shared array buffers nodejs/node#41023 * src: remove shadowed variable in OptionsParser::Parse nodejs/node#46672 * src: allow embedder control of code generation policy nodejs/node#46368 * src: allow optional Isolate termination in node::Stop() nodejs/node#46583 * lib: fix BroadcastChannel initialization location nodejs/node#46864 * chore: fixup patch indices * chore: sync filenames.json * fix: add simdutf dep to src/inspector BUILD.gn - nodejs/node#46471 - nodejs/node#46472 * deps: replace url parser with Ada nodejs/node#46410 * tls: support automatic DHE nodejs/node#46978 * fixup! src: add initial support for single executable applications * http: unify header treatment nodejs/node#46528 * fix: libc++ buffer overflow in string_view ctor nodejs/node#46410 * test: include strace openat test nodejs/node#46150 * fixup! fixup! src: add initial support for single executable applications --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <[email protected]>

codebytere requested review from addaleax and joyeecheung February 9, 2023 09:01

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Feb 9, 2023

codebytere changed the title ~~src: allow optional isolation termination in node::Stop()~~ src: allow optional Isolate termination in node::Stop() Feb 9, 2023

codebytere force-pushed the optional-isolate-termination branch from 5c253ea to 24c6777 Compare February 9, 2023 09:03

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 9, 2023

addaleax approved these changes Feb 9, 2023

View reviewed changes

src/node.h Outdated Show resolved Hide resolved

codebytere force-pushed the optional-isolate-termination branch 2 times, most recently from 2be2d9b to 7a99e13 Compare February 10, 2023 09:08

bnoordhuis reviewed Feb 10, 2023

View reviewed changes

src/node.h Outdated Show resolved Hide resolved

addaleax approved these changes Feb 10, 2023

View reviewed changes

src/node.h Outdated Show resolved Hide resolved

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 10, 2023

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 12, 2023

codebytere force-pushed the optional-isolate-termination branch 2 times, most recently from 72f0094 to f50bf1d Compare February 13, 2023 14:18

src: allow optional Isolate termination in node::Stop()

37d0e40

codebytere force-pushed the optional-isolate-termination branch from f50bf1d to 37d0e40 Compare February 13, 2023 14:18

addaleax approved these changes Feb 13, 2023

View reviewed changes

codebytere added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Feb 13, 2023

addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023

addaleax added commit-queue Add this label to land a pull request using GitHub Actions. embedding Issues and PRs related to embedding Node.js in another project. labels Feb 17, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 17, 2023

nodejs-github-bot merged commit c566a04 into nodejs:main Feb 17, 2023

github-actions bot mentioned this pull request Feb 18, 2023

CI Reliability 2023-02-18 nodejs/reliability#499

Open

32 tasks

MylesBorins pushed a commit that referenced this pull request Feb 18, 2023

src: allow optional Isolate termination in node::Stop()

c3897bf

PR-URL: #46583 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

MylesBorins mentioned this pull request Feb 19, 2023

v19.7.0 proposal #46725

Merged

github-actions bot mentioned this pull request Feb 19, 2023

CI Reliability 2023-02-19 nodejs/reliability#500

Open

21 tasks

MylesBorins pushed a commit that referenced this pull request Feb 20, 2023

src: allow optional Isolate termination in node::Stop()

f390841

PR-URL: #46583 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

danielleadams pushed a commit that referenced this pull request Apr 11, 2023

src: allow optional Isolate termination in node::Stop()

ea411d6

PR-URL: #46583 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

codebytere added a commit to electron/electron that referenced this pull request Apr 14, 2023

src: allow optional Isolate termination in node::Stop()

1705615

nodejs/node#46583

codebytere added a commit to electron/electron that referenced this pull request Apr 16, 2023

src: allow optional Isolate termination in node::Stop()

9a6cb3c

nodejs/node#46583

codebytere added a commit to electron/electron that referenced this pull request Apr 17, 2023

src: allow optional Isolate termination in node::Stop()

5671b5d

nodejs/node#46583

codebytere deleted the optional-isolate-termination branch February 8, 2024 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: allow optional Isolate termination in `node::Stop()` #46583

src: allow optional Isolate termination in `node::Stop()` #46583

codebytere commented Feb 9, 2023

nodejs-github-bot commented Feb 9, 2023

addaleax commented Feb 9, 2023

nodejs-github-bot commented Feb 17, 2023

nodejs-github-bot commented Feb 17, 2023

src: allow optional Isolate termination in node::Stop() #46583

src: allow optional Isolate termination in node::Stop() #46583

Conversation

codebytere commented Feb 9, 2023

nodejs-github-bot commented Feb 9, 2023

addaleax commented Feb 9, 2023

nodejs-github-bot commented Feb 17, 2023

nodejs-github-bot commented Feb 17, 2023

src: allow optional Isolate termination in `node::Stop()` #46583

src: allow optional Isolate termination in `node::Stop()` #46583