domenic / promises-unwrapping

The ES6 promises spec, as per September 2013 TC39 meeting
1.24k stars 95 forks source link

Promise.race with empty lists #75

Closed juandopazo closed 10 years ago

juandopazo commented 10 years ago

At the moment Promise.race([]) returns a forever pending promise. Is that a good idea? It sounds like a potentially hard problem to debug. Should it be rejected instead?

domenic commented 10 years ago

I will defer to @erights, our Promise.race champion.

erights commented 10 years ago

Hi @juandopazo , I do see both sides of this. There is a pros and cons in each direction. It comes down to a judgement call. Consistency argues for the status quo -- which has nice algebraic properties. Racing races is associative and commutative, and racing the empty list, i.e., a promise that never resolves, is the identity element.

race([a,b,c])  equiv  race([race([a]),race([b,c])])  equiv  race([race([]),race([a,b,c])])

Although all is not associative or commutative, if we ignore all's fulfillment value and only look at whether it fulfills or rejects, then alls of alls is similarly associative and commutative. Alling the empty list, all([]), i.e., a fulfilled promise, is again the identity element.

So I'm closing this as the status quo.

petkaantonov commented 10 years ago

@erights I disagree. API should not prefer some invisible theoretical consistency. It is in most cases almost surely a bug in code if .race gets called with an empty array - I don't see anyone at that point saying "ah this was hard to debug but after thinking about it, at least it's consistent when comparing racing races and alling alls".

kriskowal commented 10 years ago

I agree with @erights. Algebra informs my intuition about the behaviors of functions, cultivated by reduce, Math.min(), and Math.max(), and I would expect the degenerate case to be consistent with @erights interpretation.

juandopazo commented 10 years ago

Yes, I like the reasoning.

stefanpenner commented 10 years ago

This makes me sad. But i'll be even more sad when the first time i need to debug this scenario.

nikhilm commented 10 years ago

I really think this should be reconsidered. All theoretical consistency goes out the window when you get a array of promises from somewhere else in the codebase and pass it to race. Expecting developers to check the length before each call is ridiculous. How about when using an iterator?

There should at the very least be a big warning in the spec describing race()s behaviour and telling developers to check the length of the array. There should also be a suggestion that user agents warn when this happens.

briancavalier commented 10 years ago

@erights and @kriskowal is it possible that another algebraic interpretation of race is as a specialized fold? If so, then it makes sense that race([]) would be an error, much like folding an empty structure with no initial value is an error (eg in JS [].reduce(anyFunction)).

erights commented 10 years ago

Could you expand on the fold analogy? I don't see it. Are you thinking that racing n things is like running a tree of n-1 pairwise races?

On Thu, Jan 23, 2014 at 4:54 PM, Brian Cavalier notifications@github.comwrote:

@erights https://github.com/erights and @kriskowalhttps://github.com/kriskowalis it possible that another algebraic interpretation of race is as a specialized fold? If so, then it makes sense that race([]) would be an error, much like folding an empty structure with no initial value is an error (eg in JS [].reduce(anyFunction).

— Reply to this email directly or view it on GitHubhttps://github.com/domenic/promises-unwrapping/issues/75#issuecomment-33187647 .

Text by me above is hereby placed in the public domain

Cheers, --MarkM

briancavalier commented 10 years ago

@erights I haven't thought it through completely, so the analogy may not hold. Conceptually, a fold takes a structure and an initial value and produces a summary. When no initial value is provided, the summary must be of the same type as the structure's value type. That all seems to be true for race. From your example (given that race([a]) ~= a) then race([a,b,c]) ~= race([a, race([b, c])), which is very similar to foldr1: foldr1([a,b,c], race2promises). If that's the case, perhaps it makes sense if foldr1([], race2promises) is an error?

erights commented 10 years ago

Ok, that's plausible. But the only reason fold of an empty array and no initial value is an error is we can't know in general whether the combining function has an identity element, or what it is. When we do know, wouldn't we always prefer fold of an empty list to produce the combining function's identity element, rather than an error?

briancavalier commented 10 years ago

Ah, yes, that's a very good point. Thanks. The analogy holds, but in the end, also seems to support returning a forever-pending promise, lol. Now I'm back to being worried about how this will play out for application developers :/

It seems like there's a testing caveat here as well: a naive implementation of race can make race([]) undecidable. It seems like the way around it is to return a singleton identity, or at least something that a unit test can verify without using then.

cscott commented 10 years ago

I also strongly disagree with giving developers such an obvious footgun. Throwing TypeError in this case should be equivalent to returning a top element in the operational semantics world. Much better to fail fast here.

cscott commented 10 years ago

Oh -- one other option -- if you really want to preserve algebraic consistency here, then I suggest the spec be updated to allow a third "forever pending" state for Promises, so that any promises chained after Promise.race([]) can be disposed and gc'ed. That would avoid the resource leak.

domenic commented 10 years ago

What? How would you possibly GC a variable that a user could use?

global.p = Promise.race([]); // how will you GC me?

// 500 lines of code later...
global.p.then(f, r); // wtf, NullReferenceException!?

global.p2 = global.p.then(f, r); // are you saying p2 should be GCed? same problem!

Browsers will GC variables that are inaccessible, but that has nothing to do with Promise.race. Resources are no more leaky with Promise.race than they are with a promise that takes forever to settle, or a promise that takes an hour to settle.

domenic commented 10 years ago

Which is to say, if the variable is garbage, it will be collected, no matter if it's a forever-pending promise or a settled one.

cscott commented 10 years ago

See https://bugs.ecmascript.org/show_bug.cgi?id=2515 for a description of the spec changes required to avoid the resource leak.

Which is to say, we need to ensure that we don't hold on to references to onFulfilled and onRejected in chained promises if the promise is never going to resolve. In your example, we shouldn't hold a hard reference to p inside p2, and we shouldn't hold hard references to f and r.

domenic commented 10 years ago

You seem to be interpreting the specification too literally. If a function (like onFulfilled in your example) is never accessible, it is garbage, and implementations can collect it. Similarly to how a Map is not necessarily implemented as a List of key, value pairs that you iterate over, even though that's how the semantics are specified, promises do not need to retain references to functions they will never call, even though that's how the semantics are specified.

In other words, there is no observable difference between retaining and not-retaining the function, and so implementations are free to not-retain it. Unobservable differences are outside the realm of specifications and are in the territory of implementations.