zenparsing / es-cancel-token

Cancel Tokens for ECMAScript
44 stars 1 forks source link

Pros/Cons of races between cancel() and token.promise.then() #7

Open stefanpenner opened 8 years ago

stefanpenner commented 8 years ago

Has this discussion happened already? If so can someone direct me to it, otherwise I can gladly flesh out the thoughts and we can discuss here.

bergus commented 8 years ago

@zenparsing mentioned this in the OP of #1 (but no response there)

[An issue with promises is] Race conditions caused by the time lag between cancellation being requested and the callback getting executed. How big of an issue is this?

[In C#] When cancellation is requested, the cancellation callbacks fire synchronously. […] an exception is propagated back to the code that requested cancellation, which contains a list of exceptions that occurred inside of the callbacks.

I believe the asynchrony of a promise for publishing cancellation is a large issue. It's OK for cleanup actions and similar things that are fire-and-forget, but I believe those will be subscribed differently anyway (promise.finally, promise.onCancel(?), catch.cancel blocks etc). The main use case I see for cancellation tokens is in the promisification of asynchronous APIs, and here the semantics matter a lot. A simple example of a race condition could be

return new Promise((resolve, reject, cancel) => {
    var sub = XYZ_api.subscribe(resolve); // wrong! see below
    token.promise.then(reason => {
        sub.undo();
        reject(reason); // or cancel() if we get a third state, or whatever
    });
})

Now it could happen that 1) someone cancels the token 2) subscribe fires and resolve is called 3) the token promise callback fires and reject is called (but not effective). This is very much a bug - if we cancel the token, we expect that the promise will in no case be resolved or settled. Yes, promises using microticks does make the above unlikely as subscribe typically would use macroticks, but that doesn't need to be the case. The solution could be to use

    var sub = XYZ_api.subscribe(x => { if (!token.requested) resolve(x); });

but I don't want to put the burden of doing this correctly on our users.

Whatever we are going to implement for cancellation, I believe it is very important that the default way to subscribe to cancellations does have a guarantee similar to the one from Promises/A+ - that onFullfilled and onRejected must in no way be called both - and extended to cancellation:

Given a CancelToken token, a Promise p = makePromise(token), an onFulfilled callback that is passed as the first argument to a .then() invocation on p, and an onCancel callback that is (in some way) subscribed so that it fires on cancellation, we guarantee that onFulfilled and onCancel are never ever called both.

Given that token does not know about p, I can imagine only few sensible ways to create an API for that:


Regarding the second paragraph of the quote, the C# approach is definitely interesting. I've not found any discussion about the return value of cancel() yet and what should happen to exceptions thrown in cancellation handlers. Similarly we don't know what should happen to return values from inside finally clauses. And those could even come in asynchronous, via finally { await ???; } as a rejection or fulfillment. By executing cancellation handlers synchronously, this would allow us to return an array of promises for the completion values of all callbacks. The caller of cancel then would be able to handle errors that arose from the cancellation attempt, instead of letting the go as unhandled rejections.

bergus commented 8 years ago

@erights I'd like to hear your thoughts on synchronous cancellation handler execution. Do you see any problems similar to the ones that calling then callbacks from within the resolve function has?

jovdb commented 5 years ago

FYI: I have experimented with this proposed interface on https://github.com/jovdb/fetch-hof.

I could get my stuff to work, but I would find it easier to have:

Example: In this piece of code I find it much easier to synchronous cancel/error the Reader than to reason about a working Promise.race construction with cancelToken.promise and the StreamReader.read.