process: allow multiple uncaught exception capture calbacks #24279

misterdjules · 2018-11-09T19:14:10Z

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

This is a proof of concept for #23348. The goal is to enable the use case of having user-land implementations of the core domain module that are able to run concurrently with either the core domain module itself, or with other domain-like modules.

I wrote such a user-land implementation of domain at https://github.com/misterdjules/domaine to test this, and used that user-land implementation in a custom branch of Restify to test it is working as expected.

I will leave some comments inline to clarify some of the design/implementation decisions that might not be obvious at first sight.

I'm looking forward to reading your comments!

/cc @nodejs/domains @addaleax

nodejs-github-bot · 2018-11-09T19:14:13Z

@misterdjules build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1560/pipeline

misterdjules · 2018-11-09T19:19:41Z

doc/api/process.md

 <!-- YAML
 added: v9.3.0
 -->

+* `owner` {symbol}


owner is a symbol so that each domain-like implementation can use a unique key to register their own capture callbacks.

misterdjules · 2018-11-09T19:20:42Z

lib/domain.js

  } else {
-    setUncaughtExceptionCaptureCallback(null);
-    setUncaughtExceptionCaptureCallback((er) => {
+    setUncaughtExceptionCaptureCallback(CAPTURE_CB_KEY, null);


Setting this to null before setting it to an actual callback is actually not needed anymore, I'll update this shortly.

misterdjules · 2018-11-09T19:26:42Z

lib/internal/process/main_thread_only.js

@@ -171,6 +171,8 @@ function setupChildProcessIpcChannel() {
  }
 }

+process.domainsMap = new Map();


process.domainsMap is only needed to provide a way for applications to retrieve active domains, in the same way the currently active domain instance is available as process.domain. This might not necessarily be required though, and is here in this PR for now just to accommodate the REPL use case, which depends on being able to access any active domain in order to forward exceptions that are raised and caught by it to active domain instances..

devsnek · 2018-11-09T19:39:15Z

lib/internal/bootstrap/node.js

-      if (exceptionHandlerState.captureFn !== null) {
-        exceptionHandlerState.captureFn(er);
+      if (exceptionHandlerState.captureFns.size !== 0) {
+        for (const fn of exceptionHandlerState.captureFns.values()) {


the branch into multiple ownership here feels... off. these kinds of handlers should be at an application level, so the possibility of having like 30 functions all handling an uncaught exception seems a bit confusing

@devsnek Did you read the motivation for these changes in #23348? If so, what alternative would you suggest to achieve the goals presented there?

@misterdjules if we had some sort of stopPropagation behaviour, i think this would be doable.

maybe

for (const fn of exceptionHandlerState.captureFns.values()) { if (fn() !== false) { break; } // return `false` to signal you didn't handle it, and the next function should be used }

I agree with @devsnek – the purpose of these hooks is to (possibly?) take ownership for the errors, otherwise we’d basically end up with a list of uncaughtException listeners again

The purpose of calling all (possibly many) registered callbacks was to address the use case tested by https://github.com/misterdjules/domaine/blob/master/test/test-domaine-domain-compat.js.

Basically, if one module runs a function within a user-land implemented domain-like module and that function runs another function within another domain-like instance, when an error is thrown in that second function, the goal would be to have both domain-like instances to handle that same error. The rationale for this is that we'd want those different domain-like implementations to not step on each other by handling errors for each other (think for instance of the case where those domain-like instances are created by different dependencies of the same Node app).

However, this is an opinionated design decision and I think I'd be totally fine with only allowing the top-level active domain-like instance (d2 in the example mentioned above) to handle uncaught errors.

In that case though it seems we'd need to be able to keep a single stack of all different domain-like instances and that exposing an API to set the capture callback to user-land is not useful, as there would be a need for setting one of them internally in core. @devsnek @addaleax What are your thoughts on this?

addaleax · 2018-11-09T20:54:16Z

doc/api/errors.md

@@ -1720,15 +1703,6 @@ A `Transform` stream finished with data still in the write buffer.

 The initialization of a TTY failed due to a system error.

-<a id="ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET"></a>
-### ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET


We have a section for errors that were once present but are no longer below in this document :)

Thanks for the pointer 👍 I'll add those errors to the Legacy Node.js Error Codes section. Just to make sure: is it the section you were referring to?

Yes, exactly. :)

addaleax · 2018-11-09T20:56:14Z

lib/domain.js

  ERR_UNHANDLED_ERROR
 } = require('internal/errors').codes;
 const { createHook } = require('async_hooks');

+const CAPTURE_CB_KEY = Symbol('domain');


Can we do something similar to what https://github.com/nodejs/node/blob/master/doc/guides/using-symbols.md suggests? e.g.

Suggested change

const CAPTURE_CB_KEY = Symbol('domain');

const kCaptureKeyDomain = Symbol('kCaptureKeyDomain');

Having identical names for the variable and the symbol itself helps with grepping :)

Great suggestion, I'll do that 👍

addaleax · 2018-11-09T20:57:20Z

lib/internal/bootstrap/node.js

-      if (exceptionHandlerState.captureFn !== null) {
-        exceptionHandlerState.captureFn(er);
+      if (exceptionHandlerState.captureFns.size !== 0) {
+        for (const fn of exceptionHandlerState.captureFns.values()) {


I agree with @devsnek – the purpose of these hooks is to (possibly?) take ownership for the errors, otherwise we’d basically end up with a list of uncaughtException listeners again

addaleax · 2018-11-09T20:58:30Z

lib/internal/process/per_thread.js

-      throw new ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET();
+
+    if (typeof owner !== 'symbol') {
+      throw new ERR_INVALID_ARG_TYPE('owner', ['Symbol'], owner);


I guess this makes this semver-major?

why not use a Set and just add/remove the functions themselves

@devsnek's suggestion seems like a good alternative, I'll try that 👍

jasnell · 2018-11-10T18:07:10Z

doc/api/process.md

@@ -1807,11 +1807,12 @@ This function is only available on POSIX platforms (i.e. not Windows or
 Android).
 This feature is not available in [`Worker`][] threads.

-## process.setUncaughtExceptionCaptureCallback(fn)
+## process.setUncaughtExceptionCaptureCallback(owner, fn)


A bit concerned about backwards compat here. Is the new owner argument optional? What about existing code that may be calling setUncaughtExceptionCaptureCallback(fn)?

why not use a Set instead of a Map and use the functions themselves as keys

@devsnek's suggestion seems like a good alternative, I'll try that 👍

Trott · 2018-11-20T21:05:50Z

@misterdjules Still working on this? Should we add the work-in-progress label?

misterdjules · 2018-11-20T21:16:52Z

@Trott Yes! Next steps from my perspective:

I'd like to gather more feedback in the thread at process: allow multiple uncaught exception capture calbacks #24279 (comment) and reach consensus. I would appreciate more feedback from @addaleax if she has some time to weigh in.
I've also identified an issue with using different domain-like implementations where the EventEmitter methods get clobbered by the most recently loaded module. I plan to look into how that can be fixed and update this PR asap.

Adding the work-in-progress label sounds like a good idea 👍

misterdjules · 2019-02-26T21:13:23Z

I've also identified an issue with using different domain-like implementations where the EventEmitter methods get clobbered by the most recently loaded module. I plan to look into how that can be fixed and update this PR asap.

After looking into this, I chose to take a different approach in solving this use case with #26326. That PR is in my opinion a better solution than this one, so I'll close this one for now.

misterdjules requested review from addaleax, vdeturckheim and BridgeAR November 9, 2018 19:14

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 9, 2018

misterdjules mentioned this pull request Nov 9, 2018

Allow registering multiple uncaught exception capture callbacks #23348

Closed

misterdjules commented Nov 9, 2018

View reviewed changes

process: allow multiple uncaught exception capture calbacks

7082a27

devsnek reviewed Nov 9, 2018

View reviewed changes

misterdjules force-pushed the support-multiple-exception-capture-cb branch from a5af4ca to 7082a27 Compare November 9, 2018 19:46

addaleax reviewed Nov 9, 2018

View reviewed changes

jasnell reviewed Nov 10, 2018

View reviewed changes

misterdjules mentioned this pull request Nov 13, 2018

What will Domain be replaced with? #10843

Open

misterdjules added the wip Issues and PRs that are still a work in progress. label Nov 20, 2018

vdeturckheim approved these changes Dec 17, 2018

View reviewed changes

This was referenced Feb 20, 2019

domain: error handler runs outside of its domain #26211

Closed

domain: allow concurrent user-land impl #26326

Closed

misterdjules closed this Feb 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

process: allow multiple uncaught exception capture calbacks #24279

process: allow multiple uncaught exception capture calbacks #24279

misterdjules commented Nov 9, 2018

nodejs-github-bot commented Nov 9, 2018

misterdjules Nov 9, 2018

misterdjules Nov 9, 2018

misterdjules Nov 9, 2018

devsnek Nov 9, 2018 •

edited

Loading

misterdjules Nov 9, 2018

devsnek Nov 9, 2018 •

edited

Loading

addaleax Nov 9, 2018

misterdjules Nov 9, 2018

addaleax Nov 9, 2018

misterdjules Nov 13, 2018

addaleax Nov 13, 2018

addaleax Nov 9, 2018

misterdjules Nov 13, 2018

addaleax Nov 9, 2018

addaleax Nov 9, 2018

devsnek Nov 9, 2018

misterdjules Nov 13, 2018

jasnell Nov 10, 2018

devsnek Nov 10, 2018

misterdjules Nov 13, 2018

Trott commented Nov 20, 2018

misterdjules commented Nov 20, 2018

misterdjules commented Feb 26, 2019

	const CAPTURE_CB_KEY = Symbol('domain');
	const kCaptureKeyDomain = Symbol('kCaptureKeyDomain');

process: allow multiple uncaught exception capture calbacks #24279

process: allow multiple uncaught exception capture calbacks #24279

Conversation

misterdjules commented Nov 9, 2018

Checklist

nodejs-github-bot commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Nov 20, 2018

misterdjules commented Nov 20, 2018

misterdjules commented Feb 26, 2019

devsnek Nov 9, 2018 •

edited

Loading

devsnek Nov 9, 2018 •

edited

Loading