Proposal for more correct detection and reporting of unhandled promise

[mike-marcacci]

Background

There exists a contingent of the community who wants node's default behavior changed to crash as soon as a promise is rejected if an error handler is not yet been attached to the promise. This breaks one of the most important design features of promises: allowing predictable execution regardless of when a handler is attached (either before or after a promise is resoved/rejected; and before or after any other handlers are attached). This guarantee of promises protects them against many of the hard-to-reproduce timing bugs I often see associated with event emitters, streams, and other concurrency patterns. Accordingly, I'm very resistant to seeing javascript get strongarmed out of this feature by one runtime.

I've made my opinions quite clear in my comments on this issue, but the actual implementation described there lacks clarity and has changed multiple times throughout the issue's history.

I would like to propose a very specific course of action here, and I would also like to encourage those with differing perspectives to create standalone issues that describe specific alternative plans so that we (the broader community) can more clearly identify the consequences of each.

Proposal

1. The "end of life" for an instance of Promise occurs either:

   • when the promise instance is garbage collected
   • when the application exits normally (ie. when the exit event is emitted)
   
   

2. At the end of life for each instance of Promise, node should check for the presence of handlers for the following states:

   • fulfilled
   • reject
   
   

   If either of these states have no handlers, node should emit an unhandledPromise event on the global process object with a payload object containing the following fields:

   state

   A string, either "pending", "rejected", or "fulfilled"

   value

   The value/reason of a fulfilled/rejected promise, or undefined.

   onFulfilled

   An array of handlers for the fulfilled state.

   onRejected

   An array of handlers for the rejected state.

   stack

   A string in the format of Error.prototype.stack tracing the construction of the Promise instance.

3. If there are no listeners for a unhandledPromise process event when one is emitted, node should print a warning to stderr.

4. The resolve and reject functions made available when constructing a Promise must internally keep a weak reference to the promise instance. This prevents undesired retention of the promise which could delay or circomvent this intervention.

Important Features

There are several important features and goals guiding this proposal:

It must not break promises.

Most importantly, this does not change the semantics of javascript promises or invalidate any well-established patterns.

It must not encourage self-defeating workarounds.

By providing a mechanism for applications to programattically ignore specific warnings, we avoid encouraging libraries to modify promises in ways that make potentially problematic scenarious undetectable by node (such as adding noop handlers like promise.catch(() => {})).

It should identify problematic conditions as early as possible.

Promises intentionally don't expose a mechanism for checking an instance's internal state. Therefore, any promise that has reached its end of life has all the handlers that it will ever have.

The primary concern motivating any special handling of promises is that rejections will go unhandled. Any promise that has reached its end of life without a rejection handler (regardless of its state at the time it reached its end of life) is vulnerable to this.

It must provide developers sufficient information to track down potential problems.

A stack trace to the construction of the promise provides a developer everything necessary to identify a potential problem or to selectively ignore safe discarding of unhandled promises.

Possible Problems

Creating a stack trace could have a non-trivial performance penalty. Some optimizations may mitigate most of this:

• Discard stack information as soon as a handler is added to both final states.
• Lazily construct the string representation as a getter.

See Also

• The Promises/A+ spec
• Promise Objects in the ES6 spec

Edits

1. Added emitting of unhandledPromise events on exit.

[devsnek]

Seemingly a duplicate of #20097

[mike-marcacci]

@devsnek This is certainly related, but quite different in several regards. In particular:

1. This has nothing to do with rejections as a special case.
2. This provides a trace to the promise, not the error.
3. This does not exit the program.

This should be left open to at least receive critique, as it addresses the issue with a novel approach.

[devsnek]

I don't think node core should expose behaviour that depends on the determinism (or lackthereof) of the gc . You could make a npm module that uses FinalizationGroup though.

[mike-marcacci]

@devsnek I agree that all of this would ideally live at the application layer (IMO warnings of legal but potentially undesirable patterns should be the concern of neither the runtime nor individual libraries that use these patterns). However, I see a few reasons for incorporating this into node core:

1. There appears to be a desire for node to take some kind of action regarding promise lifecycles. Node already implements a hook/warning/crash for a very specific sub-case of promise rejections. While the current implementation of these is deterministic in one sense (in its internal implementation), it is still nondeterministic from the perspective of a user, since an asynchronous job that registers a rejection handler can race against the job that rejects the promise. In these cases the current hook/warning/crash is not just nondeterministic, but are incorrect when the handler registration loses the race. Despite these problems there appears to be an appetite for intervention on the part of the runtime. While I certainly understand an argument against this kind of thing existing at all, I argue that if there must be something providing this functionality in core the correctness and broader utility of my proposal is worth the timing unpredictability imposed by tying it to the gc.
2. This would be technically infeasible to implement outside of core. Perhaps this is just my unfamiliarity with some node internals, but it appears impossible to replace node's core Promise implementation in a way that is used by async functions. Perhaps this is a factor in the appetite described in my first point? Perhaps allowing the Promise implementation to be overridden would allow for a diversity of approaches to be implemented?
3. As the WeakRef continues to evolve and be implemented, FinalizationGroup will become available and node core will have to expose behavior that depends on the gc.

