-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
3.0 cancellation overhaul #415
Comments
Summoning some relevant people - @kriskowal @domenic @spion |
I agree that current cancellation is bulky and strange so a big +1 on overhauling it - it's a constant source of confusion for people in SO and it's not very intuitive IMO. @kriskowal raised the argument that cancellation is inherently impossible. Here is the original discussion I tend to agree that the major source of confusion is the dependency graph. The most basic confusing case is:
The way propagation works in this scenario is not obvious when you cancel I'm asking why (like progression) not do this at the user level? Why do we need a special construct? function delay(value, time, token) {
var timerId;
return new Promise(function(resolve, reject) {
timerId = setTimeout(function() {
resolve(value);
}, time || value);
token.cancel = function(){
clearTimeout(timerId);
reject(new CancellationError());
};
});
}
var token = {cancel: function(){}};
var after500ms = delay(500, token).catch(CancellationError, function(e) {
console.log("Your request took too long");
});
delay(250).then(function() {
token.cancel();
}); This way anyone holding the token can cancel so who has the ability to cancel is explicit. It seems like most times promises require cancellation of the direct problematic promise (the HTTP request, the timeout etc) - if we're not creating new primitives with single-consumer can't we give up all of propagation? This is what languages like C# or Scala do (for example: http://msdn.microsoft.com/en-us/library/dd997396%28v=vs.110%29.aspx ) although I have to admit I'm not a fan of what C# does to be fair. |
I tend to agree with Benjamin (when do I not?), this seems like an awfully lot of complexity to add to something that can be relatively simply implemented in userland. |
There has been many issues posted about cancellation, literally none about multiple consumer afaik. Anyone can call Progress is simply a callback that doesn't even need error handling (and if it does there was no good way to do it even when integrated in promises, what bluebird does when progress handler throws is absolutely horrible). However there are multiple issues with the token:
|
Excuse me for bumping in, but I would support Petka's implementation with the
This is, essentially, the very same It is ok to do it once, maybe twice. But if the application is complex enough, one has to do it all the time, constantly minding the hierarchy of collections stored. Let alone creating some heavy-loaded apps, which require stuff like Mozilla's |
By issues I was mostly referring to github but I also checked stackoverflow now. https://stackoverflow.com/questions/27479419/bluebird-promise-cancellation - No idea what they were trying to do here but seems to be related to the first issue of having to use .catch to attach cancellation hooks.. although they aren't even using that hook to cancel anything (Awkward API for attaching cancellation hook +1) https://stackoverflow.com/questions/27132662/cancel-a-promise-from-the-then - Needed to use throw instead of cancel (Also trying to compose with .props which wouldn't work, collection composability +1) https://stackoverflow.com/questions/24152596/bluebird-cancel-on-promise-join-doesnt-cancel-children - Join doesn't compose (collection composability +1) https://stackoverflow.com/questions/23538239/catch-a-timeouterror-of-a-promise-beforehand - (Awkward API for attaching cancellation hook +1) So that's only 4 out of 203 related to cancellation, and all are at least related to the top 3 issues posted in the OP. I only checked the |
I've seen it on StackOverflow several times - it's not about encapsulation at all it's about propagation not being simple to reason about. What happens when you As for tokens:
Yes! This is the point. I'm saying I think I don't want them to compose since if you want to cancel something you just need a reference to the original promise's token. This circumvents the hierarchy problem.
Why can't they throw Bluebird's
Yes, this is pretty weird but then again it's also how other languages do it. The reason it is passed to the function (rather than let's say - monkey patched) is that it is conceptually not the concern of the returned promise but the concern of the action itself (a promise just represents an eventual value - the function producing it is the one concerned with cancellation and progression). I think putting cancellation on the promise itself is definitely complecting it. Also here are more questions: https://stackoverflow.com/search?q=%5Bbluebird%5D+cancel+is%3Aquestion
This thread is here for feedback and discussion - if people weren't bumping here there would be no point :) How would you expect aggregation to work with cancellation in general? How would |
You don't typically actually have multiple consumers for any promise when using |
@benjamingr |
So What about:
This would cancel every promise in the race except for the one that resolved? What about:
Would it cancel all promises except for p1 except for those that did not resolve first? What about:
Do you think users will understand this is a race condition between p2 and p3? I'm just not convinced this is easy to reason about for the general collection method case - kind of like progression. Cancelling an operation itself seems orthogonal to the returned promise for the value. Also - how would you cancel mid-chain? The whole Right now the user has to handle aggregation of cancellations manually - my argument here is that there is no generic way to always aggregate cancellation in a way the user would find predictable. With the token approach you'd have something like:
Now, we can cancel all the requests with
However since we did this little wiring ourself we can do all this wiring to match what we actually want - cancel all of them, or just one, or maybe the last two - it's in our side and it's not as ugly as in your example at all :) |
@UndeadBaneGitHub What problem with composition is not solved? Cancelling a promise which is a promise for the result of any collection method will cancel the input promise-for-array if it's still pending. Otherwise cancelling a promise which is a promise for the result of Otherwise cancelling a promise which is a promise for the result of Otherweise cancelling a promise which is a promise for the result of |
var p = Promise.race([p1, p2, p3]);
p.then(function(){
p.cancel();
}); That's a no-op, since var p5 = Promise.map([p1, p2, p3, p4], function(elem){
p5.cancel();
}); Cancel will be no-op after the first call, but most likely the cancellation hook will reject an input promise which causes the whole map to reject and no other elements will be then iterated. p1 = getCancellablePromise();
var p2 = p1.then(doSomethingElse);
var p3 = p1.then(doSomething).then(function(){ p1.cancel(); }); No-op since |
That makes sense although it's very common to want to cancel all other requests once one has resolved in races or
That's kind of scary given that iteration might have side effects in peoples' code.
Still if you call |
That's not related to cancellation though since any promise can also reject naturally |
Which is ironic because it's very rare to need |
Now this makes me kinda concerned, as it might look like this:
This looks to me like a race condition with completely unpredictable behavior. What if |
Replace |
Fair enough, but I guess, this is a matter of what you want |
Well it doesn't have to be |
By its very nature We don't use the feature in our code, at all. In most of the cases we've encountered using it is just premature optimization. In other cases, the problem is better solved with streams. In the very few cases where its really necessary, we split up the operation into multiple atomic actions and race them with a rejectable (unresolved) promise which serves as a way to interrupt the execution. |
I tend to agree with @spion - in general we use |
Well, I use
And This sounds to me like a more or less predictable behavior (yes, still a race condition and you don't know whether |
@UndeadBaneGitHub thats a rather interesting interaction between I know that |
@spion I would really appreciate, if you wrote the code in a bit more detail.
then |
Its var p5 = Promise.all([p1,p2,p3,p4]).then(function(list) {
return list.map(f);
}).all();
lengthy.then(function() { p5.cancel(); }); |
Every C# person I talked to is against conflating cancellation with promises directly. I do think that the library should help users with that but perhaps it is best if it is not a part of the promise itself. F# passes the token implicitly - I'll look into it. |
We've had a discussion about this internally, and we came up with the following:
So in short, we support both @benjamingr and @petkaantonov 's suggestions in that we like the proposal for the new cancellation, and simultaneously, that real-world problems often need something custom. |
@rogierschouten thanks a lot for taking the time to do this and for the feedback! So in your use cases propagation was not necessary? Just making sure we're on the same page:
|
@benjamingr You're right I meant composition. Propagation is necessary. |
@rbuckton answering about my POV on your benefits ...
Last, if you do like the idea of having an additional method on the Promise prototype for observing the cancellation then all this decoupling you talked about would be even more messed up 'cause you might realize it is cancelable, and nobody gave you that power, holding who knows where, and in how many, that "big red button" My 2 cents |
I spent quite a while looking into this the other day and as somebody new to coffeescript/nodejs but with a lot of C# experience I'm pretty dead-set on have a good async workflow. Unfortunately when it came time to use cancelling to stop a coroutine I fell into a quagmire. I need to be able to treat async functions(coroutines) as promises so I can yield them or bundle them up as required but for the life of me couldn't figure out any cancellation support on them. For what it's worth, I found somebodies cancellation(tokens) package and it has worked fine for me; I'm off to the races and dev moves forward. |
I didn't see this issue, and wrote this issue: #663 (comment) @WebReflection What happens if cancellation occurs after a couple var yourProcess = new Promise(function ($res, $rej, ifCanceled) {
var internal = setTimeout($rej, 1000);
ifCanceled(function () {
clearTimeout(internal);
});
})
.then(function () {
console.log('on time');
yourProcess.cancel({because:'reason'})
})
yourProcess.catch(function () { // does it stop here? I feel like it should
console.log('did i catch it?');
//yourProcess.cancel({because:'reason2'}) // what if cancellation happens here instead?? Is it too late?
})
.then(function (value) {
console.log(value);
}); I think for cancellation to be appropriately powerful, you need to somehow define all the individual promises that should be cancelled - identifying just one promise and then propogating that to all ancestors or all descendants doesn't cut it. The easiest way to define a list of all the individual promises you're likely to mean is to define a range - say that all promises between A and B are cancelled. If the way you do this is to define a new Promise chain and call cancellable on it, I think cancellation ranges could be pretty easy to define. Example: var A = new Promise(function(ra,ta,ifCanceledA) {
ifCanceledA(function() {
console.log("A cancelled")
})
var B = new Promise(function(rb,tb,ifCanceledB){
// event 1
ifCanceledB(function() {
console.log("B cancelled")
})
}).then(function() {
console.log("B done") // event 2
}).cancellable()
return B.catch(function(e) {
return "B's cancellation caught" // event 3
})
}).then(function() {
console.log("A done") // event 4
}).cancellable() There are a few different important scenarios: So in steps III, IV, and V, I think this is simpler than a cancellation token and yet just as powerful (if not more so). Thoughts? |
@fresheneesz I think you'll want to read #565. Things like not propagating cancellation if there are other callbacks waiting for a promise are already implemented :-) |
@bergus What part of my post are you addressing? My proposal there covers a lot more than just that. |
@fresheneesz ...
cancellation should be possible at any time for chainability reason which is part of the strength of Promises ( If already canceled, nothing happens, you keep going as you would have done in any case. This is inevitable unless you want to expose Since it's the author of the Promise to decide its cancelability, whoever want to return a cancelable promise in the chain can simply do it, canceling the external promise when its new one with its new cancelability is invoked. var A = new Promise(function(ra,ta,ifCanceled) {
ifCanceled(function() {
console.log("A cancelled")
});
var B = new Promise(function(ra,ta,ifCanceled) {
ifCanceled(function() {
A.cancel();
console.log("B cancelled")
}); at that point you can simply pass B around ... I think this a very simplified solution but for all use cases I could think of it should just works. |
Do you mean the very first cancelable promise in the chain's ancestry, or do you mean its descendancy? It would be much clearer for me personally if you could address the specific example I brought up.
And how do you program something if you want to only cancel that internal promise, and not the external promise? This is what my proposal addresses. |
sorry, ancestry of course, you can decide to pass around a cancelable Promise you should never be able to cancel Promises you don't own or didn't receive ... so ancestry or nothing.
You wrap it through a cancelable Promise as my example does ... it passes around B that once canceled can cancel A too. Whoever receives a Promise, receives B, and will be unable to directly cancel A. |
Makes sense
In the last example you posted, A and B aren't related in any way except that if B is cancelled A is cancelled. I'm much more concerned with promise chains, not individual promises. I realize I'm joining this discussion late, and I don't want you to repeat everything you've already said just so I can understand, but you mentioned you wrote "3 proposals", which I assume are API proposals, and I can't find them either in the esdiscuss.org link that petka gave, or in this issue comment thread. Is there a current work-in-progress proposal we are discussing? Also, do you understand the proposal I put forth? What are the shortcomings you see in it? |
One thing I just thought about, if you have some conceptual processes X, A, and B, where A and B are parts of X, like this:
Cancelling X should cancel all of process X, including processes A and B. But if what a cancellation does is create an exception that propogates, something inside A might catch and "handle" that cancellation, so that part of process A and all of process B actually continues. This isn't what you would want in a cancellation right? It looks like some people in this thread have balked at having a third state ("cancelled"), since it wouldn't match spec, but that seems like the cleanest way to handle it. You don't want some unknown inner process catching the CancellationException and overturning the cancellation. How else would you get around this without having a 3rd state - the cancellation state? |
@fresheneesz you cancel what you want to cancel and what you have access to or what you create as cancelable. You should really think it at that simple logic, and no magic involved. whatwg/fetch#27 (comment) but you should really probably take your time and read the entire thing there and not just my opinion or code examples. Anyway, I'm really done here because there's nothing more I need to add or to understand and mostly everything has been told already. I'm every day more convinced Promises are just the wrong solution for anything asynchronous that might need to be dropped without needing to throw an error. Promises are great for small/quick things, like Promises are also great for server side tasks where you want that your full chain of asynchronous actions is executed from start to end without problems ... no interference there meant, wanted, or needed. However, if you use Promises for chains that involves unpredictable amount of time to execute you'll realize that patterns like It's not an error, it's just a little change to the very same initial action of reaching the floor, user shouldn't need to start the action again or be notified with an alarm that an error occurred. We should never forget the importance of the time variable and we also don't have crystal balls. If you need to change an action that is taking time to execute you, as developer, should be able to do that. This is how pragmatic I believe this matter is, this is how Internally, indeed, every Promise can be somehow "aborted", but I guess we just like playing philosophy on user-land and we are also often those very same that create problems to ourselves ^_^ Well, at least it's never boring here, but I'm a bit tired of this topic and I simply avoid Promises whenever it makes sense doing it. I do hope this entire story taught us at least that using Promises for everything asynchronous is a bloody footgun and that events still have many valid uses cases and applications that should probably never be replaced with Promises .... or let developers wrap them when it's convenient, so that everybody wins, and everyone can control "the flow". Best Regards, please contact me privately if you'd like to discuss more. I'm off this thread now. |
@WebReflection I think that BB 3.0 cancellation semantics may change your mind once they're released. We'll see... |
There are two points:
There is no such thing as "overturning" because the descendant chain is already cancelled. |
@bergus except |
@WebReflection Thanks for the list of examples, that helps a lot! I definitely agree that events have a completely different use case than promises, and any case where you have an event happening multiple times, promises don't help you there. In any case, I think I understand a lot more about what you said about it being more simple. @bergus Yeah, i was actually thinking of a case where you define different cancelable processes in the same chain like you wrote up on march 16th, like But cancellation is complicated, and there are a couple cases I want to bring up:
Are these cases kind of in line with how bluebird's v3 cancellation is currently being designed? |
@fresheneesz Thanks for your input, yes cancellation is complicated :)
It is - supposed that A is fulfilled and B has not already settled.
Yes, if B is used elsewhere (and has other uncancelled callbacks attached to it), it won't be cancelled. Only the callback that resolves the
Not as complicated as this. You just would do
x is automatically dependent on B as soon as A fulfills and the callback returns B. At that point, B can't be cancelled until x is cancelled.
Indeed it is.
Not sure what you mean. If there are still the steps 6-10 depending on the promise 5, you cannot simply cancel that. You have to cancel promise 10 first.
Again, there is no error to be caught when you cancel anything. The only error you will receive is the one when you
Yes, A isn't cancelled, it's still needed for y. Unless you "forcibly" cancel A (not sure whether or how that may be possible), but in that case |
@bergus Thanks for the responses!
Yes I meant "you can cancel [the promise for step 10] on step 5, and only the cancellation callbacks .. for steps 6-10 will be called."
So lets say you have In any case, sounds like 3.0 cancellation will be pretty great! Would love to see the docs for the proposed/WIP 3.0 cancellation API. |
Yes indeed, if the promises up to step 5 are already settled, then they are no more cancelled.
If we have |
@bergus Thanks for the explanation. Sounds like its everything I'd want in cancellation! |
Fixed in 3.0 release |
Nice! I see there still are many documentation holes to fill. |
Congrats! |
I just left the docs partially undone because otherwise 3.0 would never be published :D |
The current cancellation seems to be the most problematic feature due to some annoying limits in the design:
Promise.all(...).cancel()
cannot do the obvious thing and also callcancel()
on the promises in the array).cancel()
is asynchronousSince all consumers and producers must be trusted/"your code", there is no reason to enforce that cancellable promises are single-consumer or create new primitives.
Edit: The below design has been scratched, see #415 (comment)
In the new cancellation you would register the callback while marking the promise cancellable:
- Throwing an error will reject the promise with the error as the rejection reason
- Returning a value will fulfill the promise with the value as the fulfillment value
This returns a new cancellable promise (unlike right now which just mutates existing promise which causes a lot of headache internally) and the flag will automatically propagate to all derived promises. However, the reference to the
onCancel
callback will only be held by the new promise created by.cancellable()
.Calling
.cancel()
on a cancellable promise will keep propagating to cancellable parents and calling theironCancel
callbacks.The
onCancel
callback will receive the cancellation reason as its only argument (the default is aCancellationError
instance).From the callback it's possible to decide the fate of the promise:
(However I guess 99.99% of the time you want
throw reason
, so this flexibility adds a lot of inconvenience?)Bad (should use .timeout() for this) example of usage:
The text was updated successfully, but these errors were encountered: