Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node incorrectly throws ERR_UNHANDLED_REJECTION on error caught in Promise.catch #43326

Open
trusktr opened this issue Jun 6, 2022 · 22 comments

Comments

@trusktr
Copy link
Contributor

trusktr commented Jun 6, 2022

Version

17.3.0

Platform

Linux

Subsystem

internal/process/promises

What steps will reproduce the bug?

Make a project folder.

Create package.json:

{
    "type": "module"
}

Paste this code into a file test.js:

export function testPromiseCatch() {
	const p2 = new Promise((resolve, reject) => {
		setTimeout(() => {
			const e = {message: 'rejected', name: 'Error'}
			// const e = new Error('rejected')
			reject(e)
		}, 2000)
	})

	p2.then(n => {
		throw new Error('then() should not run.')
	})

	// .catch(e => {
	p2.catch(e => {
		if (e.message != 'rejected') throw new Error('It should have rejected with the correct error')
		console.log('ERROR WAS CAUGHT!')
	})

	console.log('Created promise, wait for result...')
}

testPromiseCatch()

Run the file, then you'll see this output:

$ node test.js
Created promise, wait for result...
ERROR WAS CAUGHT!
node:internal/process/promises:246
          triggerUncaughtException(err, true /* fromPromise */);
          ^

[UnhandledPromiseRejection: 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(). The promise rejected with the reason "#<Object>".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

Node.js v17.3.0

Note the output contains ERROR WAS CAUGHT! which shows that the error was handled by a .catch callback, yet Node.js still exits non-zero with ERR_UNHANDLED_REJECTION.

Note that if you comment out the line const e = {message: "rejected", name: "Error"} and uncomment const e = new Error("rejected"), the error becomes more obscure:

$ node test.js
Created promise, wait for result...
ERROR WAS CAUGHT!
file:///home/trusktr/tmp/test.js:5
                        const e = new Error('rejected')
                                  ^

Error: rejected
    at Timeout._onTimeout (file:///home/trusktr/tmp/test.js:5:14)
    at listOnTimeout (node:internal/timers:568:17)
    at processTimers (node:internal/timers:510:7)

Node.js v17.3.0

Finally, note that if you comment p2.catch((e) => { and uncomment .catch((e) => { so that the .catch callback is chained on the .then promise, the problem does not happen and the output is:

$ node test.js
Created promise, wait for result...
ERROR WAS CAUGHT!

with exit code 0.

How often does it reproduce? Is there a required condition?

Every time when .catch is not chained on .then.

What is the expected behavior?

Should Node exit with zero, because the error was caught?

What do you see instead?

1

Additional information

No response

@trusktr
Copy link
Contributor Author

trusktr commented Jun 6, 2022

How often does it reproduce? Is there a required condition?

Every time when .catch is not chained on .then.

Nevermind, it happens with chained .catch callbacks too. Here's another example to try:

// test2.js
export function testPromiseCatchFinally() {
	const p2 = new Promise((resolve, reject) => {
		setTimeout(() => {
			const e = new Error('rejected2')
			reject(e)
		}, 2000)
	})

	p2.then(n => {
		throw new Error('then() should not run.')
	}).catch(e => {
		if (e.message != 'rejected2') throw new Error('It should have rejected with the correct error')
		console.log('ERROR CAUGHT')
	})

	p2.finally(() => {
		console.log('FINALLY')
	})
}

testPromiseCatchFinally()

Output:

$ node test2.js 
FINALLY
ERROR CAUGHT
file:///home/trusktr/tmp/test2.js:4
                        const e = new Error('rejected2')
                                  ^

Error: rejected2
    at Timeout._onTimeout (file:///home/trusktr/tmp/test2.js:4:14)
    at listOnTimeout (node:internal/timers:568:17)
    at processTimers (node:internal/timers:510:7)

Node.js v17.3.0

Note that commenting out the p2.finally() block works:

export function testPromiseCatchFinally() {
	const p2 = new Promise((resolve, reject) => {
		setTimeout(() => {
			const e = new Error('rejected2')
			reject(e)
		}, 2000)
	})

	p2.then(n => {
		throw new Error('then() should not run.')
	}).catch(e => {
		if (e.message != 'rejected2') throw new Error('It should have rejected with the correct error')
		console.log('ERROR CAUGHT')
	})

	// p2.finally(() => {
	// 	console.log('FINALLY')
	// })
}

testPromiseCatchFinally()

Output:

$ node test2.js 
ERROR CAUGHT

Chaining the .finally also works:

export function testPromiseCatchFinally() {
	const p2 = new Promise((resolve, reject) => {
		setTimeout(() => {
			const e = new Error('rejected2')
			reject(e)
		}, 2000)
	})

	p2.then(n => {
		throw new Error('then() should not run.')
	})
		.catch(e => {
			if (e.message != 'rejected2') throw new Error('It should have rejected with the correct error')
			console.log('ERROR CAUGHT')
		})
		.finally(() => {
			console.log('FINALLY')
		})
}

testPromiseCatchFinally()

Output:

$ node test2.js
ERROR CAUGHT
FINALLY

@bnoordhuis
Copy link
Member

Can you check with a newer node version? Also, can you confirm/deny whether your test case is equivalent to this:

const p = new Promise((_, reject) => {
  setImmediate(() => reject(new Error("err")))
})
p.catch(console.error)

echo $? prints 0 when I run that with v18.2.0.

@benjamingr
Copy link
Member

I'm trying to understand your expectation of why node shouldn't log an error here since there is a promise chain in your example without a catch attached and there is a swallowed error. When you do:

const p = Promise.reject(whatever);
p.then(someFn); // This is a promise chain where no one handles the error.
p.catch(handleError); // this doesn't handle the error for the p.then(someFn) chain 

You have a dangling unhandled error - can you elaborate on what you think would be better behavior?

@trusktr
Copy link
Contributor Author

trusktr commented Jun 11, 2022

Hmmm. The thing is that it is easy to write code this way that actually handles an error and that is totally valid within a private scope. If I am making my own private code, and the p.then(someFn) is not passed around to anyone, then I effectively have caught errors for my use case, despite Node.js saying otherwise. Is there some way improve Node.js for it to realize when such code is ok?

@jasnell
Copy link
Member

jasnell commented Jun 12, 2022

This is standard behavior and happens also in browsers using the unhandledrejection event. Essentially, this is working exactly as it was intended. The way to tell Node.js that such code is ok and expected it to attach a non-op catch handler or to handle the unhandledRejection event.

@benjamingr
Copy link
Member

@trusktr

Is there some way improve Node.js for it to realize when such code is ok?

In your particular case the code is not ok since .then(someFn) creates a rejected promise chain and the rejection isn't handled. You can either add the .catch on p instead of later (so you chain .then(someFn) after handleError) or explicitly suppress errors on it by doing p.then(someFn).catch(() => { /* suppress errors */ }).

This is akin to adding an empty catch { /* suppress errors */ } in synchronous code.

@devoidfury
Copy link

devoidfury commented Sep 20, 2022

I'm seeing a similar issue here, on node v18.7.0.

function log(label) {
  return (...args) => console.log(`[${label}]`, ...args);
}

function wait() {
  return new Promise((resolve) => setImmediate(() => resolve()));
}

function indirect() {
  log("inside indirect, before creating rejected promise")();
  const x = Promise.reject(new Error("An Error"));

  log("inside indirect, before wait")();
  return wait().then(() => {
    log("inside indirect, after wait")();
    return x;
  });
}

indirect().then(log("finished")).catch(log("outer catch"));

gives the following output and stops execution halfway through:

[devoidfury@oryx tmp]$ node example.mjs 
[inside indirect, before creating rejected promise]
[inside indirect, before wait]
file:///home/devoidfury/tmp/example.mjs:11
  const x = Promise.reject(new Error("An Error"));
                           ^

Error: An Error
    at indirect (file:///home/devoidfury/tmp/example.mjs:11:28)
    at file:///home/devoidfury/tmp/example.mjs:20:1
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:541:24)
    at async loadESM (node:internal/process/esm_loader:91:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)

Node.js v18.7.0

changing the type of the rejection reason to a string gives an ERR_UNHANDLED_REJECTION error instead; removing the Promise.reject function call and just returning the error directly as-is works as you'd expect.

$ node example.mjs 
[inside indirect, before creating rejected promise]
[inside indirect, before wait]
node:internal/process/promises:288
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[UnhandledPromiseRejection: 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(). The promise rejected with the reason "An Error".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

Node.js v18.7.0

@benjamingr
Copy link
Member

I'm seeing a similar issue here, on node v18.7.0.

Right and as in the original post this is intended behavior. Rejected promises need to be handled before I/O (like setImmedaiate) is processed or otherwise they're considered unhandled.

You can work around this by adding an empty catch handler to the promise (forking it, not handling the error)

@devoidfury
Copy link

Right, so just to make sure I'm clear there, the fix would look like this:

function wait() {
  return new Promise((resolve) => setTimeout(resolve, 0));
}

function test() {
  const x = Promise.reject(new Error("this should be caught"));
  x.catch(() => {});
  return wait().then(() => x);
}

test().catch((e) => {
  console.log("caught error", e);
});

That's super counter-intuitive and surprising for me, and I've been writing javascript a long time. Is there some related documentation as to why this is? Some note about this quirk in the documentation anywhere? And why the traceback doesn't say "unhandled rejection" unless you use a string instead of an Error reason?

@BridgeAR
Copy link
Member

@devoidfury the reason is that it's not possible to know if and when the handler is actually attached. Attaching the handler could in fact fail as well. Imagine wait() would reject. If that's the case, .then() would not be called where the rejected promise is returned and the x promise would never be handled.

@devoidfury
Copy link

@BridgeAR okay, but this code has the same problem even though it handles an imaginary wait rejection.

function wait() {
  return new Promise((resolve) => setTimeout(resolve, 0));
}

function test() {
  const x = Promise.reject(new Error("this should be caught"));
  return wait().then(() => x).catch(() => x);
}

test().catch((e) => {
  console.log("caught error", e);
});
[devoidfury@oryx errors]$ node demo.js 
file:///home/devoidfury/projects/quest/errors/demo.js:6
  const x = Promise.reject(new Error("this should be caught"));
                           ^

Error: this should be caught
    at test (file:///home/devoidfury/projects/quest/errors/demo.js:6:28)
    at file:///home/devoidfury/projects/quest/errors/demo.js:12:1
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:541:24)
    at async loadESM (node:internal/process/esm_loader:91:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)

Node.js v18.7.0

Okay, I get it, it's a limitation in the engine's PromiseRejectionTracker (or whatever you call it) that intentionally crashes the process when it detects an unhandled rejection (right before doing IO, apparently).

Fine. All that said, at the very least, the error it spits out should indicate that the reason has to do with an unhandled rejection, in addition to just regurgitating the stack from whatever errror was rejected, no?

@BridgeAR
Copy link
Member

Yes, it is indeed a limitation in detecting such cases.

About telling the reason: it's an error, just as any other error in the application. We do not log extra information in case the process ends due to a sync error either.
The stack let's you know where the error is created. If you use async / await, it's not only going to likely resolve the issue for you (there would not be a run away promise in that case) there would also potentially be more useful stack frames included.

In case it's anything besides an error that is thrown, we attach further information for the user to know what happened as there is no way to directly know where the error came from.

@sdegueldre
Copy link

sdegueldre commented Sep 21, 2022

Maybe we could generate a new Error and set the reject reason as that error's cause? Though I can't quite remember if node handles error chains through the cause property gracefully as of right now so maybe that's not an option. I feel like getting a stack trace for where the Promise that was rejected without being handled was created is always useful information, even though when rejecting with an error the error also obviously contains useful error information. For what it's worth, chrome (and firefox) prefixes the error message with "Uncaught (in promise)", which may at least hint at the cause of the issue.

@devoidfury
Copy link

I'm seeing even worse behavior with the async/await syntax. What is even going on here?

[devoidfury@oryx errors]$ cat demo.js 
function wait() {
  return new Promise((resolve) => setTimeout(resolve, 0));
}

async function exampleWithError() {
  await wait();
  throw new Error("this should be caught");
}

async function test() {
  try {
    // use case: kick off an async thing we don't care right now what happens to it
    // ... except this is completely uncatchable
    const pending = exampleWithError();

    await wait(); // do the main thing we wanted first

    // wait for all pending tasks to finish
    await Promise.all([pending, wait()]);

    await wait(); // do other stuff
  } catch (e) {
    console.log("caught");
  }
}

test().catch((e) => {
  console.log("caught");
});
[devoidfury@oryx errors]$ node demo.js 
/home/devoidfury/projects/errors/demo.js:7
  throw new Error("this should be caught");
        ^

Error: this should be caught
    at exampleWithError (/home/devoidfury/projects/errors/demo.js:7:9)
    at runNextTicks (node:internal/process/task_queues:60:5)
    at listOnTimeout (node:internal/timers:533:9)
    at process.processTimers (node:internal/timers:507:7)

Node.js v18.7.0

@benjamingr
Copy link
Member

Right, so just to make sure I'm clear there, the fix would look like this:

Correct, in bluebird we had an explicit ".suppressUnhandledRejection" - as Ruben said Node has no way to know when a rejection will not be handled. If we could - we'd solve the halting problem.

That's super counter-intuitive and surprising for me, and I've been writing javascript a long time.

It is, we spent like 5 years debating this behavior and several competing proposals (like consider it rejected on GC) and this ended up being (by far) the least-surprising design. It is fundamentally a problem with the design of error handling in promises themselves and the alternatives (e.g. swallow errors, only err on GC etc) seemed worse.

It is often better not to "kick off" background work like that and instead have a dedicated "queueBackgroundWork" function that takes your functions you want to happen in the background and centralizes error handling.

@sdegueldre

Maybe we could generate a new Error and set the reject reason as that error's cause? Though I can't quite remember if node handles error chains through the cause property gracefully as of right now so maybe that's not an option. I feel like getting a stack trace for where the Promise that was rejected without being handled was created is always useful information,

In general we already do show where the error is (errors in V8 JavaScript capture their stack trace when they are created not thrown/rejected so the place the promise was rejected should typically be there. Can you provide an example of what we can do better?

@devoidfury

I'm seeing even worse behavior with the async/await syntax. What is even going on here?

What additional info would you expect in this case?

Note the code can be written to not leave dangling errors (e.g. do Promise.all immediately and put the logic you don't want to wait for in a then or async function whose call you pass to the .all alongside "pending").

@devoidfury
Copy link

devoidfury commented Sep 24, 2022

The problem I have with this is, unlike most other scenarios where an error pops up, is there's nothing in that stack trace you can search on the internet to get an answer.

I guarantee there are thousands of bugs out there in userland code caused by this, where the symptom is "it crashes here once in a while in production and I have no idea why but I've already spent a week trying to figure it out so we'll just have to live with it shrugs".

At least if it hinted that there was an "unhandled rejection", now we have some keywords we can search that might lead to an explanation of what's wrong with our code and how to fix it.

For example, any of these would be way more debug-able in my opinion:

Uncaught (in promise)
Error: this should be caught
    at exampleWithError (/home/devoidfury/projects/errors/demo.js:7:9)
    at runNextTicks (node:internal/process/task_queues:60:5)
    at listOnTimeout (node:internal/timers:533:9)
    at process.processTimers (node:internal/timers:507:7)
(unhandled rejection)
Error: this should be caught
    at exampleWithError (/home/devoidfury/projects/errors/demo.js:7:9)
    at runNextTicks (node:internal/process/task_queues:60:5)
    at listOnTimeout (node:internal/timers:533:9)
    at process.processTimers (node:internal/timers:507:7)
ERR_UNHANDLED_REJECTION: Error: this should be caught
    at exampleWithError (/home/devoidfury/projects/errors/demo.js:7:9)
    at runNextTicks (node:internal/process/task_queues:60:5)
    at listOnTimeout (node:internal/timers:533:9)
    at process.processTimers (node:internal/timers:507:7)

@benjamingr
Copy link
Member

So basically, ideally you'd want us to add some text if the process exits due to an unhandled rejection like we used to have to log before we exit?

@BridgeAR wdyt?

@devoidfury
Copy link

Yes, that's exactly right, just adding some important keywords to the stack trace to make it more informative, discoverable, searchable, learnable; having the location the problem occurred is great, but knowing the name of the problem is just as important.

@hitsthings
Copy link

Not sure if my beef is with TC39 or Node, but I hit this by having a finally() after the .catch().

That is:

async function tryButFail() {
  throw new Error('boom!')
}

const p = tryButFail();
p.catch(e => console.log('handled'));
p.finally(() => console.log('cleanup'));

Even though:

  1. the error in tryButFail() is handled
  2. there is no error in the finally

...I got an unhandled rejection.

I would have thought handling of the failure in tryButFail() would be recorded against p and thus p.finally() should only record an unhandled exception if the error is within the .finally() code - any previous chain errors were handled! Can you not traverse up the promise chain of the unhandled rejection to determine whether it occurred in a "handled" part of the chain or not?

As mentioned above by others, the other thing that made it hard to debug, is that the .finally() line was nowhere in any stack traces for the unhandled promise. The stack was for the error I threw and caught. I kept catching the same error in various places to no avail!

@benjamingr
Copy link
Member

benjamingr commented Aug 14, 2023

there is no error in the finally

That's the issue, p.finally(handler) creates a new promise which is unhandled:

const p = Promise.reject(new Error());
p.catch(...); // p will no longer have unhandled rejections
const p2 = p.finally(...);
// without p2.catch, p2 is a continuation of p (before the .catch) and thus will be rejected
// its rejection will be unhandled

As mentioned above by others, the other thing that made it hard to debug, is that the .finally() line was nowhere in any stack traces for the unhandled promise. The stack was for the error I threw and caught. I kept catching the same error in various places to no avail!v

I agree we should probably improve this.

To be explicit you currently get:

/Users/bgruenbaum/Desktop/example.js:2
  throw new Error('boom!')
        ^

Error: boom!
    at tryButFail (/Users/bgruenbaum/Desktop/example.js:2:9)
    at Object.<anonymous> (/Users/bgruenbaum/Desktop/example.js:5:11)
    at Module._compile (node:internal/modules/cjs/loader:1233:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1287:10)
    at Module.load (node:internal/modules/cjs/loader:1091:32)
    at Module._load (node:internal/modules/cjs/loader:938:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
    at node:internal/main/run_main_module:23:47

But would want to get:

/Users/bgruenbaum/Desktop/example.js:2
  throw new Error('boom!')
        ^

Error: boom!
    at tryButFail (/Users/bgruenbaum/Desktop/example.js:2:9)
    at Object.<anonymous> (/Users/bgruenbaum/Desktop/example.js:5:11)
    at Module._compile (node:internal/modules/cjs/loader:1233:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1287:10)
    at Module.load (node:internal/modules/cjs/loader:1091:32)
    at Module._load (node:internal/modules/cjs/loader:938:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
    at node:internal/main/run_main_module:23:47

From Promise:  
    at Object.<anonymous> (/Users/bgruenbaum/Desktop/example.js:10:7)
    at tryButFail (/Users/bgruenbaum/Desktop/example.js:2:9)
    at Object.<anonymous> (/Users/bgruenbaum/Desktop/example.js:5:11)
    at Module._compile (node:internal/modules/cjs/loader:1233:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1287:10)
    at Module.load (node:internal/modules/cjs/loader:1091:32)
    at Module._load (node:internal/modules/cjs/loader:938:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
    at node:internal/main/run_main_module:23:47

Where at Object.<anonymous> (/Users/bgruenbaum/Desktop/example.js:10:7) represents the line the promise was created (the .finally is)?

(I am not sure we can do that technically, at least without async hooks or a big performance penalty for the same reason V8 only creates stack traces for async functions - but I want to understand better + maybe with the inspector attached?)

@hitsthings
Copy link

Re: the stack trace. Yeah putting the finally() into the stack trace would be supreme. Where finally() represents the "leaf" promise. Only when inspector attached would be a very nice compromise there!

p.finally(handler) creates a new promise

My beef might not be with Node here as I dunno what is specced. But I feel like it doesn't have to be this way?
The new promise that is created has a chain. We know which promise it depends on via then(). And we know that the error from that promise has been caught - .catch() was called on it. I could see an implementation of catch() that adds the parent promise to a WeakMap, for example, marking it "caught". then() and finally() could pass that marking forward if their own callbacks don't throw, such that, by the time you get to the "leaf", either you still have the marking or you don't.

The distinction could then be made based on where the error actually occurred whether the rejection is handled. Perhaps there are similar performance reasons why you can't look back at the chain when an error occurs, though? Too many promises held?

And while I have no illusion my opinion here matters - I'm sure you all have thought long and hard about this stuff - it seems to me that there's a philosophical question. Do people care that each "leaf" promise has been caught? Or that each "original error" has been handled? The latter seems more useful to me - why is it important that I log the same error twice? But I'm sure there are legitimate reasons for both. Maybe one solution is that this "original error caught" distinction is passed to the unhandledrejection handler. Would have helped my sanity in dev, and I think I would not process.exit(1) in production if I knew a rejection was caught somewhere and handled (even if it wasn't handled everywhere)

@virenradadiya
Copy link

I encountered an unhandled promise rejection error when building my React-Native iOS project using Node.js. The error message points to a failure with xcodebuild, exiting with error code 65. Below is the error log:

node:internal/process/promises:392
      new UnhandledPromiseRejection(reason);
      ^

UnhandledPromiseRejection: 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(). The promise rejected with the reason "Error: Failed to build ios project. 'xcodebuild' exited with error code '65'. To debug build logs further, consider building your app with Xcode.app, by opening 'XXXX.xcworkspace'.".
    at throwUnhandledRejectionsMode (node:internal/process/promises:392:7)
    at processPromiseRejections (node:internal/process/promises:475:17)
    at process.processTicksAndRejections (node:internal/process/task_queues:106:32) {
  code: 'ERR_UNHANDLED_REJECTION'
}

Node.js v23.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants