cujojs / when

A solid, fast Promises/A+ and when() implementation, plus other async goodies.
Other
3.44k stars 396 forks source link

`when.all`/`Promise.all`, iterables, and unhandled rejections #360

Closed briancavalier closed 10 years ago

briancavalier commented 10 years ago

See backstory in #356. In ES6, Promise.all accepts an iterable. Iterables may be lazy and/or infinite. While I don't think it makes sense to pass an infinite iterable to Promise.all, it certainly seems fine to pass a lazy iterable--that is, one which generates promises on demand as next() is called.

In the non-lazy case, once the first rejection is encountered, it makes sense to attempt to visit the remaining items in the iterable and mark any further rejections as already observed, so that they don't generate extra unhandled rejection reports. Ie we only want the promise returned from Promise.all to be able to generate a report.

However, in the lazy case, it doesn't make sense to visit the remaining items. For one, if they aren't visited, they will never have the opportunity to generate another rejection, thus no extra unhandled rejection reports will ever happen. For another, visiting the remaining items is potentially doing lots of unnecessary work, and nullifying the benefit of the iterable's laziness.

My reading of the ES6 Promise.all spec is that the algorithm should stop when it encounters the first rejection. That takes us back to the original problem reported in #356 :/

The only thing that seems like it will work in all cases is to visit all the items (ie forcing lazy iterables), which means violating the ES6 spec when we encounter a lazy iterable.

rkaw92 commented 10 years ago

This issue casts light on a perhaps bigger problem - interoperability. After all, we can not reasonably expect users to only use when.all for aggregating when.js promises' resolutions. They could just as easily mix ES6 native Promises and other implementations in the same application.

What happens when a user calls Promise.all([ when.reject(1), when.reject(2), when.reject(3) ]), where Promise is not at all when.js-related (e.g. ES6 native)? The promise consumer may or may not visit the remainder of the array. The linked post has led me to believe the latter behaviour to be correct.

As I see it, there are only two solutions viable in the long term:

  1. Get unhandled rejection reporting added to the ES6 standard. This would take extraordinary amounts of resources, and still be very disruptive (not mentioning the difficulty of subverting the standards body's work so far) - for example, it would likely require Iterators to have either laziness/finiteness-checking methods, which in itself is a cumbersome notion, or a "remainder getter" function which could possibly throw errors / return nulls, signifying that it is not, in fact, capable of providing a remainder (and that the rest should be ignored). Of course, wreaking such vast havoc just to get unhandled rejection reporting in the standard is unjustified and would never pass. Which brings us to option 2...
  2. Disable automated rejection reporting (at least for ES6 environments, though it probably makes sense in all cases - see my "other implementations" remark at the beginning) so that rejected when.js Promises do not produce any side effects such as I/O by default. Provide a facility for the user to enable it again - for example, by calling a "static" method of the library or using a "customizer" idiom (one of my favourites).

Example of the customizer idiom using a simplistic, completely unrelated look-alike:

// module: then.js
var defaults = { unhandledReporting: false };
function customizeWhen(options){
  return {
    promise: function promise(resolverFunction){
       // Use the customized options to alter behaviours here.
    }
  };
}
module.exports = customizeWhen(defaults);
module.exports.customize = customizeWhen;

Call this with:

var then = require('then').customize({ unhandledReporting: true });

Then, the users of the customized, configured version would gain extra awareness regarding the need to observe the remaining promises when aggregating - or at least an excuse for why their console is being spammed with console.logs... (Plus, a special "export" behaviour disabler could be useful for functions which return promises for consumption by external APIs - seems like quite some hassle.)

The bottom line is, I'm forced to say when.js may need to get rid of side effects by default to work correctly in a multi-implementation world, even though I'm a huge fan of the recent debugging functionality.

(Fun fact: out of when.js, bluebird and "promise", not a single one actually conforms to the mentioned presumed ES6 spec - all of them internally mark the promises as handled in some way, whether by accident (promise) or intentionally (when, bluebird). How bluebird actually does it, I was unable to determine - its code is really hard to read for me. Must be .then(), though, since that's all the public API we can rely on... If this trend continues, the "promise library lobby" might actually have some voice in the matter and drive the designation of a secondary, "helper" API just for arrays of promises O.o )

briancavalier commented 10 years ago

You've raised some great points here, @rkaw92.

Now you've got me thinking that the best thing for when.js may be to strictly follow the ES6 spec and let the extra unhandled rejection reports fall where they may. There is already an API for hooking into the unhandled rejection reporting, so folks can disable or tweak it in situations where they need ... they can even disable it entirely for production, for example.

Some native implementations will be able to use garbage collector hooks (Firefox native promises do this already) to report unhandled rejections, but even that's not sufficient to deal with the Promise.all problem.

I think the best thing might be to get some other implementors and ES6 spec folks involved in the conversation. It could be that they've already thought through this issue. If they have, maybe they have a good answer. If they haven't, then it'll be good to get another perspective.

briancavalier commented 10 years ago

I just tried Promise.all([Promise.reject(1), Promise.reject(2)]); in FF, and it only prints one warning: for 1. I tried with a large number of promises in the array and got the same result: only the first rejection is reported. So, it must be visiting all the array items, or doing some other magic.

Interestingly, I also tried a lazy iterable:

    function* f(x) {
        while(--x > 0) {
          console.log(x);
          yield Promise.reject(x);
        }
      }

      var p = Promise.all(f(3));
      p = null;

That logs 2, 1, but then never logs an unhandled rejection (I did check that p rejects and it does). I'm not sure what to make of that ... something has to get garbage collected. Based on the console.log, though, once again, they are visiting all the items, even in a lazy iterable (despite an apparent bug of not logging the unhandled rejection).

I also re-read the ES6 spec and I could have been interpreting it incorrectly before. This phrase is interesting:

It resoves all elements of the passed iterable to promises as it runs this algorithm

That would seem to indicate that it does not stop early, but visits all the items. In that case, lazy iterables would be forced, which corroborates FF's behavior.

rkaw92 commented 10 years ago

Hi,

I've just accomplished the daunting task of reading through the Promise.all semantics draft, as seen here: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-promiseall

It seems that we were both wrong in the beginning. Also, relying on comments on MDN and such turns out to be prone to failure.

In short, here is my understanding of the complete algorithm:

This would seem to invalidate all our "early exit" concerns.

briancavalier commented 10 years ago

Only exit early and reject when an error is encountered while getting the iterable's next value

Ah ha! This is a really clear distinction. Thanks! Seems I really misinterpreted some parts of the spec ... it is daunting indeed :)

This would seem to invalidate all our "early exit" concerns.

Yep, though it does mean forcing lazy iterables, which I guess is the least bad option. Maybe we just treat it as a documentation issue, though: when.js (and any ES6 compliant impl) should just document that all items in an iterable will be visited.

Ok, so, the behavior in 3.4.5 is correct, that's good. I'll take another look at the code to see if it can be simplified in light of this new (and correct!) understanding of the ES6 spec. And we'll know what to do once we update it to handle ES6 iterables.

I really appreciate your diving into the Promise.all spec, and all the discussion on this!

I feel like we can close this issue ... and I think I'll open another one for implementing iterable support.