domain: re-implement domain over async_hook #16222

vdeturckheim · 2017-10-15T16:18:07Z

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

Affected core subsystem(s)

Tentative to re-implement domains over async_hook API.

Currently this breaks the following tests:

~~those using --abort-on-uncaught-exception flag: I have not found a way to intercept those errors event by looking to process._fatalException~~
- ~~parallel/test-domain-abort-on-uncaught.js~~
- ~~parallel/test-domain-with-abort-on-uncaught-exception~~
~~the one which expect an exit code of 7 when an error is thrown from a domain error handler: I have not found where this was done in the previous version.~~
- ~~test-domain-top-level-error-handler-throw~~
~~those linked to makeCallback: It seems there is some domain handling happening in native code I could not fix.~~
- ~~addons/make-callback-recurse/test~~
- ~~addons-napi/test_make_callback_recurse/test~~

addaleax · 2017-10-15T17:26:50Z

lib/domain.js


-function Domain() {
-  EventEmitter.call(this);
+      if (process.domain === self) { // if this operation is created while in a domain, let's mark it


You probably also want to set resource.domain = process.domain in this case

Done, good point!

addaleax · 2017-10-15T17:29:31Z

Cool that you’re taking this on! I’ll assign myself because I really want to see this happen and hope I can help here, please don’t take this as a sign that I’ll be claiming any specific tasks for only me :)

vdeturckheim · 2017-10-15T18:19:08Z

Thanks @addaleax . Let's see how we can save domains!

mcollina · 2017-10-16T09:07:10Z

lib/domain.js


+Domain.prototype._enable = function() {


can you make this function a top-level function? Exposing it inside the prototype does not seem correct, as an external user could call it.

mcollina · 2017-10-16T09:10:16Z

lib/domain.js

+    this.members = [];
+    this.asyncIds = new Set();
+    this.asyncHook = null; // will hold the asyncHook after _enable is called
+    this._enable();


this will make every domain created register new async_hooks. I'm not 100% sure this is the intended usage, and what side effect it would create. cc @AndreasMadsen

Yeah, I think a Map that keeps track of the async id → domain mapping would be a better idea…

AndreasMadsen · 2017-10-16T10:20:07Z

lib/domain.js

+  constructor(...args) {
+    super(...args);
+
+    this.domain = process.domain;


This won't work for async function promises. Set the property using resource.promise in the init-hook instead.

Be careful with #15673 if you do that though.

Great pointer, thanks @AndreasMadsen

@TimothyGu is there any way to know if the promise has been init in such context?

@vdeturckheim I think resource.promise instanceof Promise should be fine, it’s false for promises from other contexts

it works, thanks @addaleax !

AndreasMadsen

I would like to review this in detail but I don't have time right now. The comments made so far are worth addressing anyway.

Thanks for doing this.

PS: also remember to clean up in C++ land too.

AndreasMadsen · 2017-10-22T18:01:12Z

lib/domain.js

+    after(asyncId) {
+
+      if (pairing.has(asyncId)) { // exit domain for this cb
+        process.domain = null;


Would it not be possible to exit one domain and enter another, as in nested domains?

This kind of before and after pattern usually requires a stack, so I'm always suspicious when no stack is used.

Ack. Would make sense yes!

Actually, I should even use domain.enter and domain.exit for before and after hooks. That would use the domain stack.

Absolutely!

AndreasMadsen · 2017-10-22T18:03:34Z

lib/domain.js

@@ -48,13 +48,61 @@ Object.defineProperty(process, 'domain', {
  }
 });

+let asyncHook = null;


Just call createHook immediately, it is simpler. We do that for the the inspector integration and soon trace_events integration too.

Good point!

AndreasMadsen · 2017-10-22T18:04:32Z

lib/domain.js

+
+      if (process.domain !== null) { // if this operation is created while in a domain, let's mark it
+        pairing.set(asyncId, process.domain);
+        resource.domain = process.domain;


Was the handle objects mutated before too?

I don't thinks so, it was made on request #16222 (comment)

@addaleax could you explain why this is needed? I see no need to add extra domain complexity if it didn't already exist.

@AndreasMadsen The thing is – it does already exist. With this, we can remove the code that sets it in other places, which would be very nice (look around for domain_string() in the source).

For event emitters (and therefore the handle wraps), the property is added in events.js, so we might need to come up with a better solution there.

Oh, by the way; this is essentially how MakeCallback kept track of the domain – by looking at the resource object’s .domain property

The thing is – it does already exist.

Okay, then it is fine. I thought it was only EventEmitter and Promise.

Yeah, those + all request types – that sums up everything in core …

AndreasMadsen · 2017-10-22T18:12:02Z

It would be nice if we could remove https://github.com/nodejs/node/blob/master/src/node.cc#L1105L1278 at the very least DomainEnter, DomainExit, and promise setup should be removable.

AndreasMadsen · 2017-10-22T18:14:29Z

those linked to makeCallback: It seems there is some domain handling happening in native code I could not fix.

Yeah. It has always been a mystery to me, how domain managed to keep the context using only MakeCallback (before and after hooks).

vdeturckheim · 2017-10-22T18:14:33Z

@AndreasMadsen agreed regarding c++ land. I am currently looking on how to have DomainsStackHasErrorHandler (https://github.com/nodejs/node/blob/master/src/node.cc#L1127) working with this.

AndreasMadsen · 2017-10-24T20:31:10Z

lib/domain.js

+    pairing.delete(asyncId); // cleaning up
+  }
+});
+asyncHook.enable();


Move this to the Domain constructor, so require('domain') in itself doesn't slow us down. If the asyncHook is already enabled it will just be a noop. Hooks can only be enabled once.

AndreasMadsen · 2017-10-25T06:06:33Z

src/node.cc

-
-  process_object->Set(env->tick_callback_string(), tick_callback_function);
-  env->set_tick_callback_function(tick_callback_function);
-
  CHECK(args[0]->IsArray());
  env->set_domain_array(args[0].As<Array>());


We should be able to remove this too, including all uses of domain_array. As I see itset_domain_array is only used to set the domain property on resource object in C++. We now do that using async_hooks.

Ack! good catch!

AndreasMadsen · 2017-10-25T06:14:03Z

Those linked to makeCallback: It seems there is some domain handling happening in native code I could not fix.

I looked at addons/make-callback-recurse/test. I think we need to call domainEnter and domainExit when async_context_.async_id == 0 && env->using_domains() && !object_.IsEmpty(). This is to check for the domain property and set the active domain appropiately.

vdeturckheim · 2017-10-26T22:55:41Z

@AndreasMadsen thanks for the pointers here. Seems to work now \o/

AndreasMadsen

I’m pretty sure there are more things we can do but this is a really good first step.

AndreasMadsen · 2017-10-27T06:50:19Z

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 = stack;
+process._setupDomainUse(stack);
+
+class Domain extends EventEmitter {


Technically changing this to ES6 style classes is API breaking. We are likely going to land this in node 9 anyway so it shouldn't be a problem. Just as a note.

AndreasMadsen

After a closer look, I think we can remove a little more.

AndreasMadsen · 2017-10-27T06:54:22Z

src/node.cc


  // Do a little housekeeping.
  env->process_object()->Delete(
      env->context(),
      FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupDomainUse")).FromJust();
-
-  uint32_t* const fields = env->domain_flag()->fields();


Remove the domain_flag declaration from env.h and env-inl.h.

Should be good now

AndreasMadsen · 2017-10-27T06:56:42Z

src/node.cc

  CHECK(args[0]->IsArray());
-  env->set_domain_array(args[0].As<Array>());


I think it was lost in a collapsed comment: we can remove all uses of domain_array. Just do a search for domain_array.

This should be good now. I cleaned up some domain related flags in multiple places ( 821f51d ) Hop I did not miss any

If it compiles without the domain_array definition I think it should be good :)

AndreasMadsen · 2017-10-27T06:57:52Z

src/node.cc

-  uint32_t* const fields = env->domain_flag()->fields();
-  uint32_t const fields_count = env->domain_flag()->fields_count();
-
-  Local<ArrayBuffer> array_buffer =


I can't figure out what this array_buffer was actually used for before :/

I'm not certain either. Seems to come from this PR #2022 . But could not really see in detail what it does.

AndreasMadsen · 2017-10-27T07:01:40Z

src/node.cc

@@ -1155,7 +1154,6 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) {
  return isEmittingTopLevelDomainError || !DomainsStackHasErrorHandler(env);


I would like to understand why the returned caught boolean in process._fatalException is not enough to determine if we should abort on uncaughtException.

AndreasMadsen · 2017-10-29T14:54:53Z

src/env.h

@@ -563,7 +561,6 @@ class Environment {
  inline void FinishHandleCleanup(uv_handle_t* handle);

  inline AsyncHooks* async_hooks();
-  inline DomainFlag* domain_flag();


The DomainFlag type is no longer used. Can you remove the class

node/src/env.h

Line 466 in e3503ac

class DomainFlag {

:)

Good catch!

AndreasMadsen

LGTM again :)

I'm sure we can do more. But it doesn't have to be in one PR. I think it would be really nice to get the exception handling out of C++. I don't think we can do so much about the DomainBefore and DomainAfter parts, without breaking backward compatibility to some extent. But we should definitely consider doing too, it is not like we make it impossible to use domain with native addons, they can just use async_hooks :)

vdeturckheim · 2017-10-29T15:18:37Z

Awesome thanks!

Btw, do we want to update the depreciation status of the API in this PR too?

Due to some changes to async tracking of http and also in how domains are handled, it's no longer necessary to manually copy domain from req to res in http code. PR-URL: #18477 Refs: #16222 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

It is no longer necessary to explicitly set the handle to inherit the Timeout domain. PR-URL: #18477 Refs: #16222 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

Due to some changes to async tracking of http and also in how domains are handled, it's no longer necessary to manually copy domain from req to res in http code. PR-URL: #18477 Refs: #16222 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

It is no longer necessary to explicitly set the handle to inherit the Timeout domain. PR-URL: #18477 Refs: #16222 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

Due to some changes to async tracking of http and also in how domains are handled, it's no longer necessary to manually copy domain from req to res in http code. PR-URL: #18477 Refs: #16222 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

It is no longer necessary to explicitly set the handle to inherit the Timeout domain. PR-URL: #18477 Refs: #16222 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

Due to some changes to async tracking of http and also in how domains are handled, it's no longer necessary to manually copy domain from req to res in http code. PR-URL: nodejs#18477 Refs: nodejs#16222 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

It is no longer necessary to explicitly set the handle to inherit the Timeout domain. PR-URL: nodejs#18477 Refs: nodejs#16222 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

test-microtask-queue-run-immediate-domain.js tests that the behavior of another test, test-microtask-queue-run-immediate.js, is still consistent when the core domain module is loaded. This was needed because before the changes in #1622 were merged, the core domain module would replace the function that would call nextTick callbacks with a different implementation. This is no longer the case, and as such that test is no longer needed. R-URL: #23252 Refs: #16222 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

test-microtask-queue-integration-domain.js tests that the behavior of another test, test-microtask-queue-integration.js, is still consistent when the core domain module is loaded. This was needed because before the changes in nodejs#16222 were merged, the core domain module would replace the function that would call nextTick callbacks with a different implementation. This is no longer the case, and therefore that test is no longer needed.

test-microtask-queue-run-domain.js tests that the behavior of another test, test-microtask-queue-run.js, is still consistent when the core domain module is loaded. This was needed because before the changes in nodejs#16222 were merged, the core domain module would replace the function that would call nextTick callbacks with a different implementation. This is no longer the case, and therefore that test is no longer needed.

test-microtask-queue-integration-domain.js tests that the behavior of another test, test-microtask-queue-integration.js, is still consistent when the core domain module is loaded. This was needed because before the changes in #16222 were merged, the core domain module would replace the function that would call nextTick callbacks with a different implementation. This is no longer the case, and therefore that test is no longer needed. PR-URL: #23342 Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

test-microtask-queue-run-domain.js tests that the behavior of another test, test-microtask-queue-run.js, is still consistent when the core domain module is loaded. This was needed because before the changes in #16222 were merged, the core domain module would replace the function that would call nextTick callbacks with a different implementation. This is no longer the case, and therefore that test is no longer needed. PR-URL: #23343 Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

test-microtask-queue-run-immediate-domain.js tests that the behavior of another test, test-microtask-queue-run-immediate.js, is still consistent when the core domain module is loaded. This was needed because before the changes in #1622 were merged, the core domain module would replace the function that would call nextTick callbacks with a different implementation. This is no longer the case, and as such that test is no longer needed. R-URL: #23252 Refs: #16222 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

test-microtask-queue-integration-domain.js tests that the behavior of another test, test-microtask-queue-integration.js, is still consistent when the core domain module is loaded. This was needed because before the changes in #16222 were merged, the core domain module would replace the function that would call nextTick callbacks with a different implementation. This is no longer the case, and therefore that test is no longer needed. PR-URL: #23342 Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

test-microtask-queue-run-domain.js tests that the behavior of another test, test-microtask-queue-run.js, is still consistent when the core domain module is loaded. This was needed because before the changes in #16222 were merged, the core domain module would replace the function that would call nextTick callbacks with a different implementation. This is no longer the case, and therefore that test is no longer needed. PR-URL: #23343 Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

test-microtask-queue-run-immediate-domain.js tests that the behavior of another test, test-microtask-queue-run-immediate.js, is still consistent when the core domain module is loaded. This was needed because before the changes in nodejs/node#1622 were merged, the core domain module would replace the function that would call nextTick callbacks with a different implementation. This is no longer the case, and as such that test is no longer needed. Refs: nodejs/node#16222

nodejs-github-bot added domain Issues and PRs related to the domain subsystem. process Issues and PRs related to the process subsystem. labels Oct 15, 2017

addaleax added the wip Issues and PRs that are still a work in progress. label Oct 15, 2017

addaleax reviewed Oct 15, 2017

View reviewed changes

addaleax self-assigned this Oct 15, 2017

jasnell requested review from mcollina and trevnorris October 15, 2017 22:45

mcollina reviewed Oct 16, 2017

View reviewed changes

AndreasMadsen reviewed Oct 16, 2017

View reviewed changes

AndreasMadsen assigned AndreasMadsen and unassigned AndreasMadsen Oct 16, 2017

AndreasMadsen reviewed Oct 16, 2017

View reviewed changes

AndreasMadsen reviewed Oct 22, 2017

View reviewed changes

AndreasMadsen reviewed Oct 24, 2017

View reviewed changes

AndreasMadsen added the async_hooks Issues and PRs related to the async hooks subsystem. label Oct 25, 2017

AndreasMadsen reviewed Oct 25, 2017

View reviewed changes

AndreasMadsen approved these changes Oct 27, 2017

View reviewed changes

AndreasMadsen reviewed Oct 27, 2017

View reviewed changes

AndreasMadsen reviewed Oct 29, 2017

View reviewed changes

AndreasMadsen approved these changes Oct 29, 2017

View reviewed changes

misterdjules mentioned this pull request Oct 4, 2018

test: remove obsolete domain test #23252

Closed

2 tasks

misterdjules mentioned this pull request Oct 9, 2018

test: remove obsolete domain test #23342

Closed

2 tasks

misterdjules mentioned this pull request Oct 9, 2018

test: remove obsolete domain test #23343

Closed

2 tasks

vdeturckheim mentioned this pull request Dec 20, 2019

[async hooks] proposal for standard CLS API - request for feedback nodejs/diagnostics#345

Closed

mikeconley12 mentioned this pull request Aug 3, 2020

Async Local Storage for Request Context mikro-orm/mikro-orm#575

Closed

AdriVanHoudt mentioned this pull request May 21, 2021

Feature request: proper full hapi support getsentry/sentry-javascript#2172

Closed

		CHECK(args[0]->IsArray());
		env->set_domain_array(args[0].As<Array>());

		@@ -1155,7 +1154,6 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) {
		return isEmittingTopLevelDomainError \|\| !DomainsStackHasErrorHandler(env);


		Domain.prototype._enable = function() {

domain: re-implement domain over async_hook #16222

domain: re-implement domain over async_hook #16222

Conversation

vdeturckheim commented Oct 15, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Oct 15, 2017

vdeturckheim commented Oct 15, 2017

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

TimothyGu Oct 16, 2017 • 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

AndreasMadsen left a comment

Choose a reason for hiding this comment

AndreasMadsen Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdeturckheim Oct 22, 2017 • 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

AndreasMadsen commented Oct 22, 2017

AndreasMadsen commented Oct 22, 2017

vdeturckheim commented Oct 22, 2017

AndreasMadsen Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen commented Oct 25, 2017 • edited Loading

vdeturckheim commented Oct 26, 2017

AndreasMadsen left a comment

Choose a reason for hiding this comment

AndreasMadsen Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

AndreasMadsen left a 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

AndreasMadsen left a comment • edited Loading

Choose a reason for hiding this comment

vdeturckheim commented Oct 29, 2017

vdeturckheim commented Oct 15, 2017 •

edited

Loading

TimothyGu Oct 16, 2017 •

edited

Loading

AndreasMadsen Oct 22, 2017 •

edited

Loading

vdeturckheim Oct 22, 2017 •

edited

Loading

AndreasMadsen Oct 24, 2017 •

edited

Loading

AndreasMadsen commented Oct 25, 2017 •

edited

Loading

AndreasMadsen Oct 27, 2017 •

edited

Loading

AndreasMadsen left a comment •

edited

Loading