async_hooks: fatalError(e) function checking error Object

[PhakornKiong]

While going through the test coverage, I noticed that the else branch is actually not covered in the test case here

node/lib/internal/async_hooks.js

Lines 160 to 174 in 17a527e

	function fatalError(e) {
	if (typeof e.stack === 'string') {
	process._rawDebug(e.stack);
	} else {
	const o = { message: e };
	ErrorCaptureStackTrace(o, fatalError);
	process._rawDebug(o.stack);
	}
	const { getOptionValue } = require('internal/options');
	if (getOptionValue('--abort-on-uncaught-exception')) {
	process.abort();
	}
	process.exit(1);
	}

Will the else case ever happened? According to the Node.js docs here, error.stack is always a string. So why would we ever do a check on that portion and have an else branch?

From what I understand, fatalError(e) is only used to capture error when the callback from the registered hooks failed and it doesn't seem necessary in this case.

Let me know what you think.

[Ayase-252]

FYI: this branch was brought with initial implementation #12892

I don't know the exact reason. May it need view from author?

cc @AndreasMadsen

[Linkgoron]

I think that this was the reasoning : #11883 (comment), to filter "real" Errors

[AndreasMadsen]

cc @nodejs/async_hooks as I'm unlikely to look at this.

[Flarna]

According to the Node.js docs here, error.stack is always a string

That's true for Error objects but in Javascript you can throw also non errors, e.g. throw 23 or even throw undefined.

[PhakornKiong]

True enough on throwing non-error object. However my question is that whether this if-else is necessary? Based on #11883, it seems to be trying to capture error from v8?

I'm not entirely sure of the intention here.

[AndreasMadsen]

@Flarna @Darkripper214 A fatal error here is just any error thrown in an async_hooks callback. So yes, throw 23 is a possibility.

[Flarna]

@Darkripper214 The if path handles anything thrown which has a stack property of type string - which should cover all thrown Error objects. One may think that using instanceof Error would be better but there could be handwritten error types slipping through then.

The else path takes care about all the rest (e.g. throw 23) which has no stack. Then ErrorCaptureStackTrace is called to get a stacktrace pointing to the location where this was catched. Clearly not as good as getting the stack from the throw location but better then nothing - at least users know that it happened within an async hook.

If you remove the else branch and for whatever reason someone throws a non Error in one of the async hooks the node process would just exit without any indication why. Currently you get a stacktrace pointing to async hooks.

But I think we should change if (typeof e.stack === 'string') to if (typeof e?.stack === 'string') otherwise throw null results in an exception in fatalError.

[PhakornKiong]

@Flarna Thanks for the clarification. So I was misunderstanding the functionality of ErrorCaptureStackTrace here.

I think this had turned into an issue for the throw null case. I will open a new issue with new PR to fix this and update the test to cover the else case properly.