Skip to content
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

Draft A #2

Open
ForbesLindesay opened this issue Dec 22, 2012 · 11 comments
Open

Draft A #2

ForbesLindesay opened this issue Dec 22, 2012 · 11 comments

Comments

@ForbesLindesay
Copy link
Member

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:

  1. "CancellationException" is an error used to reject cancelled promises
  2. "direct cancellation" is when a promise is cancelled by the consumer of the promise by calling '.cancel'
  3. "indirect cancellation" is when a promise is cancelled as the result of another promise that was waiting on it being directly or indirectly cancelled.

Requirements

The CancellationException

When a promise is directly cancelled it is rejected with a CancellationException. A CancellationException MUST obey the following points:

  1. It must be an instance of error (cancellationException instanceof Error === true)
  2. It must have a property .name = 'OperationCancelled'
  3. It must have a property .cancelled = true

The cancel Method

When the cancel method is called on a promise it is directly cancelled. The cancel method accepts two arguments.

promise.cancel(reason, data);
  1. The promise MUST be rejected with a CancellationException
  2. Both reason and data are optional
    1. If reason is not a string it SHOULD default to 'Operation Cancelled'.
    2. If data is undefined it SHOULD be ignored.
  3. If reason is a string it is used as the message property in the CancellationException
  4. If data is not undefined it is set as the data property of the CancellationException
  5. The promise must call this.propagateCancel() and return the result.

The propagateCancel method

The 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).

  1. If the promise is waiting on another promise to complete it may call .propagateCancel() on the promise it is waiting for.
  2. It must return a promise for the result of calling any cancellation handlers attached to the resolver.

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 Event

When a promise is cancelled ( directly or indirectly) the resolver for that promise must emit a 'cancel' event.

@novemberborn
Copy link

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.

@novemberborn
Copy link

Do we need both reason and data? Allowing reason to be any value, and setting it as .reason on the error should be enough?

@ForbesLindesay
Copy link
Member Author

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.

@novemberborn
Copy link

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.

@ForbesLindesay
Copy link
Member Author

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.

@novemberborn
Copy link

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?

@ForbesLindesay
Copy link
Member Author

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.

@novemberborn
Copy link

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…

@domenic
Copy link
Member

domenic commented Dec 29, 2012

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 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?

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.

@novemberborn
Copy link

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.)

@novemberborn
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants