zenparsing / es-cancel-token

Cancel Tokens for ECMAScript
44 stars 1 forks source link

Cancel callback registration API #1

Open zenparsing opened 8 years ago

zenparsing commented 8 years ago

Currently, I have cancel.promise exposing a promise for when cancellation is requested. There are some issues with this:

C# and designs inspired by it create an ad-hoc registration/deregistration API. When cancellation is requested, the cancellation callbacks fire synchronously. In the case of C#, an exception is propagated back to the code that requested cancellation, which contains a list of exceptions that occurred inside of the callbacks.

Do we need to worry about plan-interference for such a design? It's possible that a function which registers a callback also does something to trigger cancellation itself, which would cause the cancellation callback to execute in a stack frame on top of the function which wants to respond to cancellation.

zenparsing commented 8 years ago

@stefanpenner @domenic What do you think about the last paragraph above?

domenic commented 8 years ago

I am not sure I have concrete thoughts on the last paragraph. It does seem like any system where you could have potentially multiple reactions to something (e.g. cancelation) should use microtask timing to avoid this problem.

In general I am leery of inventing a new callback registration/deregistration system. (Worth mentioning: in my "third state" view, your second bullet point is taken care of by the [[CancelReactions]] being cleared upon fulfill/reject/cancel, just like [[FulfillReactions]] and [[RejectReactions]].)

stefanpenner commented 8 years ago

Cancellation being sync may have issues, but I don't have a good picture of what those problems might be (but I can imagine that some exists).

My feeling is that promises are the ones who are to combat plan-interference not the tokens.

I suspect the following outcomes would be the least surprising (but would love feedback).

let token = new Token(cancel => cancel());
new Promise(resolve => resolve(), token); // will settled to cancelled (which may be a rejection, or a new completion record type of cancelled)
let cancel;
let token = new Token(_cancel => cancel = _cancel);
new Promise(resolve => resolve(), token); // will settled to resolve
cancel();
function doWork(token) {
  if (token.isCancelled) { return; }
  // mine for bitcoin

  nextTick(() => doWork(token));
}

let token = new Token(cancel => cancel());

doWork(token); // no work is done.

let cancel;
let otherToken = new Token(_cancel =>  cancel = _cancel)

doWork(token); // does one iteration of work
token.cancel();
zenparsing commented 8 years ago

@stefanpenner I agree with all of those. As long as cancel tokens can be synchronously queried for state, I think we're good.

stefanpenner commented 8 years ago

Yes.

My spike has this sort of adhoc register / deregulated stuff, largely feedback has been: I choose bad names that confuse people(follow, unfollow), dereg should likely be via cookie (like setTimeout/ clearTimeout)

zenparsing commented 8 years ago

dereg should likely be via cookie (like setTimeout/ clearTimeout)

The C# way is to return what amounts to a subscription object from the registration function. Is that what you mean?

stefanpenner commented 8 years ago

@zenparsing well I suppose (at-least) two approaches are possible

  1. unsubscribe(subscribe(cb, )) – like setTimeout clearTimeout
  2. subscribe(cb).unsubscribe()
zenparsing commented 8 years ago

If it turns out that a promise is not sufficient for registration/unregistration, then we might consider the observable interface here:

let subscription = cancelToken.subscribe(() => {
  // ...
});

subscription.unsubscribe();
bergus commented 8 years ago

After we finish an async operation, we typically don't want the cancellation callback to fire, but with promises we can't "deregister" a callback.

This is a big issue I see. One approach would of course be to cancel the token.promise, but that leads down a rabbit hole of cancelling cancellable onCancel promises that again would need a… forget it. Another issue I have with the .promise is that it never fails. That's not necessarily bad, but hints at an asymmetry here. Maybe the cancellation promise should reject once the action promise settles? But probably not.

But actually we must not change the state of the token and its promise when the action settles, because a single cancel token can be used for a multitude of actions. If we implement this with promises, we have a garbage collection problem with the subscribed handlers:

(async function(token) {
    await fetch(token);
    await sleep(60000, token);
    console.log(token);
}(new CancelToken));

Assuming the standard polyfill for fetch that subscribes token.promise.then(() => xhr.abort()), it would mean that a reference to xhr is held for a minute after the request finished. (Yes, this can be solved by setting xhr = null as soon as it is no longer needed, but this is not the kind of code we want to require our users to write). The canceltoken API should by design prevent accidental memory leaks.

I propose as a solution that the subscription mechanism on tokens should allow to pass in a promise next to the callback, and when that promise settles then the callback is automatically unsubscribed. An example (simplifed) fetch polyfill:

function fetch({url, data, token}) {
    var xhr = new XMLHttpRequest();
    var p = new Promise((resolve, reject) => {
        xhr.onload = resolve;
        xhr.onerror = reject;
        xhr.open(url);
        xhr.send(data);
    });
    token.onCancelUntil(p, () => xhr.abort());
//       ^^^^^^^^^^^^^^^
    return p;
}

The callback that closes over xhr will be automatically disposed when p settles.