domain: allow concurrent user-land impl #26326

misterdjules · 2019-02-26T20:16:34Z

Currently, only one domain-like implementation (the core domain one) can be used to handle uncaught exceptions or unhandled error events.

This PR aims at making it possible for different domain-like user-land implementations to be used concurrently (including with the core domain impl) so that the state of the core domain module (doc deprecated) does not prevent users of domains from having a well-maintained domain-like facility.

Ref: #23348

This is the second iteration of #24279, which uses a different approach to allow user-land implementations of a domain-like facility:

it allows to register only one uncaught exception callback (but it still allows the uncaught exception callback to be set by any caller even after the core domain module set it).
it moves the domains stack from being owned by the domain module to being shared by all domain-like implementations
it moves the handling of domains back into lib/events, as otherwise making it so that event emitter instances created within the context of different domain-like implementation have the proper implementation of EventEmitter methods without clobbering other domain modules' implementation of those methods seemed a bit convoluted. I think ideally we would allow each domain implementation to have their own EventEmitter overrides that don't conflict with each other, and if that's the only point of contention for this PR we can definitely address it in subsequent iterations/commits in this PR.

These changes have been tested by implementing a user-land domain-like module and using it as a replacement for domain in restify.

If that approach seems reasonable, there is probably a number of new tests that we can add to the tests suite, but I'd like to get some feedback before adding those.

/cc @nodejs/domains

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

nodejs-github-bot · 2019-02-26T20:16:36Z

@misterdjules sadly an error occured when I tried to trigger a build :(

devsnek · 2019-02-26T20:32:42Z

lib/events.js

@@ -32,7 +32,9 @@ module.exports = EventEmitter;
 EventEmitter.EventEmitter = EventEmitter;

 EventEmitter.usingDomains = false;
+EventEmitter.domainSym = Symbol('isDomainLike');


symbol descriptions should match how they are accessed. Symbol('events.domainSym')

Sounds good, will fix!

I’d prefer EventEmitter.isDomainLike in that case :) Also, should this be documented, since the idea is making it available to other implementations (it is, right)?

I’d prefer EventEmitter.isDomainLike in that case :)

Sounds good, given there might be a lot of different opinions on the naming of this property, let's leave that to the final stages of this review :)

Also, should this be documented, since the idea is making it available to other implementations (it is, right)?

Yes, the idea is to make it available to other implementations. You can take a look at how it's used in the proof-of-concept that I wrote to test this.

devsnek · 2019-02-26T20:34:58Z

lib/domain.js

+
+// It's possible to enter one domain while already inside another one. The stack
+// is each entered domain.
+const stack = exports._stack = process._domainsStack;


can you revert the change to this line?

What do you mean? How would stack and exports._stack be initialized?

The general goal is to avoid doing x = y = z

this would work:

const stack = process._domainStack; exports._stack = stack; // no change to this line

or better yet, remove the const stack = entirely and just refer to process._domainStack.

Sounds good, will do!

lib/internal/bootstrap/node.js

vdeturckheim

LGTM!

Currently, only one domain-lke implementation (the core domain one) can be used to handle uncaught exceptions or unhandled error events. This PR aims at making it possible for different domain-like user-land implementations to be used concurrently (including with the core domain impl) so that the state of the core domain module (doc deprecated) does not prevent users of domains from having a well-maintained domain-like facility. Ref: nodejs#23348

misterdjules · 2019-02-27T23:44:52Z

@addaleax I would love to read your thoughts on this as I think you had some objections to the previous approach that I used to allow user-land domain implementations.

addaleax · 2019-02-28T21:48:33Z

doc/api/errors.md

-
-The stack trace is extended to include the point in time at which the
-`domain` module had been loaded.
-


We have a special section for error codes that were removed, could you move these there (and add a removed: REPLACEME YAML tag)?

Yes 👍 I keep forgetting about that, my apologies.

addaleax · 2019-02-28T21:51:59Z

lib/events.js

@@ -32,7 +32,9 @@ module.exports = EventEmitter;
 EventEmitter.EventEmitter = EventEmitter;

 EventEmitter.usingDomains = false;
+EventEmitter.domainSym = Symbol('isDomainLike');


I’d prefer EventEmitter.isDomainLike in that case :) Also, should this be documented, since the idea is making it available to other implementations (it is, right)?

addaleax · 2019-02-28T21:53:50Z

Benchmark CI for emit(): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/297/

BridgeAR · 2019-03-01T00:22:11Z

                                                 confidence improvement accuracy (*)   (**)  (***)
events/ee-emit.js listeners=10 argc=0 n=2000000         ***    -13.80 %       ±1.78% ±2.39% ±3.13%
events/ee-emit.js listeners=10 argc=10 n=2000000        ***    -32.66 %       ±0.92% ±1.24% ±1.62%
events/ee-emit.js listeners=10 argc=2 n=2000000         ***    -23.67 %       ±1.03% ±1.38% ±1.80%
events/ee-emit.js listeners=10 argc=4 n=2000000         ***    -26.41 %       ±0.82% ±1.09% ±1.43%
events/ee-emit.js listeners=1 argc=0 n=2000000          ***    -23.44 %       ±4.26% ±5.72% ±7.56%
events/ee-emit.js listeners=1 argc=10 n=2000000         ***    -29.89 %       ±2.34% ±3.12% ±4.09%
events/ee-emit.js listeners=1 argc=2 n=2000000          ***    -25.91 %       ±2.69% ±3.57% ±4.65%
events/ee-emit.js listeners=1 argc=4 n=2000000          ***    -27.00 %       ±2.31% ±3.08% ±4.01%
events/ee-emit.js listeners=5 argc=0 n=2000000          ***    -18.14 %       ±3.21% ±4.29% ±5.60%
events/ee-emit.js listeners=5 argc=10 n=2000000         ***    -27.51 %       ±1.23% ±1.64% ±2.15%
events/ee-emit.js listeners=5 argc=2 n=2000000          ***    -20.26 %       ±1.34% ±1.79% ±2.34%
events/ee-emit.js listeners=5 argc=4 n=2000000          ***    -21.42 %       ±1.47% ±1.96% ±2.56%

Events seem to take quite a hit due to the changes.

misterdjules · 2019-03-01T01:37:31Z

Events seem to take quite a hit due to the changes.

Oh, that seems pretty bad indeed. Let me take a look within the next few days and see whether we can avoid that performance penalty.

BridgeAR · 2019-03-27T16:55:00Z

Ping @misterdjules

misterdjules · 2019-03-27T18:10:06Z

@BridgeAR Thanks for the ping! This is still on my radar, but I've had other work priorities conflict with this. Not sure when I'll be able to allocate any time to iterate on the current implementation, hopefully sooner than later.

BridgeAR · 2019-12-20T01:10:26Z

@misterdjules is this still something you would like to work on?

misterdjules · 2019-12-20T03:54:44Z

@BridgeAR Thanks for the ping!

I'd still like something similar (allowing full-featured user land domain implementations to coexist alongside the core domain one) to happen, regardless of whether this is done using the approach in this PR or using a different approach. I also don't mind if someone else finishes what remains to be done.

I don't know if I'll have more time to dedicate to this soon.

Are you asking to determine whether this can be closed? Maybe tagging this with the stalled label would be appropriate here?

Trott · 2020-04-22T13:56:56Z

I'm in the process of rebasing this, and want to leave some notes:

Need to document isDomainLike symbol for end users
Need to include use of isDomainLike symbol by end users in a test
Proof-of-concept code for .isDomainLike by @misterdjules is at https://github.com/misterdjules/domaine/blob/a6b928c906245dd7a1d93b581c6286b298dfdc92/index.js#L178

Trott · 2020-04-22T15:02:33Z

Successfully rebased but can't force push to @misterdjules's branch...

