Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Process exits with unresolved promises #16

Open
BridgeAR opened this issue Feb 20, 2019 · 22 comments
Open

Process exits with unresolved promises #16

BridgeAR opened this issue Feb 20, 2019 · 22 comments

Comments

@BridgeAR
Copy link
Member

BridgeAR commented Feb 20, 2019

A lot of users struggle understanding why a Node.js process exits while a promise is still running not yet resolved (see e.g., nodejs/node#22088).

This is by design but the question is if it's really the best for most users. It definitely complicates debugging some cases.

I wonder if it makes sense to try to improve the situation for users by e.g., adding an opt-in to either

  • keep the process running if promises are still running unresolved (likely a bad idea)
  • provide a warning on process exit about still running unresolved promises
  • exit with a non zero exit code in case promises are running unresolved
  • tbd

Adding such an opt-in feature would likely help debugging never resolving promises and stream based promises where the stream does not receive any data.

Example:

// Never resolves
(async function fn() {
  await new Promise((resolve) => {});
  console.log('Will not be reached');
})();

Stream based example: nodejs/node#22088 (comment)

@nodejs/promises-debugging any opinions about this?

@devsnek
Copy link
Member

devsnek commented Feb 20, 2019

big terminology change: promises are never "running". promises don't do things. they have things done to them. to keep a good mental model I'd suggest saying unresolved or unsettled, depending on which one you mean.

first one makes no sense because it would just keep node alive forever, since nothing else is happening.

third one is also untenable, since it's absolutely a valid use of promises to not resolve one, and I suspect it happens a lot. an unresolved promise is basically an uncalled callback.

I would, however, be interested in working on a --trace-unresolved--promises option, given that the output wouldn't be too spammy in large projects. We will probably need to collect more data on how often promises are left unresolved.

@bergus
Copy link

bergus commented Feb 24, 2019

The warning shouldn't happen when the process exits, but when the resolver functions of the unresolved promise are getting garbage collected.
But I agree with @devsnek that this should not be a standard warning, never resolving a promise is an absolutely valid use case.

@mcollina
Copy link
Member

I 100% agree with @devsnek.

As a side note, I've been haunted by a "cb not called" issue in NPM for 8 months.

@devsnek
Copy link
Member

devsnek commented Feb 24, 2019

@mcollina same lmao

@mhdawson
Copy link
Member

I mentioned in another thread I'd be in favor of a --strict option that turns on options like this.

@dfoverdx
Copy link

dfoverdx commented Jan 8, 2020

Out of curiosity, what is a use case where you would have an unresolved promise and not want the process to live indefinitely?

@ljharb
Copy link
Member

ljharb commented Jan 8, 2020

@dfoverdx i don't think doing new Promise(() => {}) anywhere in an app should make it unexitable.

@dfoverdx
Copy link

dfoverdx commented Jan 8, 2020

@ljharb When would you write that code except to make the app unexitable? How is writing that code any different in intention than writing setInterval(() => {}, Number.MAX_VALUE)?

@ljharb
Copy link
Member

ljharb commented Jan 8, 2020

setInterval is an explicit instruction to do something - run the function forever. As is mentioned in the second comment, Promises do not do anything - they're a placeholder for the future value of work that has already begun. In my example, I've not done any work, and I've omitted notifying the Promise constructor that it's completed or errored - so there's nothing for the process to wait for.

@dfoverdx
Copy link

dfoverdx commented Jan 8, 2020

new Promise(() => {}).then(() => console.log('foo')) is also an explicit instruction to do something. The something is to log 'foo' sometime in the future. I can maybe understand exiting if there is no chain attached to an unresolved promise, but in most cases where people run into this issue, there is a then.

@dfoverdx
Copy link

dfoverdx commented Jan 9, 2020

I just figured out why this must be the case.

let p1 = Promise.resolve(1),
    p2 = new Promise(() => {}).then(Promise.resolve(2)),
    p3 = Promise.race([p1, p2]).then(console.log);

p2 will never resolve, and p3's Promise.race() call does not cancel p2's then(). However, we would expect this program to print 1 and then exit.

@devsnek
Copy link
Member

devsnek commented Jan 9, 2020

more generally, promises are basically fancy callbacks. If you stop a node http server, even though there are event listeners set up to handle requests (like the callback in then()), you would except the process to exit, because nothing is keeping the process alive to call those event handlers (or call the .then() callbacks).

@deepal
Copy link

deepal commented Jan 15, 2020

I just noticed the following weird behaviour (at least weird for me) where the following code exits immediately without setting a timer (if the timer was set, the process wouldn't exit since it's refed).

const wait = () => new Promise((resolve) => setTimeout(resolve, 5000))

(async () => {
    await wait();
})()

The following code waits for 5 seconds before exiting.

const wait = () => new Promise((resolve) => setTimeout(resolve, 5000))

const something = (async () => {
    await wait();
})()

The only difference is that I assign the returned promise to a variable something (unused) in the second example. I'm not sure how to explain this scenario. 🤔

@jkrems
Copy link

jkrems commented Jan 15, 2020

@deepal Did you mix up the two samples? I think it should be reversed according to some local testing. Before I spoil the answer: Prettier or another auto-format tool is invaluable when writing JavaScript. This is what happens to your second code sample after prettier ran:

const wait = () =>
  new Promise(resolve => setTimeout(resolve, 5000))(async () => {
    return wait();
  })();

The answer is: Removing the assignment revealed a missing semicolon in the first line, changing your code to a noop. :)

@deepal
Copy link

deepal commented Jan 15, 2020

Yeah, I mixed up the samples earlier and corrected afterwards 😬. I then added semicolons and could see for myself what you just explained. Thanks a lot. 👏👏

@jedwards1211
Copy link

jedwards1211 commented Sep 29, 2020

I just ran into a bug in Archiver.js where it exits unexpectedly if you forget to finalize() the archive: archiverjs/node-archiver#457

It took me over an hour to realize that the process was exiting because the event loop became empty.

I wish there were a --debug-exit type flag I could run Node with that would make it explain what caused Node to exit. (I had initially suspected a process.exit() call was accidentally buried in some dependency).

And it seems like it would be possible for Node to print a warning if the event loop becomes empty while there's an unresolved promise (at least when a debug flag is turned on), which would be super helpful.

Imagine a novice dev, who doesn't know anything about the event loop, running into an issue like this...I don't think they would ever understand what's going on.

@devsnek
Copy link
Member

devsnek commented Sep 29, 2020

I use https://www.npmjs.com/package/wtfnode from time to time. Might be nice to integrate that into core.

@jedwards1211
Copy link

as far as I can tell wtfnode wouldn't have helped me debug that issue, but I guess I can try it out

@mildsunrise
Copy link
Member

Agree with @bergus that the problem can be generalized. It would be very useful to output a warning whenever the promise's resolver callbacks have been both GC'ed (or the event loop is done) so the user knows that promise will never settle. But yeah, never resolving a promise is actually valid, so...

@benjamingr
Copy link
Member

How often have we seen this issue? We can log unsettled promises on exit to stderr - but I'm really not sure we should.

@jedwards1211
Copy link

but I'm really not sure we should

With a switch though, not by default...

What would have helped me is a switch to log the cause of exit (e.g. event loop becoming empty) more than logging unresolved promises at exit.

@mildsunrise
Copy link
Member

(The event loop became empty) doesn't really tell you anything, because that's what always causes Node to exit (except for special cases like process.exit or unhandled errors). But (this part of the code died before resolving the promise) can help you fix the bug, IMHO

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

No branches or pull requests