Observables should not catch exceptions

[rpominov]

As author of Observable and Task implementations I have been thinking about this subject quite a lot. And recently summarized all my thoughts in a short article Exceptions catching in async abstractions like Promise or Task.

The conclusions I made is that we don't need automatic catching in browser at all but we might need it in Node although there other options. Ideally catching should be optional, but if it's impossible to make it optional it's better to not catch at all. At very least automatically catched exceptions should go into a separate callback.

I would love to hear everybody thoughts on the subject. With Promises the ship has already sailed, but maybe we could make it right with Observables at least!

[ljharb]

I think any inconsistencies with Promises, especially with catching errors, is going to be a really hard sell, and a really huge cognitive cost.

[rpominov]

Fair point, but the spec already introduces some inconsistencies. For example laziness and cancellation. It was important to add these features so it was worth to introduce the inconsistencies. I think not catching exceptions is as important.

I would really appreciate if everybody would read the article I mentioned in the opening comment. I did the most of the reasoning there and didn't copied it to the comment. But I'll try to repeat some thought from the article here.

After a bug, best thing we can do is to crash the program, so it a) wouldn't make more damage, like corrupting database b) would be easier to debug. Worst thing we can do is to execute some code as an response to a bug because this only moves program to a more complex inconsistent state which makes it harder to understand the bug and can corrupt database in even more complicated way.

When async abstractions catches all errors, it automatically catches bugs as well. Then it executes some code in response to a bug, and forces programmer to write even more code that will be executed in response to a bug.

Let's suppose if statement would do something similar to what Promises and Observables (currently) do. It would work something like this:

if (expression()) {
  // the `true` case goes here
} else {
  // the `false` case goes here
  // but also any error thrown from expression() goes here
}

It introduces more code paths, so it's harder to reason about code. Also there is no sane code that we could write that handles the bug. People would introduce their own non standard alternatives to if like if(expr, trueCb, falseCb), or would write more code to workaround inconvenient behavior, like this:

let thrown
function safeExpression() {
  try {
    return expression()
  } catch(e) {
    thrown = e
    return false
  }
}

if (safeExpression()) {
  // the `true` case goes here
} else {
  if (!thrown) {
    // the `false` case goes here
  }
}

if (thrown) {
  throw thrown
}

This is just silly. Yet this is basically how Promises work. And I did both with Promises: wrote/used alternatives, and wrote more code to workaround. This is a big paint point for me with Promises.

---

Edited: grammar.

[rpominov]

@RReverser made me realize on twitter that there are two views on the semantics of failure case in Promise and Observable:

1. Failure case is designed exclusively for uncaught exceptions / bugs. All expected results including expected failures should go into success case.
2. Success case for success results, failure case for expected failures and (inconveniently, as a design mistake) for uncaught exceptions.

If everybody agree that the first one is the right semantics, then there is no problem actually. Except this makes Promises and Observables less useful abstractions. Basically we can't use it for patterns described in Railway oriented programming.

[benlesh]

Any errors thrown in the subscriber function passed to the Observable will go down the error path. That's sort of edge-casey though.

Otherwise, observables and promises differ in that promises use then for side-effects, mapping, flat mapping, error handling, etc. Observable subscribe is only used for side-effects. Therefore there's no reason to catch in there other than to call teardown synchronously before throwing the error synchronously.

I think this issue might be a misunderstanding

[rpominov]

Ah yeah, I forgot that currently spec doesn't even describes map, flatMap etc. I was talking about exception thrown from functions passed to map etc. https://github.com/zenparsing/zen-observable catches those exceptions. This is my concern. I think this is wrong if we imply #2 semantics from #104 (comment)

[jordalgo]

Interesting topic. I think I like option 2 but agree that we shouldn't do any error catching in an Observable or Subscription. Calling observer.error is very explicit (as @rpominov said, it's an expected error) and shouldn't be treated as an Exception. My opinion right now (and it could change) is that error passing is similar to filling an Either Monad with a Left. This allows for a subscription to stay open if an error is passed from the top -- not handling it would be fine and not cause a throw (up to the users of the type to handle).

