src: various improvements toward a complete embedder API #30229

addaleax · 2019-11-02T20:25:41Z

These changes should help make embedding easier, by
requiring fewer internal methods to build a fully functioning
Node.js instance.

~~The first commit is #30228. (Marking this as blocked until that lands.)~~

src: track no of active JS signal handlers

This makes it possible to tell whether a signal is being tracked in JS.

src: make EndStartedProfilers an exit hook

Run EndStartedProfilers on Environment teardown.

src: make WaitForInspectorDisconnect an exit hook

Run inspector cleanup code on Environment teardown.

src: use unique_ptr for InitializeInspector()

This makes more sense than releasing and re-wrapping the raw pointer.

src: run RunBeforeExitCallbacks as part of EmitBeforeExit

This also aligns the worker_threads code with the main thread code.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

This makes it possible to tell whether a signal is being tracked in JS.

Run `EndStartedProfilers` on Environment teardown. This is part of a series of changes to make embedding easier, by requiring fewer internal methods to build a fully functioning Node.js instance.

Run inspector cleanup code on Environment teardown. This is part of a series of changes to make embedding easier, by requiring fewer internal methods to build a fully functioning Node.js instance.

This makes more sense than releasing and re-wrapping the raw pointer.

This is part of a series of changes to make embedding easier, by requiring fewer internal methods to build a fully functioning Node.js instance. This also aligns the worker_threads code with the main thread code.

nodejs-github-bot · 2019-11-05T19:58:19Z

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

src/signal_wrap.cc

src/node_internals.h

nodejs-github-bot · 2019-11-06T14:58:59Z

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

joyeecheung

Much better to do this in the exit hooks than to do this in ad-hoc places during the shutdown path!

joyeecheung · 2019-11-06T16:32:02Z

src/inspector_profiler.cc

+      }
+    }
+    if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
+      PrintCaughtException(isolate, context, try_catch);


Not really related to this PR but now come to think of it we should probably avoid executing JS at all during profile serialization considering we also write the profiles when there is an uncaught exception...

@joyeecheung Yeah … I had the same thought, and I talked about it with @bcoe – it turns out re-implementing the JS code in C++ expands the code quite noticeably, and moving the source map caches to C++ is also tricky, in part because the CJS cache relies on Module._cache and we probably don’t want to move that to C++… if you have a better solution for this, feel free to PR that 😅

joyeecheung · 2019-11-06T16:35:18Z

src/inspector_agent.cc

@@ -778,6 +778,13 @@ bool Agent::Start(const std::string& path,
    StartDebugSignalHandler();
  }

+  AtExit(parent_env_, [](void* env) {


IIUC this is the first time we use AtExit internally? (I vaguely remember we have done that before but I could not find any existing AtExit calls ATM)

Yes, I think so 👍

codebytere

also looks good to me generally!

nodejs-github-bot · 2019-11-06T17:17:06Z

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

nodejs-github-bot · 2019-11-06T18:30:06Z

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

nodejs-github-bot · 2019-11-06T21:21:49Z

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

This makes it possible to tell whether a signal is being tracked in JS. PR-URL: #30229 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>

Run `EndStartedProfilers` on Environment teardown. This is part of a series of changes to make embedding easier, by requiring fewer internal methods to build a fully functioning Node.js instance. PR-URL: #30229 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>

Run inspector cleanup code on Environment teardown. This is part of a series of changes to make embedding easier, by requiring fewer internal methods to build a fully functioning Node.js instance. PR-URL: #30229 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>

This makes more sense than releasing and re-wrapping the raw pointer. PR-URL: #30229 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>

This is part of a series of changes to make embedding easier, by requiring fewer internal methods to build a fully functioning Node.js instance. This also aligns the worker_threads code with the main thread code. PR-URL: #30229 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Shelley Vohr <[email protected]>

addaleax · 2019-11-06T22:51:55Z

Landed in 4aca277...d4b2cc7, thanks for the reviews!