tc39 / proposal-cancelable-promises

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

Avoiding the third state #14

Closed domenic closed 8 years ago

domenic commented 8 years ago

Some people have brought up that introducing a third main type of completion is a big deal and that maybe it should be avoided.

I think we need to preserve the following requirements no matter the shape of the solution:

One proposal that still meets all these requirements is as follows:

I think this alternate proposal is not as good as the third state proposal for a couple weak reasons:

As such I plan to continue pursuing the third state approach. But I wanted to leave this as a record of my thoughts on the matter. If someone wants to take ownership of an alternate proposal I'd be willing to work with them.

domenic commented 8 years ago

In the course of working on #12, I've hit upon a variant on this proposal that I think is observably equivalent, but conceptually very different. If we also nail down the guard syntax to just a special-case catch cancel (e) { ... }, it defangs both of my objections. Here it is:

Create a global function named cancel that takes a string and returns an object. This object has a magical brand. When you throw an instance of this object type, it creates a throw cancel completion. throw cancel completions are not caught by catch. They are caught by catch cancel (e) { ... }.

One big difference between this magical-object proposal and the original proposal is that given

try {
  f();
} catch cancel (e) {
  throw e;
}

in the original proposal the rethrow will create a throw completion, whereas in this magical-object proposal, it will create a throw cancel completion.

As I said, this is conceptually very different; the OP of this issue says that it's still a throw completion and then adds a bunch of checks wherever you process throw completions to avoid processing specially-branded objects in a certain way. Whereas this comment uses a new completion type. However, I think they are observationally equivalent.

martinthomson commented 8 years ago

If you require that cancellations never hit catch, then you have a third state. Sounds like you are just looking for a different/better API shape.

jan-ivar commented 8 years ago

I think we need to preserve the following requirements no matter the shape of the solution:

  • catch (e) { ... } does not catch cancelations

I question this premise. Why should catch not catch cancellations? Consider:

try {
  let flower = await fetch('flower.png', {}, cancelToken);
  draw(flower);
} catch(e) {}

try {
  let chair = await fetch('chair.png');
  draw(chair);
} catch(e) {}

You're saying cancelToken.cancel() during fetching of flower.png should cancel chair as well? Why?

Semantically, catches are move-on-points in code, where code defies failure. I.e. "We don't care about no flower, we're drawing a chair anyway." is the semantic meaning of the code above.

Having cancellation defy this, seems confusing. What's its scope then? And what if cancelToken.cancel() instead happened during fetching of chair.png? Would it still cancel chair?

domenic commented 8 years ago

The premise is explained in detail in https://github.com/domenic/cancelable-promise/blob/master/Third%20State.md and is core to this proposal. The answers to the rest of your questions can be found from the spec. Note that cancelToken does not have a cancel method, so it is not clear you have read the spec or its supporting documents.

jan-ivar commented 8 years ago

I read three times. Liberty taken:

function myCancelToken() {
  let defer, token = new cancelToken(cancel => defer = cancel);
  token.cancel = defer;
  return token;
}

I think your premise is flawed, sorry. Had I wanted chair to be cancelled I could have passed cancelToken to it as well.

My understanding is that people want to cancel _a_ fetch, not all of them. Semantically, what follows a catch does not rely on completion of what preceded it.

I see this: "Once an operation is canceled, any fulfillment or rejection reactions to it are no longer valid."

But not the converse: "Once an operation is fulfilled or rejected, any cancellation reactions to it are no longer valid."

So I wonder if cancellation happened during fetching of chair.png would it still cancel chair?

benjamingr commented 8 years ago

@jan-ivar catch catching cancellations has been a major pain point in C# - your worker goes down because a cancellation token told it to and all your logging and metric infrastructure starts going wild at night.

You end up having to do catch(Exception e) when (e is not TaskCancelledException) all over the place and be ready to wake up in the middle of the night for a false downtime alarm when you get an SMS with 1000 server errors which are not really errors - Azure just updates the OS of the workers one by one and each triggered 10 cancellations.

I'm definitely against any proposal that does this. On the other hand - I'm pretty fine with just running finally blocks and checking token.requested in those.

