zenparsing / es-cancel-token

Cancel Tokens for ECMAScript
44 stars 1 forks source link

Simple cancellation is awkward #3

Open zenparsing opened 8 years ago

zenparsing commented 8 years ago

Cancel tokens seem to work really well for complex scenarios, but for really simple use cases, the API is awkward for the user. It takes too many lines of code to do the simple thing.

let cancel;
let token = new CancelToken(c => cancel = c);
fetch("some-data.json", token).then(response => {
    // ...
});
cancel();
domenic commented 8 years ago

This might help slightly.

const { cancel, token } = CancelToken.pair();
fetch("some-data.json", token).then(response => {
  // ...
});
cancel();

A better name than pair would be good of course.

dtribble commented 8 years ago

Unlike promise/resolvers, there is little reason to prevent getting the token from the source. Thus, the code may simpler with:

let source = new CancelSource();
fetch("some-data.json", source.token).then(response => {
    // ...
});
source.cancel();

I like cancel being a function (as above). there may be a win in having more operations on them (like merging sources or linking a source to another cancellation token). If so, then this approach works well.

dtribble commented 8 years ago

Just to be clear, I'm not proposing merging the cancel source and the cancel token. the latter suggestion is that you could get the token from the source.

zenparsing commented 8 years ago

@dtribble Ah, that makes sense!

benjamingr commented 8 years ago

C# does what @dtribble is proposing.

The major reason IMO the revealing constructor pattern makes sense for cancellation is synchronous throws in the function the promise constructor is aiming to "promisify". Do we have a similar issue here?

zenparsing commented 8 years ago

The major reason IMO the revealing constructor pattern makes sense for cancellation is synchronous throws in the function the promise constructor is aiming to "promisify". Do we have a similar issue here?

Did you mean "promises" instead of "cancellation" above?

I don't think there's a similar issue here.

benjamingr commented 8 years ago

Yes, sorry you are right. The reason it made sense for promises is because it protects against Zalgo in that consumers of promises produced with the constructor don't have to add both a .catch(e => and } catch(e) handler and deal with timing issues - this is less of a problem (but still is) with async/await.

I also don't think there is a separate issue here, if we're doing C# like tokens might as well do it like C#:

var cts = new CancellationTokenSource();
var token = cts.token; // the cancellation token we can pass around
cts.cancel(); // cancel, idempotent
token.cancelled.then(x => {
   // runs after cancel is called, note that in C# there is an overload of 
   // cancel that instead cancels the cancelled task itself (it's actually a callback in C#).
});
token.isCancellationRequested // true if cancellation was requested

There is also a:

CancellationToken.None

That allows passing empty cancellation tokens.

This API is ergonomic but argument pollution is very annoying and in addition C# does cancellation based on abortive state (an exception) which is awful since I have a ton of places where I explicitly filter out TaskCancelledException and I still get it showing up on my NewRelic logs.

It even woke me up in the middle of the night once :ghost::ghost::ghost: for a false alarm when Azure changed servers and it propagated cancellation up as an exception.

zenparsing commented 8 years ago

It even woke me up in the middle of the night once 👻👻👻 for a false alarm when Azure changed servers and it propagated cancellation up as an exception.

"Should not wake someone up in the middle of the night." Now there's a design constraint I can get behind! : )

domenic commented 8 years ago

I'm fairly against any API that introduces multiple types. As such I'm against a new CancelTokenSource type, but would be fine with an analogous CancelToken.source() that returns a plain-old-object with two properties, token and cancel.

An important difference, in addition to the conceptual load, is that

const { token, cancel } = new CancelTokenSource();

is broken code, since it "un-binds" cancel, and so cancel us usable. Whereas

const { token, cancel } = CancelToken.source();

works fine, since it's a plain object with an already-bound function.

I think if there's a desire to keep the objects separate, the revealing constructor pattern should be the base, with a .source() method as convenience sugar. Whereas if there's no desire to keep them separate, then we should just put cancel on the token itself. @dtribble said

Unlike promise/resolvers, there is little reason to prevent getting the token from the source.

but I guess he did not say

Unlike promise/resolvers, there is little reason to prevent getting the cancel function from the source.

so if we assume the latter is not OK then I think we should stick with revealing constructor pattern + potential .source() sugar.

dtribble commented 8 years ago

I'm not particularly attached to historical terminology but I'll clarify in terms of it to avoid confusion: the "source" is the authority to cancel, and so corresponds to the cancel function. The "token" is the ability to detect or react to cancellation. The reasonable asymmetry is that you can get the token from the source. It would reduce the value of the library a lot if you could get the source from the token because any client who could react to cancel would be able to cancel all other clients.

Stylistically, I prefer to introduce types when there is distinct structure required, but I'm happy to have fewer types. I generally prefer to figure out the object and authority distinctions, and then try to fold them into few types. So if I introduce a type in the design process, consider it "intermediate term expansion until the rest of the design settles".

So "source" clearly is primarily the cancel operation, and so "just a function" makes a lot of sense. There are however a few others things that might make sense as operations or properties on a source. Are those just properties on the source function or does that make it a new "type"? Some examples are:

Some of these could be optional or implementation-specific (e.g., token.alsoCancel(source) could have a generic implementation if source is just a function, but have an optimized implementation if it's a source in the same optimized cancellation library). I can see how to implement while still keeping the default source appearing as just a function, so considering which ones to support doesn't necessary impact the simple core of the library (e.g., "declare never cancelled" is a useful diagnostic, but given it's secondary nature, could be CancelToken.willNotCancel(source)).

benjamingr commented 8 years ago

@domenic a cancellation token/ cancellation token source pair is like a promise/deferred pair in this regard.

In early writings (like some of @eright's) I recall using something a little similar to:

const {promise, resolve, reject} = Promise(); // get direct references

so if we assume the latter is not OK then I think we should stick with revealing constructor pattern + potential .source() sugar.

Innocent question - why is that not OK?


Let's also not forget that if we arrive at a convention where the token is the last parameter we can automate "cancellabelization":

function cancellationScope() { 
   const tokenSource = new TokenSource(); // bikeshed later
    function cancellable(fn) {
       return fn.apply(this, [...arguments, tokenSource)
    }
    cancellable.cancel = () => tokenSource.cancel();
    return cancellable;
}

Which turns the initial example to:

var scope = cancellationScope(); // do this once on top
scope(fetch)("some-data.json").then(response => {
    // ...
});
scope.cancel(); // when we want to attempt cancellation of the whole thing
wycats commented 8 years ago

To me, this issue is a variant of the concern in #2. Cancellation tokens are a nice primitive to express the underlying machinery, but as the front-line API for invoking an async function, it leaves a lot to be desired.