Repository navigation
Promise Rejections delay process exiting (and others) in processPromiseRejections #34851
Description
Activity
After some investigation, this also appears to block some other types of operations, not just process exiting. I haven't been able to nail down an additional test case though, since this other processing that was blocked occurred in a 3rd party library which involves spinning up a server.
The code more or less goes:
// same loop as above console.log("done"); // call to 3rd party console.log("done 3rd party");Which results in the output:
done done 3rd party <massive delay> <3rd party operation completes>By sleeping for some time during
foo(), we can eliminate this massive delay since it gets spread out earlier. (I assume the loop handling the pendingUnhandledRejections doesn't fill up)
a close up of the process that starts afterprocessPromiseRejectionsfinishes:
- changed the title
[-]Promise Rejections delay process exiting in processPromiseRejections[/-][+]Promise Rejections delay process exiting (and others) in processPromiseRejections[/+]on Aug 20, 2020 My guess is this happens because we keep track of rejections to determine if they are unhandled or not, which means the program is looping through 100000 rejections in one tick (whereas on real world applications this is unlikely to happen). This is the relevant code btw:
.node/lib/internal/process/promises.js
Lines 190 to 260 in ff74e35
Looking at the complete flamegraph (for Node.js v12), we're making some very expensive calls:
There might be some room for optimization here.
I can try to improve the perf here soonish. If anyone else wants to take a look before I get around to it, the problem is that we continue tracking handled promises in the
pendingUnhandledRejectionsarray even if they haven't been warned.@devsnek I'm looking. I'm not sure that's an accurate description of the problem: those rejections are tracked so we can determine if they will be warned, we only know if they are warned or not after
foo()executes (because we're "stuck" in the microtask queue, and pendingUnhandledRejections, which runs on nextTick is not executed until the end). But there's another problem: we're usingArray.shiftonpendingUnhandledRejections, which is orders of magnitude slower thanArray.pop. I changed toArray.popand the time dropped from 8 seconds to ~5ms.popchanges the semantic though, so it can't be used as a drop-in replacement.@mmarchini if a promise has not been warned about when it is handled, we can forget about it at that point. the problem is that we leave it in that array, probably because there are generally not a lot of these situations, and splicing them out would be very slow.
You're right. So the patch below should solve it, right?
diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js index f2145d425c..2a43de3535 100644 --- a/lib/internal/process/promises.js +++ b/lib/internal/process/promises.js @@ -130,6 +130,11 @@ function unhandledRejection(promise, reason) { } function handledRejection(promise) { + const i = pendingUnhandledRejections.indexOf(promise) + if (i > -1) { + pendingUnhandledRejections.splice(i) + return; + } const promiseInfo = maybeUnhandledPromises.get(promise); if (promiseInfo !== undefined) { maybeUnhandledPromises.delete(promise);
Tests are passing (
apparently, I got a bunch of apparently unrelated failuresbuild was all weird, rebuild and tests passed), but I'm probably missing some edge cases here 🤷🏻♀️. More importantly, would this have an impact on runtime performance? Do we have benchmarks for it?It will decrease the throughput of adding catch handlers to rejected promises. I'm not sure how much we care about that.
That's what I thought. It's still hard to determine if this would be an improvement on overall performance, if it would degrade it or stay the same. With this we have more latency on .catch, but less on the next tick after the catch. I think it's likely not perceptible on most applications, but only way to find out is by running some benchmarks.
With some really arbitrary not at all scientific microbenchmarks, the patch above has overall better performance. I'll open a PR and will see if I can collect more perf info around it.
Edit: ran some better benchmarks and the patch was slower, I'm evaluating alternatives.
- added a commit that references this issue
on Aug 21, 2020 Thanks for jumping on this. The PR also looks great.
whereas on real world applications this is unlikely to happen
I think you are right here. The use case I have is simulation software which intentionally uses promise rejections fairly frequently. It runs on its own as fast as it can (a while loop processing events as fast as it can). Obviously that isn't the typical case.
What steps will reproduce the bug?
Observe the long runtime. If you add some logging, you'll notice that program execution actually finishes first, but the process doesn't exit for some time afterwards.
How often does it reproduce? Is there a required condition?
The behavior is consistent across runs. The promise must be rejected. Resolving the promise does not trigger this delay in exiting.
What is the expected behavior?
To not take so much time handling rejected promises.
What do you see instead?
The program's output might finish first, but the process doesn't exit for some time.
Additional information
Profiling, reports that some time is spent in the
processPromiseRejections()function's main loop, as shown.