jan-ivar commented 8 years ago

@benjamingr fetch should not be an API for taking down a worker. There's a basic scoping flaw here.

To me, cancellation at an individual method API level, is very different than cancelling something with its own event loop.

jan-ivar commented 8 years ago

I think we're mixing things. Cancellation of something with its own event loop is different, you're cancelling another thread basically. That thread is often doing multiple independent jobs, each of which is fully caught and logged. I agree that in order to tear something like that down, you need a cancellation path that bypasses local catch handlers, to avoid spew. But that's a higher-order cancel in my book.

If we want that, we should consider an API like:

    cancellableChain(token)
    .then(() => foo())
    .then(() => fetch('a').catch(e => {})
    .then(() => fetch('c').catch(e => {});

not overload the first cancellable operation, whatever it is, as that's weird and confusing:

    foo()
    .then(() => fetch('a', {}, token).catch(e => {}) // note: token cancels whole chain!
    .then(() => fetch('c').catch(e => {});

In contrast, cancelling an individual fetch (which is what people are asking for), to me means cancelling _one_ job, not all jobs, and is something done inside the worker, as perhaps a way to react to being told to tear down, and I'd expect such cancellations to be caught within catches, not escape out of them.

jan-ivar commented 8 years ago

...because such cancellations are in the domain of the worker, if you will, not outside of it.

benjamingr commented 8 years ago

@jan-ivar it's not, the problem is stuff surrounding it might treat exceptions as well... exceptions. Cancellation as exceptions failed because of exactly that - the code "recovering" catches the exceptions and thinks it's a recovery - you would have to special-case every single exception handling or reporting mechanism to test against that.

try {
   await fetch(url, token);
} catch (e) {
  // retry
}

Has to become:

try {
   await fetch(url, token);
} catch (e) {
  // retry
}
jan-ivar commented 8 years ago

@benjamingr My point in https://github.com/domenic/cancelable-promise/issues/14#issuecomment-227723195 is that there's typically no "stuff surrounding it" in the simple case of cancelling an asynchronous fetch or a single promise chain, code usually written by one author. This "outside party wants to cancel" higher-order thing is a different beast IMHO than what people are asking for, which I think is better covered by a simple pattern that can be adopted to not just stop waiting for things, but actively rescind activity.

ljharb commented 8 years ago

It does seem there is often conflation between cancellation as "registering disinterest, which may lead towards unnecessary work no longer being performed" and cancellation as "a guarantee that any ongoing or future task will not proceed, even if other things are interested in the result". In the general case, it might be helpful to clarify which is being discussed in each comment.

shicks commented 8 years ago

Would something similar (or maybe opposite) to Java's interrupted bit help in this situation? I agree with @jan-ivar that it seems very dangerous to treat cancellations as very different from normal exceptions, but maybe a happy medium would be that if you catch a Cancellation but don't explicitly "un-cancel" it, then any future async operation will immediately re-throw the cancellation (it would probably need to be reset once the entire stack has unrolled).

A disadvantage here is that it's clear in Java when this is necessary, since InterruptedException is a checked exception, so it must be declared and explicitly caught. But I'm very wary of the situation where code that expected it would catch everything is now no longer catching everything.

benjamingr commented 8 years ago

Cancellations are not normal exceptions, they are not exceptional cases, they are different.

If I cancel an HTTP request because a user typed another letter in a typeahead there is nothing exceptional around that and modeling that through exceptions is strange. In fact in most cases I use cancellation in nothing exceptional has happened.

InterruptedException in Java and thread interruption, at least from what I remember is considered dangerous and InterruptedException is both tricky and risky for having exceptional semantics - it doesn't have these semantics conceptually.

I'm fine with any solution where catch clauses don't run on cancellation - Domenic's modeling of that through third state is just a more consistent formalism - in Bluebird for example finally blocks are run but nothing else.

domenic commented 8 years ago

More or less implemented the OP's approach now; closing. Unfortunately catch runs for cancelations, but we can use ESLint rules to disallow try ... catch in favor of try ... else in new code.