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.