Consider exposing promise unhandled rejection hook

[benjamingr]

Promise unhandled rejection

It is common for promise libraries to emit possibly unhandled rejections (promise chains no .catch listener or then with a second argument is attached to).

Most libraries as well as native promises in V8 support detecting these cases.

It is common for code to have multiple promise libraries, native promises and multiple copies of the same promise library included. Sometimes people are interested in hook on such unhandled rejections in order to add custom logging, terminate the process, suppress the rejection etc. - a survey (in the later linked gist) indicates that this is used in practice by users.

What I'm asking for

I'm asking for native promises to emit events on possibly unhandled rejections so that user level code can handle them to provide better debugging.

Here is the actual proposal

It is currently in the seeking feedback stage.

Effect on user APIs

None, there is no resource penalty, API changes or backwards incompatible changes introduced by this.

Implementation in userland

Since this change requires talking to the native V8 promise implementation which in turn cannot implement this itself since it is unaware of io, there is no possibility to implement this at a user level either because the hooks are not exposed.

Non native promises (promise libraries) will implement it at a userlevel but since people are writing code that depends on native promises and they are a language feature this hook is required.

[rvagg]

I like this idea here because the ease in which you can ignore errors with Promises is one of my major gripes, but I'm not sure it belongs in core, perhaps this can be achieved in userland?

[benjamingr]

@rvagg I'd love for a userland solution but I don't think this is possible. Since V8 promises are native and the ES promise specification does not dictate a static method on Promise doing this there is no way to "hook" on their unhandled rejection tracking mechanism from userland.

If you come up with some genius hack around that from userland that would be awesome but I'm afraid that the necessary primitives are not exposed outside by default by v8 nor could it expose them itself because it is unaware of process.

[bnoordhuis]

@benjamingr is correct, it requires some (almost trivial) interaction with V8's C++ API. Question: should we turn turn unhandled rejections into exceptions by default? Not handling a rejection is arguably an application bug.

[benjamingr]

@bnoordhuis if detection were deterministic converting rejections to exceptions would have been the correct course of action here - the problem is not all unhandled rejections are are detected and there could be possibly unhandled rejections that not really unhandled rejections. It's also quite possible some people won't understand promises the same way we do and have something like:

function doStuff(){
  var foo = Promise.reject(new Error("foo")); // cache rejection
  return tryMakeConnection().catch(function(err){
    return foo;
  });
}

Here doStuff creates an unhandled rejection (foo) and might never add a catch handler to it since the code in the .catch might not run. I'm pretty sure there are people that would not expect the server to crash in this case.

Having a configurable "possiblyUnhandledRejection" event can let users decide and also would not break existing 0.11 code bases that rely on the existing behavior.

[vkurchatkin]

@benjamingr nice proposal thank you. Here something I've been working on: vkurchatkin@980fb8f

couple of quirks discovered:

• hooks are completely synchronous, so this will trigger callback anyway:

Promise.reject(new Error('err')).catch(function(){})

• for the reasons unknown PromiseRejectMessage for revocation event doesn't have a value: https://github.com/v8/v8-git-mirror/blob/master/src/runtime/runtime-internal.cc#L79. Not a big deal, but annoying.

Here is an interesting example of how promises can be used in a way that requires rejections to be ignored completely (even if they are never handled):

Consider a File abstraction for reading from files. Files should be opened first, but that could be nicely abstracted away like in fs streams:

function File(path) {

  EventEmitter.call(this);

  this._fd = null;
  this._error = null;
  this._opening = true;

  var self = this;

  fs.open(path, 'r', function(err, fd) {
    self._opening = false;
    self._error = err;
    self._fd = fd;

    self.emit('open');
  });

}

File.prototype.read = function(callback) {
  var self = this;

  if (this._opening) {
    return this.once('open', function() {
      self.read(callback);
    })
  }

  if (this._error) {
    return process.nextTick(function() {
      callback(self._error);
    });
  }

  read(this._fd, callback);
};

And this is how it can be done with promises:

function File(path) {
  this._promise = fs.open(path, 'r');
}

File.prototype.read = function() {
  return this._promise.then(read);
};

The state (_opening, '_error', '_fd') is incapsulated in a single promise. But this promise is private so the only way for user to handle possible rejection is to call .read. If it is never called then it doesn't matter if _promise was rejected or not.

Here is how it can be handled:

var s = Symbol();

process.on('unhandledPromiseRejection', function(promise, rejection) {
  if (promise[s]) rejection.handle();
});

function File(path) {
  this._promise = fs.open(path, 'r');
  this._promise[s] = true;
}

In domenic/promises-unwrapping#19 someone proposed a special function for that like promise.undone() or promise.delayed(). That could be implement without extending prototype by exposing a symbol so any library can mark a promise as "delayed".

[benjamingr]

@vkurchatkin grander things like promise.delayed() or promise.undone are interesting but I believe that beyond exposing the most minimal hook possible: an event for possible detection and another event for a detection mistake) these things can and should be solved in user land.

Your idea (with the Symbol) looks about right and can be easily wrapped in a neat little library. After all - there are plenty of different libraries for synchronicity with different takes on what to do here - it's part of what makes this ecosystem great.

The File example is really good - some people might argue that the constructor should never perform IO or call a promise but in practice I've seen lots of codebases do this.

Also good find on the quirks. The fact Promise.reject(new Error('err')).catch(function(){}) is considered an unhandled rejection is really bad, it's pretty possible that I did not locate the correct source of unhandled rejection tracking in v8. I'm very rusty in the v8 code - what about this method in isolate.cc itself?

Great and super fast work by the way!

[petkaantonov]

Instead of using Symbol and the hooks, I think it would be better if you just implemented File like this:

function File(path) {
    this._path = path;
    this._handle = null;
}

File.prototype.handle = function() {
    if (!this._handle) this._handle = fs.open(this._path, "r");
    return this._handle;
};

File.prototype.read = function() {
    return this.handle().then(read);
};

In other situations where one might attach catch handler asynchronously it has been better to simply refactor it

[vkurchatkin]

I think it would be better if you just implemented File like this

This will probably be a good solution for most of the cases, but sometimes lazy behaviour is undesirable. Promises are used not only for asynchronous flow control, but also to represent results of an operation. The fact that promise has been created doesn't necessarily mean neither that user has any interest in these results nor that they affect any other part of the system.

Your idea (with the Symbol) looks about right and can be easily wrapped in a neat little library.

This should be done in core to ensure symbol is unique.

I'm very rusty in the v8 code - what about this method in isolate.cc itself?

It's a simple wrapper for user-provided callback called from here https://github.com/v8/v8-git-mirror/blob/55bdf90f60d72b2c3278edcc693b2e12425ba421/src/runtime/runtime-internal.cc#L79 and here https://github.com/v8/v8-git-mirror/blob/55bdf90f60d72b2c3278edcc693b2e12425ba421/src/runtime/runtime-internal.cc#L65. These are called from JS: https://github.com/v8/v8-git-mirror/blob/master/src/promise.js#L177, https://github.com/v8/v8-git-mirror/blob/master/src/promise.js#L220, https://github.com/v8/v8-git-mirror/blob/master/src/promise.js#L220

[benjamingr]

@vkurchatkin I don't think you're going to get a lot of support for any proposal that requires monkey-patching every native promise in io because:

• Promises with that symbol could be done in user level.
• That probably belongs in the TC39 domain (where you can definitely argue for it) since it requires a modification to a native object.
• The way users use unhandled rejection tracking varies greatly - if you check the statistics you can see there are people suppressing these, people throwing on it, people doing custom logging with logic, people handing with symbols like you have and so on.

While your symbol suggestion might be viable I think that it's divisive enough to defer its inclusion to a later date.

Also note that it's possible to implement this without symbols to enable code-sharing between other libraries that are to implement this and clients via an object:

  var s = {};
  // choose one of many names no one is using
  if (promise.sInteresting === s) rejection.handle();

Not as pretty but still useful.

[vkurchatkin]

just to be clear, I'm against monkey-patching. My idea is a public symbol (process.delayedPromiseSymbol) and default listener that checks for this symbol. Can be done in userland, yes, but because of dupes you can end up with many symbols that mean the same thing. I'm not sure about performance impact of attaching arbitrary stuff to promises. @petkaantonov what do you think? will it cause deoptimizations/recompilations/polymorphisms or something like that?

// choose one of many names no one is using

This is proved to be hard and that's what are symbols for)

[petkaantonov]

I'm not sure about performance impact of attaching arbitrary stuff to promises. @petkaantonov what do you think?

Native promises are very slow to begin with, they wouldn't be affected by this level of optimization at all.

[domenic]

The actual semantics of this API in terms of timing should be as they will be in browsers: wait for a full event loop turn (not just a microtask) before calling the hook.

See also https://gist.github.com/domenic/9b40029f59f29b822f3b#promise-error-handling-hooks-rough-spec-algorithm

[benjamingr]

Any updates on this @vkurchatkin ?

[vkurchatkin]

@benjamingr not yet

[vkurchatkin]

assigning this to myself, if no one objects

[benjamingr]

Thanks!
​

[domenic]

Re-reading this thread and a few things I haven't seen before.

I think the default should either be handle-it-yourself, or paired logs for when the rejection is unhandled and then handled later. Switching between these two should be easy; the question is which is default. E.g. do you opt-in with process.logUnhandledRejections() (or, perhaps more accurately, setDefaultRejectionHandlingListeners) or do you opt-out with process.hideUnhandledRejections(). I'm not sure which.

Regarding process.delayedPromiseSymbol, I don't see why p[process.delayedPromiseSymbol] = true is any better than p.catch(() => {}).

Regarding how the hooks are over-eager, the idea is to use something like the algorithm I linked to above to collate them into a single full turn around the event loop.

[benjamingr]

@domenic I think the path to take initially is that of least resistance so I'm for "handle it yourself". I think the default behaviour can be changed in a later commit and making it opt-in at least initially would give us opportunity to better battle-test it without having any surprises. I do hope that at least eventually these rejections would get reported by default - and that being able to choose as a developer is important and useful.

I agree about delayedPromiseSymbol - I don't see the huge merit over suppressing it yourself either but that's something to set.

I'm not really sure why you need a special ID in that algorithm (vs just using the reference) but I guess since I'm not the one implementing it I probably don't understand the rationale very well.

[domenic]

@benjamingr ID instead of reference is to avoid keeping the promise alive in the GC.

[benjamingr]

@rvagg hey, I'm sorry to bother you again :) I want to know how I can make this thing go faster - I'd really like to have this feature, I believe it is the minimal thing we need in order to go ahead with a better user experience for debugging promise unhandled rejections.

Who do I need to sell this too and/or who do I need to convince to implement this? It seems like everyone I've spoken to is in favour (both from userland promise libraries, Google people or here) but it's unclear to me what's the process from "People agree it's a nice idea" to "It's implemented and shipping in io.js". I really don't want to look like a nag - just interested in how to move this process forward :)

[rvagg]

@benjamingr first, I'm not the person you need to convince, you just have to make sure that you don't have any disagreement amongst collaborators and I don't believe I've seen any so far so that's a good sign.

I suggest the next step is to actually implement this and submit a pull-request. Alternatively, find someone who can implement it, there may be people in this thread or in the other big Promises thread that can do it. If you are not confident in your ability to contribute "perfect" code for this then submit a pull-request early and ask for guidance--you're likely to get lots of help that way.

When there is code for people to see, you're much more likely to bring any potential objectors out of the wood-work to argue with, you're also more likely to spur discussion on exactly the best API to end up with, but you have to start with code.

[petkaantonov]

@vkurchatkin Do you mind if I base a PR based on your code and do it like @domenic's original spec / how it's done in bluebird.

@domenic I think implementing such things should be left to third party modules, all the core needs to provide is process.on("unhandledRejection" and process.on("rejectionUnhandled") and if there is no listeners for "unhandledRejection" the default is to write the stack trace on stderr. If the default requires some action to surface the unhandled rejections this whole thing is pointless because for the general developer population the logged stack trace is the only way they are going to discover such a thing even exists.

On top of these hooks you can implement alternative mechanisms, for instance only log the trace when the promise object is garbage collected (needs native module but still). And you can also implement the manual log/unlog thing that Q does if you want of course.

[vkurchatkin]

@petkaantonov of course not, go ahead

[domenic]

@petkaantonov you made this assumption which IMO is kind of the whole question at hand

if there is no listeners for "unhandledRejection" the default is to write the stack trace on stderr.

I disagree that there should necessarily be a default. Thus all the things I talked about in my post.

I strongly disagree that you should ever implement a log-on-unhandledRejection without a counterpart log-on-rejectionHandled.

[benjamingr]

@domenic it's worth mentioning that in @petkaantonov's PR the default behavior is to not do anything on unhandled rejection detected - that is postponed to a future issue/PR as far as I understand.

Personally I find log-on-unhandledRejection (with the counterpart) very useful but after looking at how people used unhandled rejection detection with bluebird it's clear that some people felt differently.