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