[devsnek]

Node core's behaviour is still deterministic though. even if the application is randomly creating tasks, node's reaction is always deterministic.

Also, node can't replace what async functions return either. We could talk to V8 about it I guess but it would not be a trivial change to V8 and would break a lot of optimisations.

[isaacs]

This is a great idea.

It would be maybe a nice compromise if process.emit('unhandledPromise') did crash the process when:

• There are no rejected handlers
• The promise is rejected

Or, at least, it could set process.exitCode = 1, so that the process will have an exit status whenever it does exit. (Since GC timing is not guaranteed, it might be bad to kill the process when that happens.)

Even without that, this event would help track down the bane of my existence lately, which is this:

someMethod () {
  firstThing()
  .then(() => secondThing())
  .then(() => thirdThing())
  .then(() => fourthThing())
}
const firstThing = () => somePromiseThing('first')
const secondThing = () => somePromiseThing('second')
const thirdThing = () => { somePromiseThing('third') } // <--- ooops
const fourthThing = () => somePromiseThing('fourth')

I spent 2 hours pulling my hair out trying to figure out why files were randomly disappearing while being copied from one folder to another, because the cleanup action was happening before the copy was done. Being able to track down orphaned promises would be a life saver.

The non-determinism could also be addressed by, instead of emitting an event, making it a property that could be queried in an async_hook or internally by the system at some specific moments.

[devsnek]

the non-determinism is that there's no guarantee your promise is ever collected in the first place, so you are likely to never see the warnings you're expecting except in a long-lived process that allocates a lot of objects.

Adding handlers for unhandled fulfillment would be neat, and probably warrants an issue of its own. It might need to be default disabled for perf reasons but it would be a valuable debugging asset.

[mike-marcacci]

@isaacs very interesting idea RE setting the process exit code without exiting; I'm not sure I've ever seen that pattern in the wild.

@devsnek I resisted adding this to my proposal since the current state of "exit handlers" in node is already quite complex (IIRC there is some work being done to simplify this somewhere?), but we can address your point by emitting an unhandledPromise event for each uncollected promise matching the proposed criteria immediately proceeding an exit event. This would effectively limit unhandledPromise event handlers to synchronous code, which may not necessarily be problematic. This approach has another quirk (or maybe a feature) in that an exceptional exit may report unhandled promises that would have been handled. Regardless, if we think of an unhandled state at the "end of a promise's life" as a reportable condition, it would make sense to emit this event either on gc or on exit. This would reduce the nondeterminism to "eventuality."

Would this change address your concern RE the nondeterminism of associating this with gc events? If so I will update my proposal accordingly.

[devsnek]

Exiting the process doesn't run the GC, and even if it did (which it shouldn't), you'd still miss unhandled promises because they'd still be ref'd.

[mike-marcacci]

@devsnek I think you misunderstand what I'm saying here. It's not that retained promises are GC'd on exit; it's that we apply the same logic to retained promises on application exit, which I've now made part of this proposal.

[devsnek]

I believe that was discussed in the issue I linked as well. We definitely couldn't keep a list of all those promises by default (it would be enormous and hurt performance when it had to be modified). Getting bombarded with a ton of promise warnings is not a great UX either. At that point, you probably want to start considering using devtools addons instead.

[mike-marcacci]

I've read through the PR you linked, and there is a very brief mention of such a strategy; however I suspect any further discussion took place in a different channel, as I couldn't find real arguments in either direction.

I agree with the notion that literally dumping 1000 of these logs at program end would be unhelpful, and so perhaps the logging logic could limit such a scenario to 10 or so.

However, I don't believe this would necessarily incur a massive performance hit: while I'm not particularly familiar with the internals hers, this should be trivially implementable with O(1) time complexity for Promise constructions and garbage collections, and O(n) for iterating through retained promises at exit, where n is the maximum number of simultaneously live promises. And it's possible we could get even better than this.

If we can just assume that this can be written to not damage performance (which would need to be proven in an implementation), do you (or does anyone else) still have objections to this strategy in theory?

[devsnek]

I will always object to strategies that rely on the GC, because I find nondeterministic debugging tools to be near useless.

Also, even if the time of logic to store a list of promises is actual non-amortized O(1) (which is impossible), it still adds time to every promise creation, which isn't acceptable (by default). And even then, you double-ref every promise, which takes up usable memory.

[mike-marcacci]

There certainly would be some cost associated with this proposal, but I don't believe it would be substantial or problematic.

By design, a promise is valid without any handlers at any point in its lifecycle. However, it becomes suspicious once it's impossible to attach new handlers. This is provably the case on exit and is the approach taken by debug tools like loud-rejection.

My proposal here is about using the GC to optimize this flow and deliver results earlier than otherwise possible. IMO the timing/nondeterminism that you dislike here is fine, since this is just a "best effort" optimization for tools like the aforementioned. Additionally, if performance is a concern, I would be more than happy for this to live behind a flag.