misterdjules · 2020-05-04T21:07:33Z

@Trott Are you still blocked by not having force push access to my branch or are you fine with continuing your work at #33013? Regardless, thank you so much for picking this up 🙏

Trott · 2020-05-04T23:58:46Z

@Trott Are you still blocked by not having force push access to my branch or are you fine with continuing your work at #33013? Regardless, thank you so much for picking this up 🙏

I'm fine picking it up in #33013 although even better if someone else does, because I don't know when I'm going to get around to looking at those performance regressions.

I think this one can be closed, unless you plan on picking it back up (in which case I'll close #33013).

misterdjules · 2020-05-07T15:57:15Z

@Trott I do not plan to pick this up.

misterdjules requested review from addaleax, targos and vdeturckheim February 26, 2019 20:16

nodejs-github-bot added domain Issues and PRs related to the domain subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. events Issues and PRs related to the events subsystem / EventEmitter. labels Feb 26, 2019

devsnek reviewed Feb 26, 2019

View reviewed changes

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

misterdjules mentioned this pull request Feb 26, 2019

process: allow multiple uncaught exception capture calbacks #24279

Closed

4 tasks

vdeturckheim approved these changes Feb 27, 2019

View reviewed changes

misterdjules force-pushed the support-userland-domains-bis branch from b18991b to 8a4a4c6 Compare February 27, 2019 22:49

addaleax reviewed Feb 28, 2019

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

devnexen force-pushed the master branch from e8a4568 to 5289f80 Compare December 26, 2019 19:46

Trott mentioned this pull request Apr 23, 2020

domain: allow concurrent user-land impl #33013

Closed

4 tasks

Trott closed this May 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

domain: allow concurrent user-land impl #26326

domain: allow concurrent user-land impl #26326

misterdjules commented Feb 26, 2019

nodejs-github-bot commented Feb 26, 2019

devsnek Feb 26, 2019 •

edited

Loading

misterdjules Feb 26, 2019

addaleax Feb 28, 2019

misterdjules Mar 1, 2019

devsnek Feb 26, 2019

misterdjules Feb 26, 2019

devsnek Feb 26, 2019

misterdjules Feb 26, 2019

vdeturckheim left a comment

misterdjules commented Feb 27, 2019

addaleax Feb 28, 2019

misterdjules Mar 1, 2019

addaleax Feb 28, 2019

addaleax commented Feb 28, 2019

BridgeAR commented Mar 1, 2019

misterdjules commented Mar 1, 2019

BridgeAR commented Mar 27, 2019

misterdjules commented Mar 27, 2019

BridgeAR commented Dec 20, 2019 •

edited

Loading

misterdjules commented Dec 20, 2019

Trott commented Apr 22, 2020

Trott commented Apr 22, 2020

misterdjules commented May 4, 2020

Trott commented May 4, 2020

misterdjules commented May 7, 2020


		The stack trace is extended to include the point in time at which the
		`domain` module had been loaded.

domain: allow concurrent user-land impl #26326

domain: allow concurrent user-land impl #26326

Conversation

misterdjules commented Feb 26, 2019

Checklist

nodejs-github-bot commented Feb 26, 2019

devsnek Feb 26, 2019 • 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

vdeturckheim left a comment

Choose a reason for hiding this comment

misterdjules commented Feb 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Feb 28, 2019

BridgeAR commented Mar 1, 2019

misterdjules commented Mar 1, 2019

BridgeAR commented Mar 27, 2019

misterdjules commented Mar 27, 2019

BridgeAR commented Dec 20, 2019 • edited Loading

misterdjules commented Dec 20, 2019

Trott commented Apr 22, 2020

Trott commented Apr 22, 2020

misterdjules commented May 4, 2020

Trott commented May 4, 2020

misterdjules commented May 7, 2020

devsnek Feb 26, 2019 •

edited

Loading

BridgeAR commented Dec 20, 2019 •

edited

Loading