async_hooks: execute unhandledRejection cb in Promise execution context #37281

sajal50 · 2021-02-08T18:25:02Z

This commit now executes process.on('unhandledRejection') in the
async execution context of the concerned Promise.

sajal50 · 2021-02-08T18:28:02Z

test/async-hooks/test-unhandled-rejection-context.js

+//   throw new Error('Throwing on purpose');
+// });
+
+Promise.reject();


@benjamingr I noticed an interesting case. If you just do Promise.reject the async_hooks.executionAsyncId() is incorrect in process.on('unhandledRejection')'s callback. But it is correct if you do,

Promise.resolve() .then(() => { throw new Error('Throwing on purpose'); });

Therefore, I tried out setting the context in node_task_queue, and then it seems to be correct.

Let me know your thoughts.

addaleax · 2021-02-08T18:52:30Z

src/node_task_queue.cc

+        : v8::Just(AsyncWrap::kInvalidAsyncId);
+  }
+  else {
+      return Nothing<double>();


You generally don't want to return a Nothing<> unless there's an exception pending (because that's what it means)

Can probably return v8::Just(AsyncWrap::kInvalidAsyncId); here too ?

addaleax · 2021-02-08T18:53:11Z

src/node_task_queue.cc

+  GetAssignedPromiseWrapAsyncId(env, promise, env->async_id_symbol())
+            .To(&async_id);
+  GetAssignedPromiseWrapAsyncId(env, promise, env->trigger_async_id_symbol())
+          .To(&trigger_async_id);


These calls can lead to JS exceptions, so I think you want to move all of this into the TryCatchScope here and return if any of these .To() calls return false

sajal50 · 2021-02-09T03:17:31Z

src/node_task_queue.cc

+  }
+
+  if (try_catch_async_id.HasCaught()) {
+    // What must be done here?


What must we do here? I don't imagine this should throw. Shall we print like it's done below?

I don’t think we’ll get to this point here, because now we’re already returning early whenever an exception happens.

You can remove this conditional (and remove the second TryCatchScope, it’s okay to have a single one)

sajal50 · 2021-02-09T03:18:18Z

@addaleax @benjamingr Thank you both for the feedback.
I've addressed it. I have one open question.

Also, I am opening this PR from draft to run CI.

This commit now executes `process.on('unhandledRejection')` in the async execution context of the concerned `Promise`.

nodejs-github-bot · 2021-02-09T12:38:15Z

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

addaleax · 2021-02-09T13:07:48Z

src/node_task_queue.cc

+  if (async_id == AsyncWrap::kInvalidAsyncId &&
+      trigger_async_id == AsyncWrap::kInvalidAsyncId) {
+    // That means that promise might be a PromiseWrap, so we'll
+    // check there as well.


Can we add tests for both cases here?

I actually manually ran the other case by commenting this line out while running the test case added in this PR. It passed.

However, I guess we can change ActivityCollector to not use a noop function when ondestroy is not provided. What do you think?

I'm not sure why the test would need to use ActivityCollector, or be in test/async-hooks -- a lot of async_hooks tests that don't make use of the special features that that subdirectory has are just in test/parallel.

I see. What are these "special features" out of curiosity?

Also, if I just assumed initHooks had convenient methods when dealing with async hooks. What is otherwise the purpose?

What are these "special features" out of curiosity?

Things like ActivityCollector, and the rest that’s in async-hooks/init-hooks.js and async-hooks/hook-checks.js.

Also, if I just assumed initHooks had convenient methods when dealing with async hooks. What is otherwise the purpose?

They do, but it’s not like you have to use them. At least for the test here, the difference to using async hooks directly seems to be relatively small.

sajal50 · 2021-02-09T16:14:34Z

Also, are these CI failures due to flakiness?

nodejs-github-bot · 2021-02-09T16:25:01Z

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

sajal50 · 2021-02-09T22:34:44Z

@addaleax @benjamingr

I guess one check still fails. Let me know if it can be ignored.

Also, will wait for your guidance on this.

sajal50 · 2021-02-11T22:31:18Z

Polite 🔔
@addaleax @benjamingr

benjamingr · 2021-02-12T11:53:46Z

I'm fine with this landing but I want Anna to respond regarding the test question :)

If she doesn't and a week passes this can just land.

addaleax · 2021-02-12T23:57:13Z

Yeah, I'm good here, although I still think it would be good to cover both variants here. If it turns our to be hard to write a test then leaving a TODO comment about that might be fine.

This commit now executes `process.on('unhandledRejection')` in the async execution context of the concerned `Promise`. PR-URL: #37281 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

benjamingr · 2021-02-13T14:50:20Z

Landed in 7d5806e - thank you for contributing and bearing with us @sajal50

sajal50 · 2021-02-13T16:28:44Z

Thank you, both. :)
I'll address @addaleax's concern in a follow up PR.

This commit now executes `process.on('unhandledRejection')` in the async execution context of the concerned `Promise`. PR-URL: #37281 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels Feb 8, 2021

sajal50 commented Feb 8, 2021

View reviewed changes

addaleax reviewed Feb 8, 2021

View reviewed changes

sajal50 force-pushed the unhandledPromiseAsyncId branch from 8699a07 to 5b5e79e Compare February 9, 2021 03:14

sajal50 commented Feb 9, 2021

View reviewed changes

sajal50 marked this pull request as ready for review February 9, 2021 03:18

sajal50 force-pushed the unhandledPromiseAsyncId branch from 5b5e79e to 361d583 Compare February 9, 2021 03:31

async_hooks: execute unhandledRejection cb in Promise execution context

91fd525

This commit now executes `process.on('unhandledRejection')` in the async execution context of the concerned `Promise`.

sajal50 force-pushed the unhandledPromiseAsyncId branch from 361d583 to 91fd525 Compare February 9, 2021 12:32

addaleax approved these changes Feb 9, 2021

View reviewed changes

benjamingr approved these changes Feb 13, 2021

View reviewed changes

benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 13, 2021

benjamingr closed this Feb 13, 2021

This was referenced Feb 13, 2021

asyncId for unhandled rejected Promise #37244

Closed

executionAsyncId doesn't return expected value in context of process.on('unhandledRejection') #26794

Closed

sajal50 deleted the unhandledPromiseAsyncId branch February 13, 2021 16:27

This was referenced Feb 16, 2021

v15.9.0 proposal #37405

Closed

v15.9.0 proposal #37406

Merged

targos added the backport-blocked-v14.x label May 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_hooks: execute unhandledRejection cb in Promise execution context #37281

async_hooks: execute unhandledRejection cb in Promise execution context #37281

sajal50 commented Feb 8, 2021

sajal50 Feb 8, 2021 •

edited

Loading

addaleax Feb 8, 2021

benjamingr Feb 8, 2021

addaleax Feb 8, 2021

sajal50 Feb 9, 2021

addaleax Feb 9, 2021

sajal50 commented Feb 9, 2021

nodejs-github-bot commented Feb 9, 2021

addaleax Feb 9, 2021

sajal50 Feb 9, 2021 •

edited

Loading

addaleax Feb 12, 2021

sajal50 Feb 13, 2021

addaleax Feb 15, 2021

sajal50 commented Feb 9, 2021

nodejs-github-bot commented Feb 9, 2021

sajal50 commented Feb 9, 2021

sajal50 commented Feb 11, 2021

benjamingr commented Feb 12, 2021

addaleax commented Feb 12, 2021

benjamingr commented Feb 13, 2021

sajal50 commented Feb 13, 2021

async_hooks: execute unhandledRejection cb in Promise execution context #37281

async_hooks: execute unhandledRejection cb in Promise execution context #37281

Conversation

sajal50 commented Feb 8, 2021

sajal50 Feb 8, 2021 • 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

sajal50 commented Feb 9, 2021

nodejs-github-bot commented Feb 9, 2021

Choose a reason for hiding this comment

sajal50 Feb 9, 2021 • 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

sajal50 commented Feb 9, 2021

nodejs-github-bot commented Feb 9, 2021

sajal50 commented Feb 9, 2021

sajal50 commented Feb 11, 2021

benjamingr commented Feb 12, 2021

addaleax commented Feb 12, 2021

benjamingr commented Feb 13, 2021

sajal50 commented Feb 13, 2021

sajal50 Feb 8, 2021 •

edited

Loading

sajal50 Feb 9, 2021 •

edited

Loading