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

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

@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.

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 ?

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

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)

@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`.

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

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.

Also, are these CI failures due to flakiness?

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

@addaleax @benjamingr

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

Also, will wait for your guidance on this.

Polite 🔔
@addaleax @benjamingr

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.

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>

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

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 outstanding review comments, 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

+                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.

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

Uh oh!

Uh oh!

async_hooks: execute unhandledRejection cb in Promise execution context - #37281

async_hooks: execute unhandledRejection cb in Promise execution context#37281sajal50 wants to merge 1 commit intonodejs:masternodejs/node:masterfrom sajal50:unhandledPromiseAsyncIdsajal50/node:unhandledPromiseAsyncIdCopy head branch name to clipboard

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

async_hooks: execute unhandledRejection cb in Promise execution context#37281
sajal50 wants to merge 1 commit into
nodejs:masterfrom
sajal50:unhandledPromiseAsyncId

sajal50 Feb 8, 2021 •
edited
Loading

sajal50 Feb 9, 2021 •
edited
Loading