async_hooks: execute unhandledRejection cb in Promise execution context by sajal50 · Pull Request #37281 · nodejs/node

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 <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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

Uh oh!

Conversation

sajal50 commented Feb 8, 2021

Uh oh!

sajal50 Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sajal50 commented Feb 9, 2021

Uh oh!

nodejs-github-bot commented Feb 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sajal50 Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sajal50 commented Feb 9, 2021

Uh oh!

nodejs-github-bot commented Feb 9, 2021

Uh oh!

sajal50 commented Feb 9, 2021

Uh oh!

sajal50 commented Feb 11, 2021

Uh oh!

benjamingr commented Feb 12, 2021

Uh oh!

addaleax commented Feb 12, 2021

Uh oh!

benjamingr commented Feb 13, 2021

Uh oh!

sajal50 commented Feb 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sajal50 Feb 8, 2021 •

edited

Loading

sajal50 Feb 9, 2021 •

edited

Loading