tc39 / proposal-cancelable-promises

Former home of the now-withdrawn cancelable promises proposal for JavaScript
Other
376 stars 29 forks source link

Make throwIfRequested() implicit in await.cancelToken assignment #55

Closed bergus closed 7 years ago

bergus commented 8 years ago

I had previously suggested in #41 to

make await.cancelToken throw the Cancel immediately when assigning an already .requested token

Should we? Should we not? I think it's useful because it avoids the boilerplate needed to account for this edge case, and await.cancelToken is all about avoid boilerplate while maintaining reasonable semantics.

domenic commented 8 years ago

What makes me cautious about doing this is that it makes await.cancelToken = less like an assignment that impacts the behavior of await, and more like a pragma which has its own individual behavior. It's not unprecedented---setters throw sometimes---but it's a bit tricky.

It would help to have a few compelling examples of functions that would benefit from this. Otherwise, if this is an edge case that comes up rarely, I am inclined to keep the semantics as they are, as I think they are less surprising.

bergus commented 8 years ago

less like an assignment and more like a pragma which has its own individual behavior.

Yeah, that's why I had suggested to make it more like a keyword/unary operator than an assignment.

a few compelling examples of functions that would benefit from this

It basically comes up whenever you chain a cancellable async function to an uncancellable function:

async function a(x) {
   …
   return …
}
async function b(y, token) {
    await.cancelToken = token;
    // or:
    await.cancelOn token;
    …
    return …
}

const token = …;
a(x).then(y => b(y, token)).…

If the token is cancelled while a is still running, we want to ensure that at least b doesn't start any work.

dead-claudia commented 7 years ago

Doesn't return already cause a cancel rejection in this case if the cancel has already been requested?

The suggestion might make sense, though, in the event the function schedules I/O in the first tick.

async function foo(cancelToken) {
    await.cancelToken = cancelToken
    // Not having to do a `cancelToken.throwIfRequested()` here
    const data = await fetch("http://example.com/some/resource.json", {cancelToken})
    // ...
}
itaysabato commented 7 years ago

Hi there,

Perhaps I'm little bit out of my league here, but, the way I would ideally want any/most cancelable async functions to behave is equivalent to (according to the current semantics as I understand them) :

async function foo(arg1, arg2, ..., cancelToken) {
    await.cancelToken = cancelToken;
    // Before doing anything:
    cancelToken.throwIfRequested();

    //...sync stuff...

    // ONLY after each await (on top of the implicit race induced by setting await.cancelToken):
    //cancelToken.throwIfRequested();
    await somethingAsync1(cancelToken);
    cancelToken.throwIfRequested();

    // ...more sync stuff...

    // Once again validate after the await:
    //cancelToken.throwIfRequested();
    await somethingAsync2(cancelToken);
    cancelToken.throwIfRequested();

    // ...perhaps even more sync stuff...
}

As far as I understand I have to do all of this manually in order to avoid executing any code after cancellation.

If I am not mistaken in my understanding of the behavior or common needs (and please enlighten me if I am), it would be most awesome for me (and I believe for most people) if the language supported something like a casync keyword that can be used instead async and would make the following code equivalent to the above:

casync function foo(arg1, arg2, ..., cancelToken) {
    //...sync stuff...

    await somethingAsync1(cancelToken);

    // ...more sync stuff...

    await somethingAsync2(cancelToken);

    // ...perhaps even more sync stuff...
}

Perhaps this is not the right thread for this kind of suggestion (if there is one) but it seems to me the motives for opening this issue were similar to mine.

Additionally, if there is a better way to achieve this behavior with the current semantics please let me know.

Thank you for your patience :)

UPDATE

I realize now that, of course, there is no need for an additional check after the synchronous blocks, i.e. before every await. Everything else still seems necessary though.

bergus commented 7 years ago

@itaysabato

Before AND after each await (on top of the implicit race induced by setting await.cancelToken):

cancelToken.throwIfRequested();
await somethingAsync1(cancelToken);
cancelToken.throwIfRequested();

No, this is what the implicit race already does. Notice that await is a keyword expression, we cannot do anything in the "line before" the statement that involves the await keyword. So if at all you'd get something like

const p = somethingAsync1(cancelToken);
cancelToken.throwIfRequested();
await p;
cancelToken.throwIfRequested();

and this is what we're already doing. If the token is already cancelled, the race won't even start and the exception is thrown immediately. And after the await has returned a result, you can assert that the token is not yet cancelled, so it would never throw in that position - the await itself would've thrown during the race.

To get a cancelToken.throwIfRequested(); before you invoked somethingAsync1(cancelToken);, that should in general be unnecessary. During the //...sync stuff... preceding the call, you know that the token isn't cancelled because it was not at the beginning and you're not doing anything that does cancel it (right? If not, then you might want that check explicitly).

Before doing anything:

await.cancelToken = cancelToken;
cancelToken.throwIfRequested();

Yes, that's exactly what we are discussing here. The assignment should throw if the cancelToken is already cancelled.

itaysabato commented 7 years ago

@bergus thank you for the detailed explanation :) As you can see in my edit I realized eventually that the checks before each await are, in fact, redundant.

There is just one thing, if you may, that I would like to be sure about: is it guaranteed by the underlying engine that once the race concludes with the promise being resolved, rather than cancelled, no other code/callback is executed prior to the continuation of the function? E.g. is the following scenario impossible?

  1. The called async function is completed.
  2. It is determined by the "awaiter" that the calling async function should continue with the resolved value.
  3. Another piece of code somewhere executes and cancels the token.
  4. The calling async function continues with the resolved value, regardless of the cancelation.

If it is impossible, that's great. In that case, I would be interested to know from what definition this guarantee stems from. In theory at least there is no guarantee that another consumer of the same promise won't be invoked before the await consumer.

Thanks again!

bergus commented 7 years ago

@itaysabato Oh, good call. I'm not sure any more. Please open a separate issue about that, I'll try to come up with an illustrating example and will check the spec draft.

itaysabato commented 7 years ago

@bergus OK I will do that shortly.

Just one last suggestion in regards to the actual issue at hand: what about something like the following?

await with cancelToken;

With this syntax the intention seems clearer - that henceforth the current function's execution is bound to the state of the given cancel token. Alternatively, you could use curly braces to define the scope of the binding to the token, but the extra nesting would be annoying - which leads me back to trying to extend async but with little more verbosity, e.g.:

async with cancelToken function foo(args..., cancelToken /* or whatever name you choose as long as its compatible with the async with statement */ ) {
    //...
}
dead-claudia commented 7 years ago

I kind of like that idea, but I'm not sold on the keyword choice.

On Thu, Dec 8, 2016, 12:48 itaysabato notifications@github.com wrote:

@bergus https://github.com/bergus OK I will do that shortly.

Just one last suggestion in regards to the actual issue at hand: what about something like the following?

await with cancelToken;

With this syntax the intention seems clearer - that henceforth the current function's execution is bound to the state of the given cancel token.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/tc39/proposal-cancelable-promises/issues/55#issuecomment-265805817, or mute the thread https://github.com/notifications/unsubscribe-auth/AERrBFXyfxTHsfpQulFJ697hg3aKrEvOks5rGELTgaJpZM4JzN-r .