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

Process exits with unresolved promises #16

Open

Description

@BridgeAR

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?

Activity

  1. devsnek commented on Feb 20, 2019

    @devsnek
    Member

    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.

  2. bergus commented on Feb 24, 2019

    @bergus

    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.

  3. mcollina commented on Feb 24, 2019

    @mcollina
    SponsorMember

    I 100% agree with @devsnek.

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

  4. devsnek commented on Feb 24, 2019

    @devsnek
    Member

    @mcollina same lmao

  5. mhdawson commented on Feb 28, 2019

    @mhdawson
    Member

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

  6. dfoverdx commented on Jan 8, 2020

    @dfoverdx

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

  7. ljharb commented on Jan 8, 2020

    @ljharb
    SponsorMember

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

  8. dfoverdx commented on Jan 8, 2020

    @dfoverdx

    @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)?

  9. ljharb commented on Jan 8, 2020

    @ljharb
    SponsorMember

    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.

  10. dfoverdx commented on Jan 8, 2020

    @dfoverdx

    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.

  11. dfoverdx commented on Jan 9, 2020

    @dfoverdx

    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.

  12. devsnek commented on Jan 9, 2020

    @devsnek
    Member

    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).

  13. deepal commented on Jan 15, 2020

    @deepal

    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. 🤔

  14. hybrist commented on Jan 15, 2020

    @hybrist

    @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. :)

  15. deepal commented on Jan 15, 2020

    @deepal

    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. 👏👏

  16. jedwards1211 commented on Sep 29, 2020

    @jedwards1211

    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.

  17. devsnek commented on Sep 29, 2020

    @devsnek
    Member

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

  18. jedwards1211 commented on Sep 29, 2020

    @jedwards1211

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

  19. mildsunrise commented on Oct 1, 2020

    @mildsunrise

    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...

  20. benjamingr commented on Oct 1, 2020

    @benjamingr
    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.

  21. jedwards1211 commented on Oct 1, 2020

    @jedwards1211

    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.

  22. mildsunrise commented on Oct 1, 2020

    @mildsunrise

    (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 join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions