nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.65k stars 29.08k forks source link

Nodejs does not wait for promise resolution - exits instead #22088

Closed ddeath closed 6 years ago

ddeath commented 6 years ago

The code bellow will end with output:

Looping
before
working

and then it ends. The expected output is:

Looping
before
working

and the node should not exit

function sleep(ms) {
    return new Promise((resolve) => { setTimeout(resolve, ms) })
}

class TestClass {
    async processData(data) {
        return new Promise((resolver, rejecter) => {
            console.log('working')
        })
    }
}

class RunnerClass {

    async start() {
        const instance = new TestClass()
        while (true) {
            console.log('Looping')
            if (true) {
                try {
                    console.log('before')
                    const processedData = await instance.processData('data')
                    console.log('after')
                } catch (error) {
                    console.log('errored')
                }
            }
            await sleep(1000)
        }
    }
}

async function run() {
    const rebalancer = new RunnerClass()
    await rebalancer.start()
    console.log('end')
}

run()

If the if statment is set to if (false) then the process is outputting:

Looping
Looping
Looping
...

and never ends...

vsemozhetbyt commented 6 years ago

If I am not mistaken, Node.js does not wait for ever-pending Promises:

'use strict';

(async function main() {
  await new Promise((resolve) => { console.log(42); });
})();
42
[exit]
vsemozhetbyt commented 6 years ago

Refs: https://stackoverflow.com/questions/46914025/node-exits-without-error-and-doesnt-await-promise-event-callback https://stackoverflow.com/questions/46966890/what-happens-when-a-promise-never-resolves

jiripospisil commented 6 years ago

If I am not mistaken, Node.js does not wait for ever-pending Promises

In other words, the mere existence of a Promise won't keep the process alive. You actually need to put something on the event loop (e.g. create a timer in the processData method).

By the way, even if the process stayed alive because of some other I/O, I think the expected output would be

Looping
before
working

because the promise Executor is invoked imediately.

apapirovski commented 6 years ago

The answers above are correct. Going to close this out.

ddeath commented 6 years ago

@jiripospisil yes you are right, I edited issue. Of course that working is expected too.

@apapirovski but why the node end the process in the middle of nowhere? It has other code to execute after that await.

There is console.log('end') on the end. In this case I would expect that the process will throw en error or exits with some kind of error.

ddeath commented 6 years ago

For example:

'use strict';

(async function main() {
  await new Promise((resolve) => { console.log(42); });
  console.log(`
    This will never be executed is this expected?
    Also node exits as everything is ok with exit code 0...
  `);
})();
jiripospisil commented 6 years ago

I agree it's a bit weird when you encounter it for the first time (and I have certainly been bitten by this in the past) but the way to think about it is that when you await something, V8 jumps out of the current function (I believe it's implemented as syntax sugar on top of generators, or at least that's the high level idea). At this point Node.js doesn't care about the function anymore and checks whether there are some pending tasks on the event loop. If not, it exits. If you did this instead:

await new Promise((resolve) => { console.log(42); setTimeout(resolve, 2000) });

then Node.js would see there's a timer and stayed alive. When the timer goes off, Node.js invokes the callback and V8 jumps back to the place from which it jumped out off and starts executing the rest of the function.

Dzenly commented 5 years ago

Maybe we could have warning: "Process is exited, but you have pending promises, created on the file:xxx:yyy". But of course if will require some Promise manager and must be optional, just for debug.

tbranyen commented 5 years ago

This bit me pretty hard and has been extremely difficult to understand the behavior implications. I have a bundler which operates in a giant async Promise chain. Once a task completes it moves on to the next. I've added the ability for "loaders" to pre-process an AST and wanted to use a browserify plugin. I wrote the following code:

      transform: async ({ state, source }) => {
        return new Promise((resolve, reject) => {
          const stream = combynify(state.path, {});

          stream.on('data', console.log);

          stream.resume();
          stream.on('finish', function(d) {
            resolve(d);
          });

          stream.on('error', err => reject(err));
        });

This is injected into the Promise chain. Since I'm doing something wrong in this code, there is nothing on the event loop and immediately after this code executes, my bundler dies. This seems incredibly bad for an end user. How would they know what was wrong when they write such a loader? My bundler exits with no errors, exit code 0, and exits in the middle of this giant promise chain.

Should it be my bundlers responsibility to set a huge long timer before any loader, and once the loader is done, call clearTimeout?

tbranyen commented 5 years ago

This is how I've been able to "solve" the problem from my bundler's perspective:

    if (loaderChain.length) {
      const warningTimer = setTimeout(() => {
        console.log('Loader has not resolved within 5 seconds');
      }, 5000);
      const timer = setTimeout(() => {}, 999999);

      code = await handleTransform({ code, loaderChain });

      clearTimeout(timer);
      clearTimeout(warningTimer);
    }
devsnek commented 5 years ago

@tbranyen as long as the stream you wrap is doing things it will work. if the stream absolutely stops but doesn't end or error, nothing will be keeping the process open anymore.

tbranyen commented 5 years ago

@devsnek I believe I covered that in my initial comment:

Since I'm doing something wrong in this code, there is nothing on the event loop and immediately after this code executes, my bundler dies.

The point here is that something was wrong, and given that everything in the chain was a Promise which either resolves, rejects, or hangs, this behavior feels out-of-place. If you were using my tool and there was no indication that the loader was successful or not (since it never resolves or rejects), and the tool simply dies with a non-error exit code, that's pretty bad for debugging. In a browser, Promises can just hang indefinitely since there is no "process" to close in the web page. In Node, I can see how this behavior is ambiguous since there is a distinction made in Node as to whether v8 or libuv scheduled events determine if the Node process is out of work or not.

axkibe commented 4 years ago

conceptionally speaking this is not good. IMO node should definitely wait until all promises resolved/rejected.

I guess it is this way, because it is very difficult to implement? As promises are a V8 internal thing and the node.js mother process doesn't know about them?

bnoordhuis commented 4 years ago

@axkibe It's possible, see https://github.com/nodejs/node/issues/29355#issuecomment-525935873 for a PoC.

technicallyfeasible commented 4 years ago

I just got bitten by this as well and it makes sense. I was able to solve it with a dummy timeout of 0 duration:

  await new Promise(resolve => {
    setTimeout(() => null, 0);
  })

This actually waits indefinitely or until I call the resolve function.

Edit: I was wrong, the dummy timeout with 0 duration does not keep it alive.

nabilfreeman commented 4 years ago

Also pretty sure I am experiencing this while running 80 or so child processes in a promise pool, 4 at a time in parallel. After getting to about 60 processes, or 2 minutes in, the script just exits.

I'm going to try putting a massive timeout before running the promise pool and see what happens.

setTimeout(() => {}, Number.MAX_SAFE_INTEGER);

Edit: the above didn't work, but I will share what did.

Node doesn't wait for this...

async () => {
    // some random awaits and stuff before here...

    return new Promise((resolve, reject) => {
        // fork() and more child process things in here
    });
}

However if you don't return the promise and instead resolve it with await, it will consistently stay alive:

const keepalive = await new Promise((resolve, reject) => {
    ...
}

return keepalive;

Very interesting engine quirk.

max-hk commented 4 years ago

Just get into this issue. It is frustrated to debug without an message printed to console, telling the programmer that the progress is exited without an ever-pending Promise being resolved.

A warning message printed to the console would have saved my day.

craigphicks commented 3 years ago

Just get into this issue. It is frustrated to debug without an message printed to console, telling the programmer that the progress is exited without an ever-pending Promise being resolved.

A warning message printed to the console would have saved my day.

There is the 'beforeExit' event described in node docs and called with process.on('beforeExit',...) -

mayfield commented 3 years ago

This is obviously an issue for a LOT of people, including myself today. There should be a standard and intuitive pattern (e.g. doesn't require explanation of V8 internals during a code review) or builtin that facilitates safe exec of async main() {}.

As it stands now, using setTimout hacks is a great source of headache for every code review. And when you step away from node programming for a few months and come back you'll have to dig out your notebook for that "don't exit before you're done" hack.

Python's asyncio.run does a nice job of handling this exact scenario..

https://docs.python.org/3/library/asyncio-task.html#running-an-asyncio-program

craigphicks commented 3 years ago

To understand this topic we need to understand what "deadlock" is, and how it applies to javascript. In javascript their are multiple fibers running, and they may await on Promises (or events) which are resolved by other fibers. In a self contained program, with no possibility of external events resolving any awaiting fibers, if all the fibers are waiting then the program is "deadlocked", and it's state cannot change. The is equivalent to "when Node.js empties its event loop and has no additional work to schedule". Under those circumstances the best thing for the program to do is exit, otherwise it would have to killed at the process level.

The reason for deadlock is always design and/or programmer error. At least the probability of any other reason is so small that it is not worth persueing. Instead - redesign and/or solve the programmer error.

We can set a callback with process.on('beforeExit', beforeExitCallback) to let us know when deadlock has occurred:

function beforeExitCallback(){
   if (typeof process.exitCode=='undefined') {
      console.log('exiting with deadlock');
      process.exitCode=2;  // anything but 0 because we failed
   }
}

Meanwhile, we set process.exitCode at all "normal" exit points, for example:

main().then(()=>{process.exitCode=0;})
.catch((e)=>{console.error(e.message);process.exitCode=1;});
axkibe commented 3 years ago

To understand this topic we need to understand what "deadlock" is, and how it applies to javascript. In javascript their are multiple fibers running, and they may await on Promises (or events) which are resolved by other fibers. In a self contained program, with no possibility of external events resolving any awaiting fibers, if all the fibers are waiting then the program is "deadlocked", and it's state cannot change. The is equivalent to "when Node.js empties its event loop and has no additional work to schedule". Under those circumstances the best thing for the program to do is exit, otherwise it would have to killed at the process level.

The reason for deadlock is always design and/or programmer error. At least the probability of any other reason is so small that it is not worth persueing. Instead - redesign and/or solve the programmer error.

We can set a callback with process.on('beforeExit', beforeExitCallback) to let us know when deadlock has occurred:

function beforeExitCallback(){
   if (typeof process.exitCode=='undefined') {
      console.log('exiting with deadlock');
      process.exitCode=2;  // anything but 0 because we failed
   }
}

Meanwhile, we set process.exitCode at all "normal" exit points, for example:

main().then(()=>{process.exitCode=0;})
.catch((e)=>{console.error(e.message);process.exitCode=1;});

This has all little to do with the issue at hand. There are some promise that can resolve the node core isn't aware of (in my case as far I can remember it has something to do console input, or it was some other library which control IO was a promise. it's long ago, don't ask me for details) , yet it kills if there are only promises left. As you can see above people in this situations create dummy I/O for the node core to not quit, as did I.

Yes if there are only promises left whose resolve calls depend on each other, it's a dead lock and a coding error. IMO the node core doesn't need to keep track of that, an coding error is a coding error. And as far I recall in this case it used to exit without error message and exit code 0, which is certainly what no one wants. If you think you can never miss any callback from an extensions library ever then by all means exit with an approperiate error message please.

JonathanGawrych commented 3 years ago

I'm going to try putting a massive timeout before running the promise pool and see what happens.

setTimeout(() => {}, Number.MAX_SAFE_INTEGER);

Edit: the above didn't work...

This doesn't work because Number.MAX_SAFE_INTEGER is 2^53 − 1 (9007199254740991) and setTimeout only allows a 32-bit integer. (you should see a warning in the console (node:36587) TimeoutOverflowWarning: 9007199254740991 does not fit into a 32-bit signed integer.. Interestingly, I tried returning a promise like you did, but it didn't work.

I used this: (function runUntilExit() { setTimeout(runUntilExit, ~0>>>1) })(); The last bit sets it to the maximum signed int using bitwise operators, and it'll just call itself after the max timeout.

targos commented 3 years ago

What kind of operations are you using that force you to keep the process alive with setTimeout ? Maybe I'm wrong but I would say that unless you are working with a native C++ addon that works independently of the event loop, there's a bug somewhere in core.

axkibe commented 3 years ago

OK, I digged up my old code when this was an issue and the great gist-hack from ben (above) solved it back then. This isn't active code base for me anymore, since I changed some bigger things an that whole branch got cut off anyway. (It was related to piping a readStream through a json parser and writing async handlers)

I cannot reproduce it anymore with my current node (14.13) so whatever it was, that made node exit prematurely back then, I think it has been fixed by now.

And from the original post, yes that was a coding error..

RepComm commented 3 years ago

kind of makes node unusable in one of my use cases. every day someone else's code in the way. always for no reason. not even surprised at this point.

craigphicks commented 3 years ago

Yes if there are only promises left whose resolve calls depend on each other, it's a dead lock and a coding error. IMO the node core doesn't need to keep track of that, an coding error is a coding error. And as far I recall in this case it used to exit without error message and exit code 0, which is certainly what no one wants. If you think you can never miss any callback from an extensions library ever then by all means exit with an approperiate error message please.

In times past I created a simple program with a event listener registered for standard input. The program didn't exit. I concluded that NodeJS does check whether event listeners waiting for external events, and does not exit the program if they are ready and waiting. If you have an actual example where that mechanism is not working that might be a NodeJs bug or at least limitation. (Anecdotes don't count - it has to be testable code!)

craigphicks commented 3 years ago

kind of makes node unusable in one of my use cases. every day someone else's code in the way. always for no reason. not even surprised at this point.

A program can await promises where said promises are only resolved in conditional branches that are not guaranteed to run. It would be a huge overhead to require the programmer to resolve each of those promises before the program were allowed to exit.

jan-swiecki commented 3 years ago

This should never happen without any warnings. Extremely hard to debug in complex application. Please add warnings in future node releases.

My workaround:

async function main(argv) {
  // set exit code 1 on unhandled promise errors
  // https://stackoverflow.com/a/63509086/1637178
  process.exitCode = 1;

  /**
   * Node process exits with exitCode==0 when there are still
   * promises awaiting. We prevent this from happening by adding
   * event-loop timer and clearing it after code finishes.
   */
  const i = setInterval(() => {
    /* do nothing but prevent node process from exiting */
  }, 1000);

  try {
    await runApplicationLogic();
  } finally {
    clearInterval(i);
  }

  // revert back to correct status code, because we didn't encounter any errors
  process.exitCode = 0;
}

main(process.argv);
boutell commented 2 years ago

This is resolved for applications that use top-level await. This requires that your program be written as an ES module rather than using require, but that's not uncommon these days. As someone still using require, I guess setting up the setTimeout hack is My Problem.

boutell commented 2 years ago

(Top-level await is available without a flag starting in Node 14.8.)

boutell commented 2 years ago

Example that works as long as it's test.mjs rather than test.js:

const asyncMsg = await go();

console.log(asyncMsg);

function go() {
  return new Promise((resolve, reject) => {
    setTimeout(() => resolve('WORKS!'), 1000);
  });
}
tomsaleeba commented 2 years ago

Here's a back-to-back demo showing that when you use NodeJS modules (.mjs files) and top-level await, you at least get a non-zero return code:

First, the old way:

 # non-module syntax test
cat <<"HEREDOC" | docker run --rm -i --entrypoint sh node:17.6-alpine && echo "zero return code"
f=/tmp/blah.js
echo ";(async function () {" >> $f
echo "  await new Promise(r => console.log(42))" >> $f
echo "  console.log('hello')" >> $f
echo "})()" >> $f
cat $f
node $f
HEREDOC

...and that gives us:

42
zero return code

Now we'll test on the latest Node, using modules (important!) and top-level await:

 # top level await in module test
cat <<"HEREDOC" | docker run --rm -i --entrypoint sh node:17.6-alpine && echo "zero return code"
f=/tmp/blah.mjs
echo "async function blah() {" >> $f
echo "  await new Promise(r => console.log(42))" >> $f
echo "  console.log('hello')" >> $f
echo "}" >> $f
echo "await blah()" >> $f
cat $f
node $f
HEREDOC

...and that gives us

42

...and then a non-zero return code :tada:, specifically 13

zackw commented 1 year ago

cat <<"HEREDOC" | docker run --rm -i --entrypoint sh node:17.6-alpine && echo "zero return code" f=/tmp/blah.mjs echo "async function blah() {" >> $f echo " await new Promise(r => console.log(42))" >> $f echo " console.log('hello')" >> $f echo "}" >> $f echo "await blah()" >> $f cat $f node $f HEREDOC

Note a critical subtlety in both versions of your code:

async function blah() {
  await new Promise(r => console.log(42));
  console.log('hello');
}
await blah();

blah() is awaiting a promise that will never resolve. You dropped the resolution callback on the floor (intentionally? I'm a little confused about all the different cases thrown around in this issue)

This slight variant

async function blah() {
  await new Promise((r) => { console.log(42); r(); });
  console.log('hello');
}
await blah();

prints "42\nhello\n" and exits successfully (for me, with node v18.12).

I think it would be better if the first version of the program hung forever instead of exiting (regardless of exit code) but what I, personally, care about more is: Is it guaranteed that version 2 of the program will print hello before exiting?

tbranyen commented 1 year ago

cat <<"HEREDOC" | docker run --rm -i --entrypoint sh node:17.6-alpine && echo "zero return code" f=/tmp/blah.mjs echo "async function blah() {" >> $f echo " await new Promise(r => console.log(42))" >> $f echo " console.log('hello')" >> $f echo "}" >> $f echo "await blah()" >> $f cat $f node $f HEREDOC

Note a critical subtlety in both versions of your code:

async function blah() {
  await new Promise(r => console.log(42));
  console.log('hello');
}
await blah();

blah() is awaiting a promise that will never resolve. You dropped the resolution callback on the floor (intentionally? I'm a little confused about all the different cases thrown around in this issue)

This slight variant

async function blah() {
  await new Promise((r) => { console.log(42); r(); });
  console.log('hello');
}
await blah();

prints "42\nhello\n" and exits successfully (for me, with node v18.12).

I think it would be better if the first version of the program hung forever instead of exiting (regardless of exit code) but what I, personally, care about more is: Is it guaranteed that version 2 of the program will print hello before exiting?

The point of this issue is the first case. You don't always have control over what promises are in the chain (think middleware). Someone passes in a promise that isn't properly resolved, now the node process completely dies with no indication why. Sure the fix is obvious in your case, but if you have 50 middleware in a massive application, this could be completely dumbfounding.

axkibe commented 1 year ago

Anyway, as long nothing listens anymore (including all the middlewares) to any network event, file event, timer event etc. and there is nothing in the "nextTick" stack either etc. and the code passes control back to the event loop.. other than dying would be halting/zombing forever.

AFAIK, there was a rare bug in some cases, where node died, albeit there was still something incoming from a stream.. but it has been fixed.

tbranyen commented 1 year ago

Anyway, as long nothing listens anymore (including all the middlewares) to any network event, file event, timer event etc. and there is nothing in the "nextTick" stack either etc. and the code passes control back to the event loop.. other than dying would be halting/zombing forever.

AFAIK, there was a rare bug in some cases, where node died, albeit there was still something incoming from a stream.. but it has been fixed.

This is precisely what happens in the browser and allows you to easily inspect the stack and current runtime, not sure why this would be controversial for Node on the command line. If you were to try and use --inspect the whole process would die before you even knew where to start.

axkibe commented 1 year ago

Does it really close while you have an --inspect debugger enabled, because in that case it shouldn't, since its an event source (debugger attaches). If it does, you should file a bug. If it doesn't, its IMO fine. As said, otherwise it would be a zombie process.

bnoordhuis commented 1 year ago

I think @tbranyen is saying JS in the browser stays around so you can attach a debugger.

Of course node is not a browser. A --hang-around-as-long-as-there-are-unresolved-promises flag is conceivable for debugging (because that's all that it's good for) but not as the default behavior.

I'm not going to reopen this issue - too much meandering discussion - but if someone wants to open a new issue and back-link to this one, go ahead.

axkibe commented 1 year ago

And generally speaking, I can see a point for having node hanging around for a debugger to connect even after it is finished in conventional sense .. don't need a flag for promises for this, generally speaking inspecting data structures even after it's done.

On the other hand, this is easy to workaround, just make a dummy listener to any port and it will keep running.

With this on the head of the root file, @tbranyen should get what they want. if(require('inspector').url()) require('net').createServer().listen(9999);

I agree, I don't see any issue as of today.

benjamingr commented 1 year ago

We ran into this with the test runner today. I think we should revisit our choice to end the process with promises pending.

In our case - we had a test that spawned a child process and transformed its stdout. Its stdout was transformed with async code which meant the parent process exited and did not wait to validate the output.

This means code like

const result = await fs.createReadStream("./file.txt").map(async (x) => { return await transformAsync(x); }).toArray();
// code below never runs, because nothing is keeping the process alive between the stream being finished 
// and the transforms/toArray which run after a microtick.
doSomethingWith(result);  
axkibe commented 1 year ago

benajmingr, your code seems faulty, since doSomething() is executed right away, there is nothing waiting for the stream you created to finish. And "result" is a promise you need to wait somewhere for.


async function transformAsync( x )
{
    return '(' + x + ')';
}

async function run( )
{
    const result = fs.createReadStream("./file.txt").map(async (x) => { return await transformAsync(x); }).toArray( );
    console.log( 'dosomthing', await result );
}

run( );

Works fine.

benjamingr commented 1 year ago

Yeah the "this code doesn't run" bit was obviously missing an await :)

josbert-m commented 4 months ago

I know this discussion is closed, but I'm working on a Nest.js application and creating a custom ConfigModule similar to the one provided by the framework, as I need additional custom logic. I ran into this issue when trying to resolve a Promise from outside of its scope.

The strange thing is, in the existing Nest.js ConfigModule, there's a similar Promise that does work:

https://github.com/nestjs/config/blob/bee951799db96c7778c1c33914ad88202bac8768/lib/config.module.ts#L47

Does anyone know why it works there? I'm very confused.