The current approach to warning on rejected errors is flat-out incorrect, and should be removed. I would be OK removing it, full stop; but my sense is that the community would want a replacement, and this proposal provides the machinery to make a correct and timely (ie useful for long-running processes) replacement.

[devsnek]

there's no guarantee that the gc will deliver results "earlier than otherwise possible". It also seems like you could just pr the behaviour you want to loud-rejection with FinalizationGroup.

[mike-marcacci]

I would do this if it were possible – this proposal is about making this kind of thing possible. Node currently lacks any mechanism for detecting the construction of native promises, and javascript code has no ability to inspect the internal state (including attached handlers) of any native promise.

This is how the loud-rejection keeps track of unhandled rejections.

[jasnell]

Node currently lacks any mechanism for detecting the construction of native promises,...

This is incorrect. Async_hooks can be used to detect the construction of native Promises.

const { createHook } = require('async_hooks');

const hook = createHook({
  init(id, type) {
    if (type === 'PROMISE') { /** native promise created **/ }
  }
});

hook.enable();

The hook will also be notified when the Promise resolves and when the Promise is destroyed. Note, however, that there is a significant performance cost to enabling the lifecycle tracking for Promise. With your proposal, there is not just "some cost", it would be significant in that every Promise instance must be wrapped -- and we've seen applications in the wild with 30-60 thousand Promise instances created in every 10-30 second span under load. The performance cost is neither trivial nor theoretical. We've tested this stuff extensively.

and javascript code has no ability to inspect the internal state (including attached handlers) of any native promise.

This is also incorrect. It's not super efficient, but you can pass a Promise instance in to Node.js util.inspect() function and get back a string that specifies it's resolved state.

> util.inspect(Promise.reject())
'Promise { <rejected> undefined }'

> util.inspect(new Promise(() => {}))
'Promise { <pending> }'

The current behavior is not without it's justifications. By the time a Promise rejection is handled, the application state that led to the rejection has been altered or lost, making post mortem diagnostics extremely difficult if not impossible. In browser applications, this is typically not that important of an issue but in server and embedded applications, it is often absolutely critical to the ability to diagnose issues.

That said, Node.js recognizes that the default behavior does not serve everyone which is why we provide the --unhandled-rejections=none option to completely silence all warnings around unhandled rejections.

[devsnek]

@jasnell

The current behavior is not with it's justifications. By the time a Promise rejection is handled, the application state that led to the rejection has been altered or lost, making post mortem diagnostics extremely difficult if not impossible.

Can you expand on what kind of diagnostics are being done here? Post-mortem implies the entire application is gone, so I'd assume you're already collecting data, which means whether you handle the rejection at the time of rejection or not is kind of a moot point?

[jasnell]

e.g. a core dump taken at the instant a fatal exception occurs before the stack unwinds...

[devsnek]

@jasnell why does that require that .catch be attached synchronously? Are you putting .catch(fatalCoreDump) on each promise individually?

[jasnell]

That's not what I'm saying. The argument is that the node.js process should crash immediately when the error is thrown, not when it is handled by catch, even if catch is agreed synchronously. The reason is because the application state changes and the relevant data is lost by the time the exception is reported and handled.

The other key issue here is that unhandled rejections have a nasty habit of delaying critical mitigations that need to happen to maintain system stability. Closing file handles is one example and is again, not theoretical.

[devsnek]

@jasnell so you're saying even a promise with a catch handler attached synchronously (no unhandled rejection) handles the error too late?

[jasnell]

Yes, because the catch handler is not invoked until the microtask queue is drained. By that time process state has already been lost.

[devsnek]

@jasnell so then why does the existence of unhandled rejections matter at all to your use case? By the time node is deciding whether the unhandled rejection should be warned/silenced/thrown/etc, it is already beyond the useful time for your system.

[jasnell]

@devsnek ... that's precisely why --unhandled-rejections=strict mode exists. It crashes the node.js process at the point the error is thrown. Several have argued that the strict behavior should be the default. The current warning behavior is a middle road that at the very least makes it far easier to detect where and when an unhandled rejection is happening.

[devsnek]

@jasnell strict is dispatched through the same mechanism as everything else.

If strict works for your use case, you can just attach an unhandledRejection listener in your app and not have to pass a flag anymore.

[mike-marcacci]

Hi @jasnell,

Thanks so much for your comments here and sorry for the slow reply. I have a whole article's worth of thoughts, scenarios, and arguments (and also an alternative proposal) that I still plan on adding when I have time.

However, I did just want to quickly follow up on the ability for user code to inspect promise internals. Your note about using async-hooks to detect construction is super helpful; noting the trick of using util.inspect to inspect fulfillment state may also be useful.

Hhowever, there's still (as far as I know) no way to access the list of handlers registered on a promise, which is obviously required to implement my proposal. If there is a way to prototype my proposal here I'll certainly build it.

[jasnell]

Closing given the lack of further discussion and it's not clear at all that there's anything remaining to do here.