If we opt for option 1, then even if you pass expected errors down the success path if you have anything likemap or filter (yes, I know they aren't yet in this spec) that expect a certain data type, you're going to get a thrown error and the subscription will self cancel anyway. That doesn't seem useful at all for Observable composition.

[rpominov]

JFYI. I've rewritten the article I was mentioning before. It should describe my position on the issue more clearly now.

[jordalgo]

Want to bump this thread as I think it's important to get more information about why folks want to preserve error catching inside Observables.

At the moment it seems like the biggest argument for it is because Promises do it and that if Observables are inconsistent with this style then this spec is, potentially, at more risk of not being approved (please correct me if I'm wrong), which to me seems like a very weak argument. I'd almost rather Observables not be part of the language so that they too don't promote this automatic mixing of asynchronicity and error handling.

I think it would be great if people who are for error catching inside Observables would chime in and offer an opposing view to @rpominov . I myself am really craving a staunch supporter of automatic error catching in both Observables and Promises that could contribute some valuable insight because i'm just not seeing it.

[jordalgo]

Ok, not to keep spamming people but I wrote up a short explanation as to why @rpominov and I (and probably many others) want to keep error handling out of Observables:

TL;DR: error catching in observables removes the usefulness of the error path and hurts composition.

The inherent problem with observables catching errors internally and sending them down the error path is that a consumer/subscriber isn't able to distinguish between an unexpected error and an expected one.

For example, you have some async computation, a repeating AJAX request, which at some point may get a response that is not a 200 (an expected error). You don't necessarily want to kill the observable in this case (calling error or complete) but rather notify the consumer that you did not receive a successful response; bypassing composing functions that manipulate success data. This is possible if the error path is not also responsible for catching unexpected errors because a contract forms between the observable and the consumer that the error path will produce an error-like value of type X (the same implicit contract used by next) which can be used to show an error message to a user, trigger some other request, etc... Additionally, if Observables stipulate that only values that are instanceof Error should be traveling down the the error path you lose the ability to both send a non-string error value to consumers and keep an Observable open/active in the case of an expected error. If there is no stipulation or guideline, you end up with some very shaky logic being applied to all error consumers e.g.

observable.subscribe({
  error: err => {
    if (err instanceof Error) {
      // re-throw or handle err
   } else {
      showPlaceholderView(err); // still probably have to check what err's type is
   }
});

The other issue with auto-catching thrown errors is that most of the time, you want these errors to be noisy and program crashing, this is not intuitive if you have a function that is responsible for consuming all the errors in an Observable chain. You could/should probably then re-throw all these errors unless you know you're doing something risky like parsing JSON (which should have it's own try/catch anyway) but even if you re-throw the errors, this error is no longer being thrown from the place it ACTUALLY broke, which makes debugging that much more difficult.

It might seem scary to remove error catching from Observables because a long observable chain might be more prone to breakage with so much potential composition but the wonderful thing about functional chains like this is their purity and predictability. If we want to make Observables less intimidating, we could always add some degree of type checking in methods like map, filter, etc..., which isn't that expensive performance-wise if done properly.

The last thing I'll say is that Observables are innocent until proven guilty. There should be well defined reasons why adding opinionated error handling in Observables is neccesary and why Observables, by default, cause a lot of pain in handling and debugging unexpected errors.

[zenparsing]

I'm not sure I understand the arguments raised in this thread.

In this proposal (which doesn't have map or filter), errors that occur as a result of calls to next are only caught so that the cleanup function can be run. The error is then re-thrown. If you look at the implementation, you'll see that the only place where observer.error is called is when the subscriber function has thrown an error. And in that case, if the observer provided to subscribe doesn't have an error handler, the error will be re-thrown.

Can you explain your concerns concretely in terms of this proposal, rather than in general terms?

[rpominov]

I wasn't careful when I've opened this issue, and forgot that spec doesn't contain operators yet. Sorry about that. I understand if this is too early to discuss this, but maybe we could start the discussion earlier.

But catching in subscribe has the same issues basically. If we want to use Observables as if they implement built-in Either type random exceptions must be never put into error callback.

Sorry to keep referring to the article I wrote, but I really don't know how to explain my concerns completely in some short elaborated way. It's kinda a paradigm shift so it takes a short article to explain. I'll highlight some examples with current spec though:

Number 1

In Flow or TypeScript we won't be able to specify type of errors facebook/flow#1232

Number 2

Say we built a web server:

function handleRequest(req, resp) {

  // obs contains only one event with the data from database
  const obs = getDataFromDb(req);

  obs.subscribe({
    next(data) {
      sendSucceesHeaders(resp)
      // buildBody throws
      sendSuccessBody(buildBody(data), resp)
    },
    error(error) {
      // by the time we're here success headers already send, so we're sending headers twice
      sendErrorHeaders(resp)
      sendErrorBody(buildErrorBody(error), resp)
    }
  })

}

So basically we can have both next and error handlers executed in an observable that supposed to have only one event, can cause subtle bugs.

Number 3

@jordalgo 's example also applies to the case where exception comes from next handler #104 (comment)

[ljharb]

If you're only getting one success or one failure, isn't that modeled already by a Promise? Might not matter for your point, but I'd love to see a use case that is specific to Observables.

[zenparsing]

Number 1

Some aspects of typing JS are a challenge, sure.

Number 2

If the producer only sends one value, then according to this proposal they won't get sent both a next and error.

[rpominov]

Probably the fastest way to understand this issue is:

1. Read the article https://github.com/rpominov/fun-task/blob/master/docs/exceptions.md
2. Read this comment Observables should not catch exceptions #104 (comment)
3. Done. You can ignore everything else written here by me so far.

[rpominov]

Some aspects of typing JS are a challenge, sure.

This is actually just a symptom of a bigger problem: in our error handlers we have to handle expected failures as well as random unexpected errors (bugs), see #104 (comment) and https://github.com/rpominov/fun-task/blob/master/docs/exceptions.md#trycatch

[zenparsing]

I understand that you are convinced that certain error handling aspects of promises (and here in the subscribe function) is a design mistake, but that view is hardly universal. Indeed, "fixing the design errors of Promises" is a non-goal for this proposal, and would surely be met with a high level of resistance.

[jordalgo]

@zenparsing Are the use of promises in Observables part of the spec? If not, the intention here is not to fix the design errors of Promises but rather prevent them in Observables.

I totally agree that the view about the error handling design "mistake" is "hardly universal", which is why we're trying to present our best and clearest arguments for not including this same design in Observables -- though clearly we have more work todo 😝

In this proposal (which doesn't have map or filter), errors that occur as a result of calls to next are only caught so that the cleanup function can be run. The error is then re-thrown.

So this is not sent down the error path? Also, can you give me an example of when an error would get thrown when calling next ? Or do you mean when this value actually hits a subscriber which then throws an error?

[zenparsing]

Here's what next does (easier to let the code explain): https://github.com/tc39/proposal-observable/blob/master/src/Observable.js#L139

[jordalgo]

Ok, so in attempt to be very specific. I'll focus on this bit of code instead of the next call which doesn't attempt to push the error down the error path.

In that block, we call the subscriber (passing in the observer) and if that action throws, then we attempt to throw the error down the error path. Our argument is that doing this is a mistake. You lose the usefulness of exposing observer.error to the subscriber as a path for expected errors. You also then make the assumption that if a subscriber uses that path that they want the stream to close.

[benjamingr]

@jordalgo @rpominov I think you're making a very big claim here saying that all errors communicated through error are expected and all errors communicated through throw are unexpected.

The canonical terms for "expected" and "unexpected" errors are "programmer errors" and "operational errors". Oh so very much has been written about this.

When we debated promise throwing semantics in Node - I think we came to a conclusion that one cannot determine if an error is operational or programmer by the way it is raised since only the consuming code can decide which is which. If a file is not found and an error is raised that can be both and a synchronous TypeError is thrown when invalid JSON is parsed which is hardly a "crash the server" scenario.

For example, binding to a closed UDP socket, listening on an invalid port or parsing invalid JSON are things I'd expect to get as an "expected" error as a consumer and be able to handle in error. On the other hand when I try to read the configuration file to start my app and it doesn't exist - that's probably a programmer error but you might get an error on it.

What's really missing from promise (and async function) error handling is filtering catch with predicates which bluebird does with .error and .catch(PredicateOrType, handler) - but the TC will likely fix this for exceptions overall and has expressed interest in doing so. When we write our observables inside async functions - awaiting the subscription and wrapping that up in a try/catch that now has filtering can solve your issues.

Also - I would appreciate it if you tone down the "promises suck" discussion - it makes it very hard to have conductive discussion. I do encourage you to go through the countless design discussions that went into the error handling design of promises - at promises/a+ and in esdiscuss and meeting notes. I also agree with blesh that promises solve a fundamentally different problem from observables - the two use cases are not the same at all anyway.

[rpominov]

Thank you for your response @benjamingr, you've brought up a lot of interesting information.

I think you're making a very big claim here saying that all errors communicated through error are expected and all errors communicated through throw are unexpected.

This is not exactly what I'm trying to say. I think that some thrown errors are programmer errors. And it would be more useful if all errors in observable error path would be operational. In order to achieve that we should not wrap basically random pieces of code into try..catch and then pass caught errors into observable error path.

When we debated promise throwing semantics in Node - I think we came to a conclusion that one cannot determine if an error is operational or programmer by the way it is raised since only the consuming code can decide which is which.

Good point. Still, if we will be able to introduce errors in observables only explicitly by doing new Observable(o => {o.error(...)}) or obs.flatMap(x => error(...)), we already remove huge amount of programmer errors (typos that raise exception, etc.). Then we could use type system like TypeScript or Flow to deal with remaining errors: basically we will have types like Observable<string, "file_not_found" | "incorrect_json"> or Observable<string, never> (for an observable without errors). At this point if programmer are given a value of type Observable<string, "file_not_found"> they can decide what "file_not_found" means in the particular case: is it a programer error or operational one and handle it accordingly.

Also - I would appreciate it if you tone down the "promises suck" discussion

Very sorry if something I wrote sounded that way. I agree that some of my criticism of promises comes from misunderstanding of actual reasons behind some aspects of promises/a+. And I have huge respect to all people who worked on that spec.

[jordalgo]

Also - I would appreciate it if you tone down the "promises suck" discussion - it makes it very hard to have conductive discussion. I do encourage you to go through the countless design discussions that went into the error handling design of promises - at promises/a+ and in esdiscuss and meeting notes.

My sincere apologies if I said anything insulting or incendiary. I really make no claim to know more then all the folks who spent years talking through and working on Promises and I intend on reading through those discussions.

I also agree with blesh that promises solve a fundamentally different problem from observables - the two use cases are not the same at all anyway.

Sounds good, I'll leave them out of the discussion 👍

[jordalgo]

To continue this discussion, I just wanted to point out that we are really only talking about one part in this spec's current implementation that sends an error down the error path:
https://github.com/tc39/proposal-observable/blob/master/src/Observable.js#L114

I'm unsure of the usefulness of this particular block because if the subscriber is potentially throwing an error every time it's invoked, then the whole observable is fairly useless anyway; seeing as how you just get an error every time you subscribe. Is the idea that a consumer doesn't have to get bit by a thrown error if they can't fix the subscriber code (e.g. it's in an external dependency) ?

Also, the other places that wrap code around try/catch (example) simply attempt to clean up the subscription and then re-throw the error -- this seems inconsistent with the logic above because if the Observable has a reference to the observer, why not send that error down the error path before attempting clean up -- not saying I'm advocating for that, just curious about why the behavior is different.

[jhusain]

This issue is a duplicate of another issue.

#119

The SubscriptionObserver will catch errors. Please see here for the rationale.

#119 (comment)