Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

Avoiding the third state #14

Closed
domenic opened this issue Jun 3, 2016 · 15 comments
Closed

Avoiding the third state #14

domenic opened this issue Jun 3, 2016 · 15 comments

Comments

@domenic
Copy link
Member

domenic commented Jun 3, 2016

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:

  • catch (e) { ... } does not catch cancelations
  • cancelations never reach the host environment's error handling mechanisms (HostReportErrors in the spec)
  • There are symmetrical constructs for catching/throwing cancelations as there are for throw completions (catch/throw)

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

  • Introduce a special type of object, a "cancelation", which is branded in some way.
  • Cancelations are not caught by catch (e) { ... }
  • If a cancelation reaches the top level it is not sent to HostReportErrors.
  • We somehow introduce new guard syntax which can catch cancelations. General catch-clause pattern matching? catch cancel (e) { ... }?
  • To throw a cancelation you just construct it and throw it like any other exception, e.g. throw new Cancelation("..."). I'd even be in favor of introducing a global function named cancel so you can do throw cancel("..."), but people might object to introducing a new global with such a generic name. (We got away with it for fetch and close and open and others, though.)

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

  • I think it is trying to impose a false parsimony of concepts. Cancelations and exceptions are actually very different things, and saying that cancelations are just a type of exceptions with several special rules surrounding them is conceptually messy and wrong, in my opinion.
  • The details of the catch guard syntax is very unclear. Catch guards could be their own very large proposal in their own right, and introducing such a dependency is not a good way to get cancelation working.

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
Copy link
Member Author

domenic commented Jun 4, 2016

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
Copy link

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
Copy link

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
Copy link
Member Author

domenic commented Jun 22, 2016

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
Copy link

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
Copy link

@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
Copy link

jan-ivar commented Jun 22, 2016

@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
Copy link

jan-ivar commented Jun 22, 2016

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
Copy link

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

@benjamingr
Copy link

@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
Copy link

@benjamingr My point in #14 (comment) 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
Copy link
Member

ljharb commented Jul 11, 2016

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
Copy link

shicks commented Jul 23, 2016

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
Copy link

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
Copy link
Member Author

domenic commented Jul 26, 2016

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants