Node: Segfault / error while bootstrapping

[BridgeAR]

• Version: 8.x, 9.x, 10.x, master
• Platform: Ubuntu 18.04
• Subsystem: domain

The following code segfaults running on 8.x.
Running the same code from 9.x onward it will throw an Maximum callstack size exceeded error even though no error should be thrown at all due to the catch handler attached (sync error are converted to a unhandled rejection and that rejection should be silenced by the catch handler).

'use strict';

const domain = require('domain');

const d = new domain.Domain();

d.run(() => {
  async function fn() {
    fn().catch(() => {});
  }
  fn();
});

The error on 10.5 looks like:

internal/async_hooks.js:144
    fatalError(e);
    ^

RangeError: Maximum call stack size exceeded
    at PromiseWrap.emitInitNative (internal/async_hooks.js:144:5)
    at <anonymous>
    at fn (/home/ruben/repos/node/node/t.js:8:20)
    at fn (/home/ruben/repos/node/node/t.js:9:5)
    at fn (/home/ruben/repos/node/node/t.js:9:5)
    at fn (/home/ruben/repos/node/node/t.js:9:5)

The call site on master is:

internal/async_hooks.js:131
function emitInitNative(asyncId, type, triggerAsyncId, resource) {
                       ^

On 8:

FATAL ERROR: node::AsyncWrap::MakeCallback domain enter callback threw, please report this
 1: node::Abort() [node]
 2: 0x8c21ec [node]
 3: 0x8c22cd [node]
 4: 0x8c252b [node]
 5: node::Environment::EnvPromiseHook(v8::PromiseHookType, v8::Local<v8::Promise>, v8::Local<v8::Value>) [node]
 6: v8::internal::Isolate::RunPromiseHook(v8::PromiseHookType, v8::internal::Handle<v8::internal::JSPromise>, v8::internal::Handle<v8::internal::Object>) [node]
 7: v8::internal::Runtime_PromiseHookBefore(int, v8::internal::Object**, v8::internal::Isolate*) [node]
 8: 0x195018f042fd
Aborted (core dumped)

Ping @nodejs/async_hooks @nodejs/domains

[AndreasMadsen]

The error is not caught by domain because the following code doesn't even throw an error, it rejects a promise. And domain doesn't capture promise rejections. This is something we could change, but right now it is the correct behavior.

function fn() {
  const p = new Promise(function () {
    fn();
  }).catch(function (err) {
    console.log('promise catch');
  });

  return p;
}

fn();

However, the behaviour of the above code is still wrong. Because it prints UnhandledPromiseRejectionWarning but not promise catch. The expected behavior is that it prints only promise catch. But I think this a separate issue.

In conclusion, I suspect that this has little to do with async_hooks or domain. Again, domain doesn't catch promise rejections!

[jasnell]

Just wanted to verify quickly... running the following in Chrome results in the max call stack error also.

async function fun() { fun() };
fun();

Attaching the catch handler in chrome handles the error

async function fun() { fun().catch(() => {}); }
fun();

... just as one would expect...

Interestingly... in Node.js 10.5, take a look at the difference between these two... We get different results simply by creating the domain.. even if the async function fun is not run within the domain:

fun1.js:

const domain = require('domain');
const d = new domain.Domain();

async function fun() { fun().catch(()=>{}) }
fun();

$ node ~/tmp/fun1.js
internal/async_hooks.js:144
    fatalError(e);
    ^

RangeError: Maximum call stack size exceeded
    at PromiseWrap.emitInitNative (internal/async_hooks.js:144:5)
    at <anonymous>
    at fun (/home/james/tmp/fun1.js:5:19)
    at fun (/home/james/tmp/fun1.js:5:24)
    at fun (/home/james/tmp/fun1.js:5:24)
    at fun (/home/james/tmp/fun1.js:5:24)
    at fun (/home/james/tmp/fun1.js:5:24)
    at fun (/home/james/tmp/fun1.js:5:24)
    at fun (/home/james/tmp/fun1.js:5:24)
    at fun (/home/james/tmp/fun1.js:5:24)
$ 

fun2.js:

async function fun() { fun().catch(()=>{}) }
fun();

$ node ~/tmp/fun2.js
$ 

[addaleax]

@jasnell I don’t think Node is behaving incorrectly in your examples – we basically have an explicit try { … } catch {} in there that makes sure we crash the process and don’t propagate the exception, even if there is a JS stack below

[jasnell]

Yeah, I don't think it's incorrect either, it's just unexpected, and likely worth documenting. The segfault that @BridgeAR is seeing in 8 likely needs to be fixed tho :-)

[BridgeAR]

@addaleax

I don’t think Node is behaving incorrectly in your examples

Please have another look at the fine grained differences: each promise created besides the top level promise has a individual catch handler that should silence all errors, no matter what kind of error it is.
The error should therefore not be printed at all.

But that is not the only fun part: running @jasnell "fun2" example in the repl will indeed print the exception and it will also exit the repl immediately. [edit] The reason for that could be though that we use domains in the repl [/edit].

[addaleax]

@BridgeAR Yes, I understand that. However, we also document that errors thrown from inside an async_hooks hook will terminate the process – the thing that seems to surprise people here is that async_hooks can be called even when the call stack limit is close to being exceeded.

[BridgeAR]

I am able to trigger weird behavior without domains (I guess async_hooks are called due to the domain) as well:

async function fn() {
  fn().catch(() => {});
}
fn().catch(() => {}).then(() => setTimeout(() => {}, 1));

This prints:

(node:2290) UnhandledPromiseRejectionWarning: RangeError: Maximum call stack size exceeded
    at Promise.then (<anonymous>)
    at Promise.catch (<anonymous>)
    at fn (/home/ruben/repos/node/node/t.js:10:13)
    at fn (/home/ruben/repos/node/node/t.js:10:3)
    at fn (/home/ruben/repos/node/node/t.js:10:3)
    at fn (/home/ruben/repos/node/node/t.js:10:3)
    at fn (/home/ruben/repos/node/node/t.js:10:3)
    at fn (/home/ruben/repos/node/node/t.js:10:3)
    at fn (/home/ruben/repos/node/node/t.js:10:3)
    at fn (/home/ruben/repos/node/node/t.js:10:3)
(node:2290) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:2290) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

