unhandledRejection falls into infinite recursion

[eduardbme]

• Version: 8+ (probably all versions that supports unhandledRejection event)
• Platform: linux
• Subsystem: process

According to documentation of uncaughtException - 'Exceptions thrown from within the event handler will not be caught. Instead the process will exit with a non-zero exit code and the stack trace will be printed. This is to avoid infinite recursion.'.

So why unhandledRejection allows you to fall into infinite recursion if code within handler creates unhandled promise rejection ? I expect the same behavior as for uncaughtException (exit process with non-zero code).

unhandledRejection handler may contain some logic to log that situation, and owing to the fact that 3-d party logger implementation can produce unhandled rejection within himself , we can have infinite recursion.

Code example to reproduce:

'use strict';

process.on('unhandledRejection', uncaughtRejection => {
    console.log('unhandledRejection handler catch: ' + uncaughtRejection);

    Promise.reject(3);
});

function foo() {
    return Promise.reject(1);
}

foo();

Thanks in advance.

[eduardbme]

I propose to emit warning about promise unhandled rejection on the second recursion call, and exit process with error code, probably some special code to indicate specificity of this exit event.

I don't expect that any code rely on this behavior, so I assume that it's safe to fix it.
May prepare Christmas fix for that.

[bnoordhuis]

Working as intended, IMO. Promise.reject(3) doesn't throw an exception, it creates a promise that triggers a new unhandledRejection event on the next tick. There is no recursion, just repeated unhandledRejection events.

If you replace it with a throw 'boom', you get the behavior you expect. Replace it with Promise.reject(3).catch(console.error) and you get only a single unhandledRejection event.

[eduardbme]

Okay, let me show you some another code.

'use strict';

process.on('unhandledRejection', uncaughtRejection => {
    console.log('unhandledRejection handler catch: ' + uncaughtRejection);

    logger.error('log some error'); // 3-d party logger
});

function foo() {
    return Promise.reject(1);
}

foo();

// somewhere in logger module
// code that you don't control
logger = {
  error() {
    Promise.reject(1); // sometimes it happens..
  }
}

you got the idea ?

[eduardbme]

There is no recursion, just repeated unhandledRejection events.

Why we cannot say the same thing about uncaughtException. "There is no recursion, just repeated uncaughtException events." ?

But uncaughtException IMO has correct behavior avoiding infinite recursion. The same should have unhandledRejection.

[apapirovski]

@eduardbcom Since unhandledRejection happens on nextTick, there's no actual infinite loop. The other code in your app can still run.

[eduardbme]

@apapirovski

By saying infinite recursion I mean that we infinitely entering that function. And there is a case when no another code would be run cuz user's micro function can completely fill up promise micro stack.

[eduardbme]

As a plus sign, I don't see any advantage in current implementation.
If after unhandledRejection callback execution you have one another unhandledRejection there is a chance (a big change) that you will have it infinitely. And as a result side effect (infinite functions call, whatever you call it).

Any reason to love this behavior ?

@apapirovski that was not working code :)

[apapirovski]

@eduardbcom After looking at the code, I don't think the current behaviour is intended. Pretty sure it was meant to let the event loop proceed first.

[eduardbme]

@apapirovski but you also don't like the idea about process.exit on the second sequential call ?

don't get me wrong, its okay to emit unhandledRejection several times during program execution, but IMO it's not okay to emit this event after processing callback for that event..

[apapirovski]

@eduardbcom I think that we should conform to the expected async behaviour of promises. The problem is that the current code that emits unhandledRejection inadvertently creates an infinite loop — but it probably shouldn't.

@bnoordhuis I think we need to at least allow the nextTick loop to proceed but I could see an argument for even allowing the nextTick loop to finish and only invoke any new promise rejection on the next run of _tickCallback. Thoughts? This is the problematic code:

node/lib/internal/process/promises.js

Lines 100 to 116 in 4117e22

	function emitPendingUnhandledRejections() {
	let hadListeners = false;
	while (pendingUnhandledRejections.length > 0) {
	const promise = pendingUnhandledRejections.shift();
	const reason = pendingUnhandledRejections.shift();
	if (hasBeenNotifiedProperty.get(promise) === false) {
	hasBeenNotifiedProperty.set(promise, true);
	const uid = promiseToGuidProperty.get(promise);
	if (!process.emit('unhandledRejection', reason, promise)) {
	emitWarning(uid, reason);
	} else {
	hadListeners = true;
	}
	}
	}
	return hadListeners;
	}

As you can see, the pendingUnhandledRejections can just get refilled with new rejections while we're iterating over it. That seems problematic to me.

[eduardbme]

Can fix it, but we need to reach an agreement of the expected behavior.

[bnoordhuis]

Something like this is acceptable, IMO:

const rejections = pendingUnhandledRejections;
pendingUnhandledRejections = [];
while (rejections.length > 0) {
  // ...
}

(And might as well change to that a for loop for efficiency:)

for (let i = 0; i < rejections.length; i += 2) {
  const promise = rejections[i + 0];
  const reason = rejections[i + 1];
  // ...
}

Anything more complex is probably not worth it because it's an edge case and the plan is to make unhandled rejections fatal errors anyway.

[eduardbme]

the plan is to make unhandled rejections fatal errors anyway

Even in the case we listen that event ?

[mcollina]

I think the expected behavior is for them to be fatal. However, it is very hard to reach an agreement on the matter. I have a polyfill for that behavior if you want to try that out: https://github.com/mcollina/make-promises-safe.

I think this is too much of an edge case and a programmer error: I do not think it's Node.js responsibility to handle.