async_hooks: async hook stack corruption when exception prevents AsyncResource.emitAfter being called.

[akdor1154]

• Version: 8.2.1, 8.7
• Platform:
• Subsystem: async_hooks

According to async_hooks docs,

If the user's callback throws an exception, emitAfter() will automatically be called for all asyncIds on the stack if the error is handled by a domain or 'uncaughtException' handler.

However, this doesn't seem to work in practice. If I set up a simple AsyncResource to bind a function to its own execution context,

class BoundFunction extends asyncHooks.AsyncResource {
 constructor(f) {
 super('BOUND_FUNCTION');
 this.f = f;
 }

 run() {
 this.emitBefore();
 f();
 this.emitAfter();
 }
}

and use it like

const f = () => {
 throw new Error('uh ho');
};

const boundF = new BoundFunction(f);

process.nextTick( () => {
 try {
 boundF.run();
 } catch (e) {
 console.log('nothing to worry about');
 }
});

Then I get the following output from node:

$ node src/demonstration.js
nothing to worry about
Error: async hook stack has become corrupted (actual: 5, expected: 6)
 1: node::AsyncWrap::PopAsyncIds(v8::FunctionCallbackInfo<v8::Value> const&) [node]
 2: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [node]
 3: 0xb8f43c [node]
 4: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node]
 5: 0x3aa19890463d

This happens even if I set up handlers for uncaught exceptions and rejections:

process.on('uncaughtException', (e) => { console.log('no worries'); });
process.on('unhandledRejection', (e) => { console.log('relax bro'); });

If I change BoundFunction#run to look like

run() {
 this.emitBefore();
 try {
 f();
 } finally {
 this.emitAfter();
 }
}

the error goes away. I am fine with my handler looking like that, but it's a bit confusing that the docs seem to say that it shouldn't be required.

[addaleax]

if the error is handled by a domain or 'uncaughtException' handler.

That isn’t the case here because you have wrapped the call to run() inside a try/catch – so it doesn’t quite qualify as an uncaught exception :)

the error goes away. I am fine with my handler looking like that, but it's a bit confusing that the docs seem to say that it shouldn't be required.

Yeah, that’s how the handler should look like. I think your example here would make a good addition to the docs, would you be interested in submitting it as a PR?

[AndreasMadsen]

Yeah, that’s how the handler should look like. I think your example here would make a good addition to the docs, would you be interested in submitting it as a PR?

@addaleax No! That is not how it should look. We have code specifically in place such that isn't necessary. https://github.com/nodejs/node/blob/d828c01a656c41454adda3876eaeaa2c001f55d8/lib/internal/bootstrap_node.js#L392L401

I think it is simply that the code block should be after if (!caught) and not inside if (!caught).

[addaleax]

@AndreasMadsen I don’t think so – in the situation described here the current approach (that I do want to get rid of in place of proper C++ RAII handling) is not really going to work…

I think it is simply that the code block should be after if (!caught) and not inside if (!caught).

Changing anything inside the uncaught exception handler isn’t going to be effective here?

[AndreasMadsen]

Changing anything inside the uncaught exception handler isn’t going to be effective here?

Oh, right. But, I'm pretty sure @trevnorris didn't want it to look like that. Because it wasn't robust or something.

[AndreasMadsen]

From the EPS.

If the user's callback thrown an exception then emitAfter() will automatically be called for all id's on the stack if the error is handled by a domain or 'uncaughtException' handler. So there is no need to guard against this.

In the documentation PR I wanted makeCallback so we could easier control this #12953 (comment), @trevnorris was somewhat against this #12953 (comment).

But I think I might have misunderstood the discussion since the discussions mostly deal with when there are no try{} catch{} above. I think we simply forgot about the try{} catch{} above case.

[addaleax]

I would agree that a wrapper like makeCallback would be a better option in any case.

[akdor1154]

Case above also happens when there is no try/catch in nextTick.
EDIT: ignore me, no it doesn't sorry.

[apapirovski]

Doesn't look like there is anything actionable here so I'm going to close it out. That said feel free to re-open if you feel I'm wrong.