But I believe it should not print any rejection at all here.

[addaleax]

@BridgeAR Why is that most recent example here unexpected behaviour? The first thing to happen in that script is inifinite recursion of fn() calls, even before any Promises come into play

[BridgeAR]

@addaleax that is correct. So that error is then turned into a rejection that should be caught by the calling function in the catch handler and silenced. The handler itself could throw a new maximum call stack size error but I would expect it to be a sync error and not a rejection in that case (I might be wrong though). The rejection above is also not triggered in case the setTimeout function is removed in the then handler, so I wonder what influence that actually has on the code.

I have another example though that has a interesting behavior:

setTimeout(() => {
  async function fn() {
    fn().catch(() => {});
  }
  fn();
}, 1);

This throws an exception in Node.js but it does not trigger one in Chrome. Doing the same without wrapping it in the setTimeout will not trigger an error in Node.js.

[addaleax]

Yes, that last example really shouldn’t fail. :/

[BridgeAR]

@hashseed @bmeurer @MayaLekova PTAL

[BridgeAR]

Here is another example that should behave differently:

async function fn() {
  fn();
}
fn();

This should trigger a rejection and it does in Chrome but it triggers an exception in Node.js.

[benjamingr]

@AndreasMadsen you forgot to return in the code that doesn't enter the catch. We're talking about adding warnings for this.

[MayaLekova]

I have another example though that has a interesting behavior: ...

@BridgeAR Random observation: accidentally tried this example with Node.js v8.11.1 first and it doesn't fail there. Will try to provide more relevant info soon.

[AndreasMadsen]

@AndreasMadsen you forgot to return in the code that doesn't enter the catch. We're talking about adding warnings for this.

@benjamingr Sorry. I can't visualize what that means.

[Trott]

@BridgeAR This should stay open?

[hashseed]

I believe the issue with this code

async function fn() {
  fn().catch(() => {});
}
fn().catch(() => {}).then(() => setTimeout(() => {}, 1));

is that

• we first perform an infinite recursion
• on the N-th recursion, we overflow the stack
• this results in an exception being thrown
• the N-th recursion therefore returns a rejected promise
• the (N-1)-th recursion takes this return value, and calls .catch on it
• .catch calls into .then
• we are still very close to the stack limit, so this again causes an exception
• because the exception aborted .catch, there is no rejection handler
• the (N-1)-th recursion returns a rejected promise as well
• the (N-2)-th recursion gets a rejected promise and tries to call .catch on it again
• that fails again
• the unhandled rejection gets reported
• the stack trace we see is the one where the (N-1)-th recursion failed to call .catch

I'm not sure what should be done differently here.

It's _technically_ correct :)

[jasnell]

It's not clear if there's anything actionable here and there's been no activity. Closing. Can reopen if necessary