Open ForbesLindesay opened 11 years ago
Is it canceled
or cancelled
? Both are valid but we should be consistent.
Does the CancellationError
really need a .cancelled
property?
Does propagateCancel()
propagate the reason
and data
? Why not simply call cancel()
instead? Having a method publicly available on a promise but saying it's "not for external use" seems weird.
It must return a promise for the result of calling any cancellation handlers attached to the resolver.
This is strangely worded. I assume you mean to say that the resolver that's behind the promise we invoked .propagateCancel()
on has handlers for that cancellation, and those may throw?
When a promise is cancelled ( directly or indirectly) the resolver for that promise must emit a 'cancel' event.
What does this event mechanism look like? How are the handlers allowed to affect the resolver?
E.g.
resolver.onCancelled = function(reason, data, resolver){}
resolver.onCancelled = function(rejectionReason, resolver){}
resolver.on("cancelled", function(reason, data){}
etc.
Do we need both reason
and data
? Allowing reason
to be any value, and setting it as .reason
on the error should be enough?
I don't mind which, I come from the UK but (as much as I dislike it) we should probably use the US spelling, whatever that is.
I think we can drop .cancelled
in favour of name, we only really need one of the two.
.propagateCancel()
doesn't need to propagate the reason or the data because that reason and data would not be visible to anyone. It is for use by other promise libraries when the promise is assimilated by being returned from then or used in all or whatever. It's required so we don't reject promises when cancellation propagates. If we didn't do this then lots of rejection handlers would fire, and do work, and potentially then need to be cancelled straight after starting.
This is strangely worded. I assume you mean to say that the resolver that's behind the promise we invoked
.propagateCancel()
on has handlers for that cancellation, and those may throw?
Yes, that's what I mean.
What does this event mechanism look like?
I was to be honest thinking something more like:
resolver.on('cancelled', function () {
}
It shouldn't actually affect the resolver in any way at all, it's only there to cancel the underlying operation. The resolver/promise combo handle behaving correctly when cancelled, but they can't stop the now un-necessary work from continuing to be done. By terminating connections to web-servers, for example, lots of wasted resources might be freed up.
The advantage of having reason and data is that we can turn the result into an exception with reason as the message, but if data is used it could be an object and that wouldn't work as well. It's a good point though, and the solution presented here is probably sub-optimal.
I don't mind which, I come from the UK but (as much as I dislike it) we should probably use the US spelling, whatever that is.
Ha, that's the response I got in London when talking about Dojo's API. I picked canceled
since it's shorter, which makes it easier to remember which to use. It feels though like more people naturally use cancelled
, but then I'm a non-native English speaker based in the UK.
.propagateCancel() doesn't need to propagate the reason or the data because that reason and data would not be visible to anyone. It is for use by other promise libraries when the promise is assimilated by being returned from then or used in all or whatever. It's required so we don't reject promises when cancellation propagates. If we didn't do this then lots of rejection handlers would fire, and do work, and potentially then need to be cancelled straight after starting.
Oh! You mean that the upstream promise transitions into a canceled
state but does not fire any onRejected
handlers? The promise that was .cancel()
ed however does get rejected?
What would happen to other promises that branched of the upstream one? I presume they also get silently canceled, which means they won't realize and can't clean up any associated state.
You could reject all promises but then onRejected
handlers need to check if the error received was an OperationCanceled
error, which is doable but tedious.
Regarding using a .on()
interface for events, while it does look nice syntactically it means we have to drag event systems into resolvers… how for example would you remove a listener? Add multiple listeners? A simpler solution is to look for a onCanceled
property on the resolver. One could use AOP to add multiple listeners.
The advantage of having reason and data is that we can turn the result into an exception with reason as the message, but if data is used it could be an object and that wouldn't work as well.
In my implementations .cancel()
took a reason
value which was passed to a canceler
function on the resolver, which could then fulfill or reject the promise. There was no generic OperationCanceled
error but instead the reason
would come back down the promise chain.
I like the generic OperationCanceled
error that enables you to separate cancelation from other rejection reasons. It's also important to discern the cancelation reason, ideally by doing a string comparison (e.g. error.reason === "InboundRequestSocketClosed"
). I'm not sure whether message
is the right field for that. It seems better though than for example error.data.type
.
Hmmm, maybe we do need to mandate some kind of reference counting so we don't get the situation described. Perhaps each call to .then
after the first one should increase a counter. Each call to .cancel
should decrement the counter and each promise resulting from a call to .then
can only decrement the counter once. We only actually cancel the promise then once the counter gets to 0.
Yeh, onCanceled
would be fine
I just think that because it might end up getting logged like an exception sometimes, we should make sure it's possible to set the message to something helpful.
Hmmm, maybe we do need to mandate some kind of reference counting so we don't get the situation described.
I assume you mean having branched promise chains not being affected if one of them gets canceled? I may experiment with reference counting tomorrow. What would happen if two branches are canceled? The first cancelation won't immediately reach the resolver, but when the ref count drops to 0 at least one of them has to. I suppose the first could win?
I just think that because it might end up getting logged like an exception sometimes, we should make sure it's possible to set the message to something helpful.
True, but that's immediately relevant for control flow purposes. Perhaps .cancel(type, data, message)
, all three being optional?
Yes that's what I meant. I don't think we should provide any info except that it's been cancelled to the resolver, so it doesn't matter which one wins.
Message and data should be sufficient. You can always set data to be a string.
I've added an updated draft at https://gist.github.com/4399269, not sure whether this should already be Draft B. It covers a few edge cases found whilst doing the implementation, see https://github.com/novemberborn/legendary/tree/cancelation. Still need to write some tests…
Is there a way to do this without a public propagateCancel
method? E.g. could calling cancel
be made to work?
I dislike promise.cancel(data, message)
. Is it necessary to have any communication at all, other than that a cancellation occurred? WinJS doesn't. If anything I'd prefer promise.cancel(options)
which ends up calling Object.assign(theCancellationError, options)
. So you could stuff message
, data
, whatever in there.
When
propagateCancel
is called, the promise transitions into an extracanceled
state.
This extra cancelled state seems bad. When directly cancelled, you just transitioned to the rejected state. Am I unable to observe that rejection, since now we're no longer in the rejected state? What was the point of rejecting then?
An
onCanceled
callback can be added to the resolver.
I don't think I like this mechanism, but I admit I can't think of much better. Maybe it becomes new Promise(functionCalledWithResolver, onCancelled)
?
The expected
this
value inside the callback is left unspecified.
Ick, I don't like this. We should have specified it to be undefined
in Promises/A+.
Conversely, a resolver can only be indirectly canceled if that cancelation comes from the only promise that depends on it.
I am very leery of this kind of behavior that relies on a promise knowing who depends on it. It defeats programs that set up promise dependency chains after some time, and thus defeats using promises as first-class objects. See related discussions in the unhandled-rejections repo.
Furthermore, none of this seems to address the communications channel issue. Promises need to be by-default uncancellable; that is, the cancel
method should be a no-op unless the promise was specifically constructed to be cancellable. (And propagating cancellation should not be able to override this.) I would imagine that if no onCancelled
callback is provided then cancel
is a no-op.
Is there a way to do this without a public propagateCancel method? E.g. could calling cancel be made to work?
The way we've defined cancel
is to directly cancel that promise and all that derive from it. propagateCancel
is supposed to signal to the resolver that a promise was canceled, and only if that cancelation has no negative side-effects can the resolver itself be canceled:
var A = fulfilled();
var B = pending().promise;
var C = A.then(function(){
return B;
});
var D = B.then(function(){
…
});
C.cancel();
Here canceling C
shouldn't cancel B
, because then D
would end up canceled. However if D
didn't exist, B
could safely be canceled (at this point, more below).
I dislike promise.cancel(data, message). Is it necessary to have any communication at all, other than that a cancellation occurred? WinJS doesn't.
I've found it useful, especially since the rejection travels down the promise chain.
If anything I'd prefer promise.cancel(options) which ends up calling Object.assign(theCancellationError, options). So you could stuff message, data, whatever in there.
Oh that's good! But then should you be able to override the name
property?
When propagateCancel is called, the promise transitions into an extra canceled state.
This extra cancelled state seems bad. When directly cancelled, you just transitioned to the rejected state. Am I unable to observe that rejection, since now we're no longer in the rejected state? What was the point of rejecting then?
With the implementation I did at this point, the resolver is only canceled if cancelation was propagated from its only promise. Therefore there is no rejection to observe. However if you call then()
again, that promise will be rejected with a generic cancel error. In other words I don't think there's any specific need for a canceled state.
An onCanceled callback can be added to the resolver.
I don't think I like this mechanism, but I admit I can't think of much better. Maybe it becomes new Promise(functionCalledWithResolver, onCancelled)?
I like that, and it's closer to what e.g. Dojo has done up to now, with new Deferred(canceler)
. It also makes it easier to opt-in to cancelation support when the promise is created, lessening the performance hit.
The expected this value inside the callback is left unspecified.
Ick, I don't like this. We should have specified it to be undefined in Promises/A+.
Agreed. And I suppose most implementations will invoke the callback that way.
I am very leery of this kind of behavior that relies on a promise knowing who depends on it. It defeats programs that set up promise dependency chains after some time, and thus defeats using promises as first-class objects.
What we're trying to avoid is having cancelation on one promise affect other promises that stem from the same resolver. The way to do that is to only cancel the resolver when its last promise is canceled. If more promises are derived at a later point those would be rejected because the resolver was rejected. Alternatively cancelation could be advisory only at the resolver level, but I'm not sure whether that would solve the problem.
Furthermore, none of this seems to address the communications channel issue. Promises need to be by-default uncancellable; that is, the cancel method should be a no-op unless the promise was specifically constructed to be cancellable. (And propagating cancellation should not be able to override this.) I would imagine that if no onCancelled callback is provided then cancel is a no-op.
I like that. However if Promises/A+ defines a promise as having a then
method, I'm not sure cancel
should be a no-op. Surely it just wouldn't exist?
(I'd prefer it to be a no-op though, testing for .cancel
is annoying.)
@domenic I've implemented your suggestions and opened #5 for discussion.
Made cancel
a no-op by the way, it was one for the result of then().then()
anyway.
Very early draft of Cancellation Spec, I think it's probably too early to do a great job, but hopefully this will create some interesting talking points.
Cancellation/A+
This proposal specifies how cancellation is triggered, handled and propagated in a Promises/A+ promise library.
Terminology
In addition to the terminology from promises/A+ we use the following:
Requirements
The
CancellationException
When a promise is directly cancelled it is rejected with a CancellationException. A CancellationException MUST obey the following points:
cancellationException instanceof Error === true
).name = 'OperationCancelled'
.cancelled = true
The
cancel
MethodWhen the
cancel
method is called on a promise it is directly cancelled. The cancel method accepts two arguments.CancellationException
reason
anddata
are optionalreason
is not a string it SHOULD default to'Operation Cancelled'
.data
isundefined
it SHOULD be ignored.reason
is a string it is used as themessage
property in theCancellationException
data
is notundefined
it is set as thedata
property of theCancellationException
this.propagateCancel()
and return the result.The
propagateCancel
methodThe propagateCancel method is intended only to be called by this and other promises, it is not for external use.
When propagateCancel is called, the promise transitions into an extra
cancelled
state. This does not trigger any events. It does however mean that the promise can never be resolved (i.e. it never leaves the cancelled state)..propagateCancel()
on the promise it is waiting for.In most cases it should call
.propagateCancel()
on the promise it's waiting for. The exception is if the promise has been in some way 'forked', when it may choose not to in an implimentation specific way.The
cancelled
EventWhen a promise is cancelled ( directly or indirectly) the resolver for that promise must emit a
'cancel'
event.