Open domenic opened 11 years ago
That's nice, I like the idea of just explicitly saying .uncancellable
. 2 cases that need proper treatment come to mind:
Case 1:
var prom = get('https://foo.com/1')
.then((res) => get('http://foo.com/' + res));
...
prom.cancel();
Is there any circumstance in which the second promise gets properly cancelled?
Case 2:
I think uncancellable
probably isn't quite the right behaviour, I think ideally it would be:
uncancellable() {
return new CancellablePromise(
resolve => resolve(this),
() => {}
);
}
That is to say we should still return a promise that can be rejected with an OperationCancelled exception by calling cancel
. I then think we'd need a new name for uncancellable
since it wouldn't be an apt description of the behavior.
It looks good so far. I'm not thrilled about calling super
in then
that way because it's creating yet another intermediate promise. There could be a better way. But besides that it looks like how I'd implement it.
Your "then" implicitly supports parent cancellation instead of just cancelling the work you added by calling "then".
If I'm understanding your use of super
and resolve
correctly I think you just want:
then(...args) {
return new CancellablePromise(
resolve => resolve(super(...args))
);
}
@skaegi Implicitly supporting parent cancellation is the entire goal: otherwise getJSON(url).cancel()
does not cancel the underlying XHR.
@ForbesLindesay
Case 1
That's an interesting one we should probably make possible and think more about. Good catch.
Case 2
The point of uncancellable
was to revoke the ability of promise consumers to interfere with promise state, so I don't think we want to vend CancellablePromise
s from it.
If that's what you want then write:
function getJSON(url) {
var xhr = get(url);
var result = xhr.then(JSON.parse);
result.cancel = function() {
xhr.cancel();
result.cancel();
}
}
Alternately....
If we want to say that parent cancellation is the default how can I just cancel the operation I added via then
sigh
function getJSON(url) {
var xhr = get(url);
var result = xhr.then(JSON.parse);
var resultCancel = result.cancel.bind(result);
result.cancel = function() {
xhr.cancel();
resultCancel();
}
}
@domenic I see your point re case 2
@skaegi you've just demonstrated the problem. If it's anywhere near that difficult to propagate cancellation people won't bother to do so unless they personally need cancellation in their app, which would mean cancellation was never truely available to consumers of third party libraries. It needs to propagate by default, but propagation needs to be trivial to stop.
Here's a simpler and probably better variation that doesn't override cancel
function getJSON(url) {
var xhr = get(url);
return xhr.then(JSON.parse, function(error) {
if (error && error.name='Cancel') {
xhr.cancel();
}
throw error;
});
}
@ForbesLindesay Our app is very much in the space that needs cancellation and is something we both use and test every day. That absolutely means that we're willing to write code like above when we're writing a "filter" however far and away the more typical case is where we've add a new operation to the chain. Whatever the propagation behavior I want to make sure it's easy to cancel just that operation without impacting the parent and the rest of it's children.
Our Deferred implementation used parent propagation for the last six months and we ultimately removed it and are much happier for it. Now when an operation is not cancelling correctly we can actually track it down instead of trying to understand why seemingly unrelated sibling calls are being rejected. With that said I'm not militantly opposed to the idea and will certainly follow whatever it is we standardize here. It would however really help if I could see a code snippet that showed how to localize the cancel propagation to just the operation then
-ed to the chain.
@skaegi
It would however really help if I could see a code snippet that showed how to localize the cancel propagation to just the operation
then
-ed to the chain.
So your desired semantics are something like this?
get("http://example.com").then(function () {
return get("http://example.org");
}).cancel(); // should only cancel the get to example.org, not example.com
?
Yes that is correct.
Let me try to cover the cases as I see them...
1) For the case where the parent is pending a cancel
call on a promise created by a then
would call its onRejected
handler with a Cancel Error. This then
promise should also be removed as a listener to the parent to prevent an additional callback when the parent completes.
For your example above where the onRejected
handler is undefined, the reject would propagate normally and the call to get("http://example.org")
would never happen at all.
2) For the case where the parent has completed but the onFulfilled/onRejected
handler for the then
promise has returned a promise, the then
promise assumes this state (e.g. as per the promises spec). In that case a cancel
call will occur on this newly assumed promise.
For your example above this is the case where the onFulfilled handler returns the promise from get("http://example.org")
and it is still pending. A cancel
call in this case results in a Cancel Error getting propagated and depending on the implementation the XHR might choose to abort
.
3) For the case where the promise created by a then
is already completed the cancel
call is a noop.
If we replace then
witht eh following, we track all the promises that appear inside of the then
call, so that they are also cancelled when .cancel
is called.
then(cb, eb, ...args) {
var parents = [];
function wrap(fn) {
return function (...args) {
var res = fn(...args);
if (isPromise(res) && typeof res.cancel === 'function') {
parents.push(res);
}
return res;
}
}
return new CancellablePromise(
resolve => resolve(super(wrap(cb), wrap(eb), ...args)),
() => {
for (var parent of parents) parent.cancel();
this.cancel();
}
);
}
The behavior is then:
get("http://example.com")
.then(function () {
return get("http://example.org");
})
.cancel(); // should ancel the get to example.com or example.org
// (depending on which request is currently in progress)
To support @skaegi's request:
Add a method fork
:
fork() {
return new CancellablePromise(resolve => resolve(this), noop);
}
Which can be used:
get("http://example.com")
.fork()
.then(function () {
return get("http://example.org");
})
.cancel(); // should only cancel the get to example.org, not example.com
And equally:
get("http://example.com")
.then(function () {
return get("http://example.org");
})
.fork()
.cancel(); // shouldn't cancel any requests, but still results in a rejected promise with the
// cancellation exception
and
get("http://example.com")
.then(function () {
return get("http://example.org");
})
.uncancellable()
.cancel(); // Truly doesn't cancel anything, does `cancel` even exist here?
To make explanations clearer, I've separated out the promises so each one that gets created has its own name.
var A = get("http://example.com");
var B;
var C = A.then(function () {
B = get("http://example.org");
return B;
});
//some time later...
C.cancel();
Case 2
This seems fine:
Case 3
This also seems fine:
Case 1
All promises are pending. C has not assumed any state.
Without propagation:
then
handlersWith propagation:
Your proposed flow
alternatively
With the use of the fork method we get the behavior @skaegi desires (but with an explicitly created promise to not resolve, rather than having to magic one out of thin air):
var A = get("http://example.com");
var B = A.fork();
var C;
var D = B.then(function () {
C = get("http://example.org");
return C;
});
//some time later...
D.cancel();
case 1
case 2
This seems fine:
case 3
Thanks @ForbesLindesay that all makes sense to me and covers the cases I care about. I personally think both uncancellable
and fork
are not strictly necessary but apart from that I think we're ready for a Draft C.
Excellent, I think we're definitely moving towards agreement here.
The use cases that may necessitate both fork
and uncancellable
:
function asyncMethodA() {
return somethingIDontWantCancelled()
.fork()
.then(somethingThatCanBeCancelledSafely);
}
var A = asyncMethodA();
// Some time later
A.cancel();//should only cancel second part of A
function asyncMethodB() {
return somethingThatINeedToShare()
.uncancellable();
}
var B = asyncMethodB();
//some time later
thirdPartyCodeIShouldBeSuspiciousOf(B);
//some time later
B.then(function (res) {
//do something important
//better hope third party code
//didn't cancel B
});
It would be good to flesh out what we think would be best for these two cases before the next strawman spec.
P.S. I really like @domenic's idea of doing the strawman as just some source code with the appropriate behavior, it's a lot easier to see what's going on sometimes.
Would inlining work? It would require directly creating new promises but I was hoping to minimize API on promise
function asyncMethodA() {
// fork
return new CancellablePromise(resolve => resolve(somethingIDontWantCancelled()), noop)
.then(somethingThatCanBeCancelledSafely);
}
function asyncMethodB() {
// uncancellable
return Promise(resolve => resolve(somethingThatINeedToShare()));
}
Here's a first crack at a draft C https://gist.github.com/4694432
That has nothing in it about propagation
re: That has nothing in it about propagation
@ForbesLindesay Indeed -- I wanted to think a bit more about the child propagation I see in your then
implementation above but then ran out of time. I'd definitely missed that on first read and had thought there was just regular then
promise assumption and did not realize the promise would be wrapped to retain a reference to the "source".
Is the intent there to have a cancel call propagate to the children if the source has already been fulfilled?
Yes, if source
is unfulfilled, propagate to source
if source
is fulfilled, propagate to each child
If source
is fulfilled and then later cancelled I think the resulting state is a bit odd.
What happens when a new child is then
ed to source
?
I believe we would first fulfill
it and then if that call returns a promise cancel
it. The problem there is that it implies that we have introduced this new "cancelled" side state (e.g. a non-pending promise can be cancelled or not cancelled in addition to fulfilled or rejected.) Alternately, we could eliminate this state by saying that the cancel
call propagates to just the set of children at the time of the call but that seems inconsistent.
I buy that we might want this aggressive cancel behavior in some cases and you've shown how one might implement it however this feels to me like one of a range of implementation choices for a cancellable promise.
This might not be popular however I feel the best we can do is say that cancel
attempts to cancel a pending promise, suggest propagation might occur, but then leave it up to the implementation to figure out what that means instead of trying to specify it. Even without being prescriptive on propagation there is still value in the cancel semantics that have been worked out here and we should try to spec it and then stop and see where we are.
I'm pretty sure the idea was that cancellation is just a type of rejection, and that if you're already fulfilled or rejected, cancellation should have absolutely zero impact.
Ah, I see, the original post doesn't reflect that---this._onCancelled
still gets called. I'll edit that.
@skaegi I think you're missing something here and might want to re-read it a bit more carefully.
var source = new Promise();
var outputA = source.then(function () { return childA; });
var outputB = source.then(function () { return childB; });
If source
is pending and we cancel source
, it calls onCancelled
for source
and then rejects source
with a cancellationError. Nothing else happens other than normal rejection propagation.
If source
is fulfilled and then cancel
is called on source
it is a no-op. Literally nothing should happen.
If source
is pending and one of outputA
or outputB
is cancelled, then source
is cancelled via propagation. source
is then rejected with a CancellationException
which causes normal rejection propagation and rejects outputA
and outputB
.
If source
is fulfilled but childA
and childB
are pending and outputA
is cancelled, outputA
propagates that cancellation to childA
and childA
becomes rejected, which leads to outputA
being rejected. outputB
is unaffected and can be resolved once childB
(also unaffected) gets resolved.
Hmm... that sounds exactly right to me (nearly Cancellation axioms) so I think I must be getting confused by the sample code and its use of children/parents
in the then
implementation.
Below is the (translated) code I have in my implementation. Does this do the same thing or is there a case where there are multiple children in that children/parents
array?
then(cb, eb, ...args) {
var _cancel = this.cancel.bind(this);
function wrap(fn) {
return function (...args) {
var res = fn(...args);
if (isPromise(res)) {
_cancel = (res.cancel) ? res.cancel.bind(res) : function(){};
}
return res;
}
}
return new CancellablePromise(
resolve => resolve(super(wrap(cb), wrap(eb), ...args)),
() => {
_cancel();
}
);
}
Regardless, apologies as I think there is no problem here and we're in agreement on a reasonable default propagation mechanism.
The other issue I wanted to cover was onCancel
. In the current Draft C if onCancel
throws we reject with that exception instead of the CancellationError.
Is the intent there to give the onCancel
call a measure of control over how the promise is rejected or is it to help with error handling that occurred in 'onCancel'. If it's the former I think we need a use case to show why this is helpful because being able to consistently distinguish a CancellationError (and potentially propagate and handle it further) is really useful. If it's the latter then I think this should instead be handled in an implementation specific manner as again giving code further down the chain a chance to handle Cancellation is useful.
Looks equivalent to me. I think you're correct that children
is only ever a single element array.
For specification purposes what if we didn't have onCancel
at all and stipulated that cancel
always throw a CancellationError. One could then write the minimal implementation as follows...
class CancellablePromise extends Promise {
constructor(fn) {
super(fn);
}
cancel() {
// Ideally... if (pending) return
var cancellationError = new Error();
var cancellationError.name = 'Cancel';
this._reject(cancellationError);
}
then(cb, eb, ...args) {
var _cancel = this.cancel.bind(this);
function wrap(fn) {
return function (...args) {
var res = fn(...args);
if (isPromise(res)) {
_cancel = (res.cancel) ? res.cancel.bind(res) : function(){};
}
return res;
}
}
var promise = new CancellablePromise(
resolve => resolve(super(wrap(cb), wrap(eb), ...args)
);
promise.cancel = function() {
_cancel();
);
return promise;
}
}
A promise implementation "could" of course add an 'onCancel' to their constructor however this would be a convenience and just sugar for:
cancellablePromise.then(null, function (error) {
if (error.name === 'Cancel') {
onCancel();
}
})
The point of onCancel
is to allow the underlying asynchronous operation (e.g. an Ajax request) to be cancelled. The minimal implementation above doesn't seem to have that ability, making it pretty useless I would think.
Here's an example of what I'm thinking
function makeCancellableRequest(url) {
var xhr = new XMLHttpRequest();
var xhrPromise = // hookup resolver to xhr
xhrPromise.then(null, function (error) {
if (error.name === 'Cancel') {
xhr.abort();
};
});
return xhrPromise;
}
I'm not saying an implementation wouldn't provide onCancel
as a convenience.
I'm beginning to write tests now and noticed for external behavior I could replace the need for onCancel
with a `catchCancellation/handleCancellation' block. That made me wonder if this is something that we really need to stipulate in the specification.
Interesting... nice illustration.
Here's a gist to my Cancel tests -- https://gist.github.com/skaegi/4736504 . I just put the contents in CancelTests.js next to https://github.com/promises-aplus/promises-tests/tree/master/lib/tests and ran.
One thing I found that is relevant to the spec is that the cancel
call to the parent in a "then promise" must happen after the original cancel
call returns. This has to be done for cases like...
var apromise = fulfilled().then(function() {return anotherpromise;})
apromise.cancel();
... otherwise anotherpromise
does not get a chance to "assume" apromise
before the cancel
call happens because the then
contents are not fulfilled until the next tick. The two last tests from that gist above exercise the fulfilled and rejected variation of that case.
For the minimal implementation that means that instead of:
var _cancel = this.cancel.bind(this);
one should use...
var thisCancel = this.cancel.bind(this);
var _cancel = function() {
setTimeout(thisCancel, 0); // or nextTick etc.
};
I've updated my tests (https://gist.github.com/skaegi/4736504) to include an additional case where there was more than one fulfilled and rejected link in the promise chain before getting to a pending promise. For example:
var promise = fulfilled().then(function() {
return fulfilled();
}).then(function() {
return pending();
}).then(assert.fail, function(reason) {
assert.ok(reason.name==='Cancel');
done();
});
This helped show a problem in the _cancel
implementation above where the thisCancel
does nothing because the parent promise has already been settled. Instead the implementation needs to check for the situation where _cancel
has been re-assigned in which case it should call it instead of the original.
var thisCancel = this.cancel.bind(this);
var propagateCancel = function() {
setTimeout(function() {
var cancel = _cancel === propagateCancel ? thisCancel : _cancel;
cancel();
}, 0);
};
var _cancel = propagateCancel;
For whatever it is worth, this is how I currently abort() a promise. Requires the use of promise.attach() from the originator i.e an ajax wrapper.
Promise.prototype.attach = function(handle) {
/* Attaches a handle (typically) to the promiser. */
/* In an Ajax scenario this could be the xhr object */
this.attached = handle;
return this;
}
Promise.prototype.abort = function(message) {
/* notifies the attached process */
if(this.attached && typeof this.attached.abort === 'function')
this.attached.abort();
this.reject(message);
return this;
}
And timeout triggers abort().
Promise.prototype.timeout = function(time,func) {
/* null cancels timeout */
if(time === null) {
clearTimeout(this._timer);
return this;
}
/* fulfilled or rejected */
if(this._state) return this;
/* trigger abort on timeout */
if(!func) func = function() {
this.abort("timed out");
}
this._timer = setTimeout(func.bind(this),time);
return this;
}
Inspired by https://github.com/promises-aplus/cancellation-spec/issues/1#issuecomment-12377406, what about something as simple as this? Assume underscored properties are "actually private," i.e. wouldn't be specced and are only here to illustrate. Using ES6 for brevity.