-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Conversation
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? |
CI: https://ci.nodejs.org/job/node-test-pull-request/33158/ (:green_heart:) |
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 |
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? |
no, debian is always hard to maintain over time, no objections to removing it |
55fe022
to
65b7bf4
Compare
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Landed in 1d3638b...26ede7f |
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), orvcbuild test
(Windows) passes