SubscriptionObserver should never throw, just like Promise

[jhusain]

Observables, like Promises, can be either multicast or unicast. This is an implementation detail, should not be observable. In other words, it should be possible to switch an Observable from unicast to multicast without changing any code in any of the Observers of that Observable.

Given that design constraint it seems necessary that SubscriptionObservers, like Promises, must swallow errors. Allowing errors thrown from Observers to propagate can leak details of whether an Observable is unicast or multicast. To demonstrate this issue, consider the following Subject implementation:

class Subject extends Observable {
  constructor() {
    super(observer => {
      this._observers.add(observer);
    });
    this._observers = new Set();
    return () => this._observers.delete(observer);
  }

  _multicast(msg, value) {
    const observers = Array.from(this._observers);
    for(let observer of observers) {
      observer[msg](value);
    }
    return undefined;
  }

  next(value) {
    return this._multicast("next", value);
  }
  error(error) {
    return this._multicast("error", error);
  }
  complete(value) {
    return this._multicast("complete", value);
  }
}

Note in the code above that if an observer throws from either next, error, or complete, then none of the remaining observers receive their notifications. Of course, we can guard against this by surrounding each notification in a catch block, capturing any error, and rethrowing it later.

  _multicast(msg, value) {
    const observers = Array.from(this._observers);
    let error;
    for(let observer of observers) {
      try {
        observer[msg](value);
      }
      catch(e) {
        error = e;
      }
    }

    if (error) {
      throw error;
    }
    return undefined;
  }

Unforunately this only works if there is only one error. What if multiple observers error? We could aggregate all observer errors into a CompositeError like so:

  _multicast(msg, value) {
    const observers = Array.from(this._observers);
    let errors = [];
    for(let observer of observers) {
      try {
        observer[msg](value);
      }
      catch(e) {
        errors.push(e);
      }
    }

    if (errors.length) {
      throw new CompositeError(errors);
    }
    return undefined;
  }

Unfortunately now we have a new leak in the abstraction: a new error type which is only thrown from multi-cast Observables. This means that code that captures a particular type of error may fail when an Observable is switched from unicast to multicast:

try {
  observable.subscribe({ next(v) { throw TypeError })
} catch(e) {
  // worked before when observable was unicast,
  // but when switched to multicast a CompositeError thrown instead
  if (e instanceof MyCustomError) {
    //...
  }
}

Once again we can see that the interface changes when moving from a uni-cast to multi-cast Observable, and the implementation details leak out.

The only way I can see to cleanly abstract over multi-cast and uni-cast Observables is to never mix push and pull notification. I propose the following simple rule: subscribe never throws. All errors are caught, and if there is no method to receive the push notification that the error occurred, the error is swallowed.

[zenparsing]

Interesting point.

So, more specifically, are you suggesting that before subscribe returns, the SubscriptionObserver will not allow the error to propagate? Or that SubscriptionObserver will never allow errors to propagate?

[zenparsing]

Another option besides swallowing would be to use HostReportErrors to allow the browser to report the exception (just like exceptions are reported for DOM event handlers). And Node would terminate, of course.

This might be more appropriate, since (unlike with promises) a swallowed error would be simply thrown away without any ability to recover it at a later time.

[mpj]

Please, please, find ways to avoid error swallowing. Swallowed errors is an absolutely dreadful experience.

This is my main gripe with many of the current observable implementations out there. If you have a chain of observables that you're debugging and it's not obvious where the error is occuring, it is going to take so much time to debug it that any productivity benefit you got from using observables is void and null.

[benjamingr]

@mpj note that promises solve this by emitting an event whose handler defaults to logging the error - so it's possible to make this not swallow errors.

---

@jhusain I think our only real option is to do what @zenparsing suggests so observables behave the same way as DOM event listeners or Node event listeners.

Node has a special behavior in event listeners where .on("error" is given a chance to clean up first before the exception is thrown "globally", maybe it makes sense to fire error on observables to allow this. The DOM just assumes the user code handles this.

I also don't understand your code in:

try {
  observable.subscribe({ next(v) { throw TypeError })
} catch(e) {
  // worked before when observable was unicast,
  // but when switched to multicast a CompositeError thrown instead
  if (e instanceof MyCustomError) {
    //...
  }
}

Either the observable fires immediately when you subscribe to it at which point it's fine since every handler subscribing would have a chance to clean up when it is subscribed to (so the code above would work with multicast).

Or the observable is emitting asynchronously so you wouldn't get in the catch block anyway. You can't just try/catch around observables and assume it'd work since it violates a much stronger assumption than uni/multicast IMO - that observables might emit asynchronously and your code should be agnostic to that.

[acutmore]

+1 for HostReportErrors

I feel it is common practice/courtesy across many different programming languages/communities to not intentionally throw from inside a listener/callback as part of the designed program control flow.

So making sure the error is logged to a global, non Observable specific, error sink which is highly likely to be monitored so errors are picked up and fixed seems the correct choice to me. As opposed to encouraging/facilitating errors to be caught and handled locally to the subscribe

[jhusain]

@benjamingr the issue I'm trying to demonstrate with the code example you reference is that if we aggregate all observer errors and throw (the only way to avoid any form of swallowing), the error type changes when moving from unicast to multicast. Therefore the leak persists.

@mpj Using the same approach as Promises is really the only way forward here. The debate over error swallowing was effectively finished when Promises were standardized. Given the example in this thread, I'm now convinced that the committee made the right decision.

I'm strongly in favor of using whatever mechanism Promises use to signal uncaught errors. If that is HostReportErrors, that's what we should do.

[jhusain]

@zenparsing I'm suggesting that SubscriptionObserver will never allow errors to propagate. Presumably the responsibility to prevent propagation could lie with the subscribe impl, but not sure why we'd want that.

[kirkshoop]

Rxcpp defaults on_error() to call terminate(), if no function is supplied to subscribe(). This sounds like the same principle as HostReportErrors. So obviously, I think that is the correct default. :)

[zenparsing]

@jhusain Promises use HostPromiseRejectionTracker which is geared specifically to the challenges of reporting promise rejections. As the note says, it's called both when a promise is rejected without any handlers and also when a handler is added to an already-rejected promise.

Since a handler can't be added "later" to an error coming from an observable/observer, I don't think it would work too well to use HostPromiseRejectionTracker. HostReportErrors is probably more appropriate.

Also, it makes sense that we would model errors which happen in observables using the same kind of mechanisms that have worked with EventTarget.

[jhusain]

@zenparsing Would you report to HostReportErrors if an error was sent to an Observer twice, like so:

let observable = new Observable(observer => {
  observer.error("FAIL");
  observer.error("FAIL");
  return () => {};
});

observable.subscribe({ next(v) { }, error(e) { }, complete(v) { } });

This would be pretty arduous, because developers would always have to check the closed() value prior to notifying. Having the observer noop all calls after the subscription has closed is much more ergonomic.

I'm inclined to send errors to HostReportErrors in the following situations:

1. a next, error, or complete handler throws
2. no error handler exists to receive an error

Any thoughts?

[trxcllnt]

I thought we'd long ago decided that errors thrown by next/error/complete is undefined/default behavior and should propagate to the global scope (same as errors thrown in node-flavored callbacks). This is the behavior Alex Liu and the Netflix node teams desire so they can get an accurate core-dump, and something we can support with minimal effort in Rx.

If we are going to catch errors thrown from next/complete, forwarding them on to the error handler (like I've argued for in the past) would also solve this problem. If the first observer throws, the error is forwarded to its error handler, and all observers awaiting notification. If an observer's error handled throws, that error is used instead. If the last Observer's 'error' throws, the error is re-thrown globally.

[benjamingr]

@benjamingr the issue I'm trying to demonstrate with the code example you reference is that if we aggregate all observer errors and throw (the only way to avoid any form of swallowing), the error type changes when moving from unicast to multicast. Therefore the leak persists.

Right but that's not really an issue since the catch (e) { strategy doesn't work in either unicast or multicast anyway. Since it's asynchronous you have to report the error globally, don't even run the other observers let alone execute their errors. I'm also in favor of HostReportErrors which aligns with the DOM event model.

[trxcllnt]

see also: https://github.com/tc39/proposal-observable/issues/47

[zenparsing]

@jhusain

Having the observer noop all calls after the subscription has closed is much more ergonomic.

I agree.

I'm inclined to send errors to HostReportErrors in the following situations: ...

Agree with this as well.

A related question (which I think you alluded to in the original post): currently the spec says that the return value from the observer's callback is transmitted back to the caller through SubscriptionObserver. Does the change proposed here mean that we need to revisit that choice? Should the return value from the observer callbacks be discarded?

[jhusain]

Taking the principle of "push-only" further, it makes no sense whatsoever that you cannot receive an error thrown from next/error/complete, but you can receive a return value. Being fully push means that no data is pulled whatsoever. Under the circumstances I see no justification for receiving anything from next/complete/error but undefined. In addition to catching errors, SubscriptionObserver must suppress all return values from downstream Observers and return undefined.

The change that the various Observer notifications return void actually brings the proposal closer to most existing implementations. Furthermore it's never been fully explained what happens to the return value when notifications are scheduled (ex. using observeOn operator). This lossiness smells, because it leaks whether downstream notifications are handled sync or async.

[zenparsing]

Taking the principle of "push-only" further, it makes no sense whatsoever that you cannot receive an error thrown from next/error/complete, but you can receive a return value.

I think that makes sense to me. My original intention was to easily allow async observers (where the callbacks can return a Promise), but I agree it doesn't "come together" nicely.

Another question comes to mind:

Currently we allow errors thrown from the cleanup function to propagate to the caller of unsubscribe():

let observable = new Observable(observer => {
  return () => { throw new Error('error in cleanup') };
});
let subscription = observable.subscribe({});
subscription.unsubscribe(); // Throws a catchable error here

In light of the proposed changes here, does the current behavior still make sense?

[jhusain]

BTW @trxcllnt I'm increasingly warming to your idea that throwing from next should forward to the error handler. Will comment more in the corresponding issue.

[jhusain]

@zenparsing with respect to what subscriptions do, it's not clear to me that allowing subscriptions to throw interferes with our ability to cleanly abstract over unicast and multicast Observables. Can you think of an example where this would be problematic?

[zenparsing]

@jhusain I can't think of any. Allowing the subscriber to "see" errors coming from the producer doesn't appear to violate the one-way flow constraint.

I'm concerned about how this change will impact frameworks though. For instance, in an express app any errors that occur within a route handler need to be catchable by the framework so that it can invoke it's per-request error handler (which might show a 500 page or something). What do you think?

[jhusain]

Seems to be same hazard as Promise error swallowing to me. The Zones proposal seems like the best way to address this issue.

On Wed, Nov 30, 2016 at 8:37 AM, zenparsing notifications@github.com wrote:

@jhusain https://github.com/jhusain I can't think of any. Allowing the subscriber to "see" errors coming from the producer doesn't appear to violate the one-way flow constraint.

I'm concerned about how this change will impact frameworks though. For instance, in an express app any errors that occur within a route handler need to be catchable by the framework so that it can invoke it's per-request error handler (which might show a 500 page or something). What do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tc39/proposal-observable/issues/119#issuecomment-263923382, or mute the thread https://github.com/notifications/unsubscribe-auth/AAcfr9Su1ztBSHjQjzofajgTRHQZjVCtks5rDaZlgaJpZM4K-PPE .

[zenparsing]

After thinking this over for a while, I'm not sure I quite understand the rationale for disallowing return values and exceptions to pass to the producer. From the motivating example:

try {
  observable.subscribe({ next(v) { throw TypeError })
} catch(e) {
  // worked before when observable was unicast,
  // but when switched to multicast a CompositeError thrown instead
  if (e instanceof MyCustomError) {
    //...
  }
}

First, why should we be able to make the assumption that switching observable with some other observable (with different behavior) should result in the same throwing behavior for subscribe? Is the switch from unicast to multicast special for some reason? Why?

Second, it's pretty easy to create an Observable from the current spec which swallows errors:

new Observable(sink => {
  sink = new SwallowingObservable(sink);
});

So why can't producers opt-in to this behavior if they want?

I'm pretty confident that users will come up will cool use cases for returning values back to the producer if we allow it. The danger here is artificially limiting possibilities like:

observable.subscribe({
  async next() { },
});

[jhusain]

When switching one Observable from another, it's perfectly reasonable to expect different output and side effects. Switching an Observable from unicast to multi-cast (ie publish().refCount()) is a gentler change. The intention is not to observably switch either the side-effects or output. You simply want to have more consumers. I argue it is very valuable to be able to make this change transparently. I've performed this refactor many times.

Why so confident that users will come up with cool use cases for returning values to the producer? Are there are any userland examples you're aware of? Given that the main rationale for Observable is composition, and the return value from notifications is difficult to meaningfully compose in fan out operations (flatMap), I don't think it pays for its semantic cost.

[jhusain]

To summarize, I don't see a clear use case for return values. Furthermore return values would come at the expense of an invariant that I think is very valuable: the ability to safely refactor from unicast to multicast. That's why I favor eliminating return values.

On Tue, Dec 6, 2016 at 2:37 PM, Jafar Husain jhusain@gmail.com wrote:

When switching one Observable from another, it's perfectly reasonable to expect different output and side effects. Switching an Observable from unicast to multi-cast (ie publish().refCount()) is a gentler change. The intention is not to observably switch either the side-effects or output. You simply want to have more consumers. I argue it is very valuable to be able to make this change transparently. I've performed this refactor many times.

Why so confident that users will come up with cool use cases for returning values to the producer? Are there are any userland examples you're aware of? Given that the main rationale for Observable is composition, and the return value from notifications is difficult to meaningfully compose in fan out operations (flatMap), I don't think it pays for its semantic cost.

[zenparsing]

Are there are any userland examples you're aware of?

Yeah, I have a work project where we use zen-observable (which supports return values), and we use a multicast observable for publishing and subscribing to server-side events which might happen on an HTTP request. The subscriber can return a promise and the publisher can await all of the various results before proceeding on with the request processing.

We can change things so that instead of returning a promise we set a promise on the "event" object, but given async functions it's a lot less ergonomic to do it that way.

[staltz]

:-1: for returning values back to the producer. In that case we could just rename this to Flowable. Like in RxJava v2: https://github.com/ReactiveX/RxJava/wiki/What's-different-in-2.0

There is so much that is battle proven in RxJS that there isn't a lot of reason to deviate from it. If we're looking into making this a 2-way communication primitive, I'd rather back some simpler and cleaner primitive like CSP channel.

[trxcllnt]

From what I remember, the initial back-pressure work in RxJava proved returning values from next doesn't compose, right @jhusain?

[RangerMauve]

@zenparsing

The subscriber can return a promise and the publisher can await all of the various results before proceeding on with the request processing.

In my opinion, for duplex communication like that, async iterators are a much nicer fit. Though they aren't as far along last I checked. I agree that removing return values and making things more push based would be useful for the spec. It would also help differentiate Observables from Iterators and Async Iterators. If the two are too similar, it could be confusing as to which tool is right for what job.

[benjamingr]

Though they aren't as far along last I checked.

They're actually very far along - being a stage 3 proposal while observables and cancellation tokens are both stage 1. They're also much simpler to model.

[RangerMauve]

@benjamingr Oh awesome! It's probably the most exciting feature for me after async functions, to be honest. However I haven't seen much activity in the past few months, so I was worried it was going stale.

I think what I meant by "far along" is that there wasn't as much use of it in the ecosystem as far as I can see. Most people I've talked to about it IRL haven't heard about Async Iterators, while most of them have heard of Observables in one way or another. As well there's a lot of libraries and projects out there actively using Observables, whereas I don't know of anything that uses Async Iterators.

(Sorry for going so far off topic)

[zenparsing]

returning values from next doesn't compose

Agreed, but doesn't the same argument apply to iterator.next(someValue)? This seems to be an argument for not using the return value, as opposed to an argument for absolutely blocking the return value. It seems nanny-ish.

JS isn't Java (or TypeScript); the base type doesn't need to be strict with respect to static (return) typing. A base API which supports both push-only observable and "Flowable" seems like a win to me. Or to put it another way, having a base API which merely allows the possibility of return values doesn't impinge at all on push-only abstractions.

@jhusain Regarding the throwing behavior with respect to unicast/multicast, does the hazard still persist if we just write all multicast observables as swallowing, so that the behavior changes from possibly throwing an error from subscribe to not throwing at all?

[benjamingr]

If I recall correctly @jhusain has expressed regret for .return(value) but I might not be recalling correctly.

having a base API which merely allows the possibility of return values doesn't impinge at all on push-only abstractions.

I'd like to back this up in that I haven't seen any abuse in iterators. Then again - I'm sad we don't have built-in combinators on those yet.

[jhusain]

@zenparsing want to echo the view expressed that the use case you provide for allowing the Observable to receive the return value of notifications is better served by an async iterator. Notably async iterator also would allow the hurry-up-and-wait interaction you describe to compose, whereas Observable would not.

@zenparsing You have expressed the concern that precluding return values is heavy-handed, and I sympathize with that concern. Allow me to explain my perspective: it's my view that we should be designing an API that causes developers to fall into the pit of success. Encouraging developers to use semantics that effectively disappear when certain compositions are applied creates re-factoring hazards.

Allowing Observables to see the return values of observer notifications exposes two big abstraction leaks:

• whether an Observable is unicast or multicast (ex. publish().refCount())
• whether a consumer handles notifications synchronously or asynchronously (ex. observeOn)

Both of these refactors are very common. The former hazard I've already explained. The latter is common in situations where developers decide that they want to handle the output of an Observable asynchronously on a scheduler. For example a game developer may discover that they get better performance by scheduling the handling of notifications on request animation frame. As another example, a developer may discover they need to asynchronously schedule notifications on the event loop in order to ensure that the system has a chance to enforce certain invariants before the notification is handled

Scheduling the handling of a notification necessarily precludes the Observable from receiving the return value on the stack. A late decision to apply this composition would cause pain for developers who had written their system around the assumption that they could receive return values from notifications. As I've mentioned previously, the same could apply if a developer decided to refactor their Observable to be multicast in order to allow multiple consumers to observe its output.

I think it is better ergonomics to preclude the use of semantics than it is to give developers semantics and then remove them unexpectedly. Under the circumstances my view remains that we have more to gain from giving developers the ability to fearlessly compose than we do from giving them the ability to see the return value of notifications in very constrained situations. I have yet to see a compelling use case for the latter.

[zenparsing]

@jhusain (Thanks for putting up with me pushing back on this 😄)

the use case you provide for allowing the Observable to receive the return value of notifications is better served by an async iterator

Hmm... I don't think so, because it's a pub-sub event system and push semantics seem more natural.

All of the arguments you give are great arguments for not using the return value channel in RX-style reactive programming where composition is key. I think there are use cases for Observable (as a primitive) which fall outside of that philosophical word-view, though. JS is a pretty free-wheeling language. I'm not sure I see the danger or footgun that would justify artificially restricting those use cases.

There's a subtle difference between allowing something and encouraging something. I don't think simply allowing the return value to propagate through SubscriptionObserver encourages the use of that value, any more than allowing an argument to iterator.next encourages the use of that value.

[jhusain]

@zenparsing you make a good point about your example being best served with pub/sub. In that case it would perhaps be better served by an observable designed specifically to support back pressure (Flowable). Like async iterator, this type implements back pressure in such a way that it composes and as such is a more appropriate candidate. Other than back pressure, can you think of a single use case for allowing Observables to receive values from notifications? I've racked my brain and have not been able to come up with a single example that is not better served with generators.

I think there is a long precedent of the committee making decisions to protect developers against precisely the types of foot guns I described. While it may seem like generators are an obvious counterexample, I argue that the comparison is actually inappropriate. While the generator proposal predates me by many years, I understand from @littlecalculist that the main justification for generators was to support coroutine-like interactions, not iteration. There were a lot obvious use cases for coroutines in a single-threaded language like JavaScript - mostly centered around supporting asynchronous programming. My understanding is that supporting iteration was a secondary goal (@littlecalculist it's been a while since we discussed this so please correct me if my recollection is off).

In contrast, the main use case for Observable is not bidirectional communication. The justification we have presented to the committee and the community was improved support for building composable, event-driven programs. In this context allowing a return value in order to match generators is self-defeating because it makes Observables more hazardous to compose.

Allow me to challenge your argument that preventing the producer from seeing a return value is unduly restrictive. If we were to consistently apply that argument, we might perhaps conclude that SubscriptionObserver should not suppress additional error and complete notifications delivered to an Observer after a subscription has been closed. It's easy to contrive a situation in which a developer might want to deliver multiple error notifications to an observer. If we are willing to suppress messages from the producer to the consumer to preserve invariants we care about, it's not clear to me why suppressing messages from the consumer to the producer is unduly restrictive.

Sent from my iPhone On Dec 7, 2016, at 1:58 PM, zenparsing notifications@github.com wrote:

@jhusain (Thanks for putting up with me pushing back on this 😄)

the use case you provide for allowing the Observable to receive the return value of notifications is better served by an async iterator

Hmm... I don't think so, because it's a pub-sub event system and push semantics seem more natural.

All of the arguments you give are great arguments for not using the return value channel in RX-style reactive programming where composition is key. I think there are use cases for Observable (as a primitive) which fall outside of that philosophical word-view, though. JS is a pretty free-wheeling language. I'm not sure I see the danger or footgun that would justify artificially restricting those use cases.

There's a subtle difference between allowing something and encouraging something. I don't think simply allowing the return value to propagate through SubscriptionObserver encourages the use of that value, any more than allowing an argument to iterator.next encourages the use of that value.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[zenparsing]

Let me sidestep a bit here and say that I certainly don't think suppressing the return value is fatal. What concerns me more is suppressing exceptions, and how that makes it difficult for a framework to handle errors.

You mentioned that Zones would be an appropriate solution to this, and I generally agree. Unfortunately, we don't have Zones and probably won't for a very long time. And leaving aside Zones, the situation is more troubling for Observable than for Promises because you can always return a promise down the stack; a "framework" (which could be anything from a big framework to an ad-hoc pub-sub thing within Express) has an opportunity to get ahold of that and deal with rejections appropriately for the given context.

With the changes proposed here, we seem to be in a situation where (absent Zones or somesuch) and error thrown from an observable will crash a node program as a whole. Is that what we want?

[jhusain]

You mentioned that Zones would be an appropriate solution to this, and I generally agree. Unfortunately, we don't have Zones and probably won't for a very long time.

I continue to think we benefit from solving this problem once in the ecosystem. Once it is finally solved there will no doubt be a host ecosystem integration points built around it. I'm willing to wait for the right solution. Regardless of either of our individual viewpoints on the subject, I believe that taking a different approach to handling errors in Observables and Promises will be highly controversial in committee. I believe that we will have to meet a pretty high bar to justify semantic differences in the way that Observables and Promises handle errors. And leaving aside Zones, the situation is more troubling for Observable than for Promises because you can always return a promise down the stack; a "framework" (which could be anything from a big framework to an ad-hoc pub-sub thing within Express) has an opportunity to get ahold of that and deal with rejections appropriately for the given context.

Assuming I have understood you correctly, I think you are mistaken with respect to the question of how frameworks will integrate with Observables. It is unlikely that frameworks will expect app code to subscribe and alert the framework in the event of error. Instead, frameworks will likely call app code which returns an Observable and then the framework code will subscribe to it. I believe that this is the dominant pattern today in frameworks which integrate with Observables like Angular 2 and CycleJS.

Frameworks have a strong incentive to control when side-effects occur, and the laziness of Observables allow side effects to be encapsulated until the framework is ready to execute them. Those frameworks which trigger subscription will have no difficulty intercepting errors. Otherwise keep in mind that the forEach method creates a Promise which will reject in the event that the code that handles a notification throws.

With the changes proposed here, we seem to be in a situation where (absent Zones or somesuch) and error thrown from an observable will crash a node program as a whole. Is that what we want?

I would like different Node developers to be able to apply any policy that they want in the event of an error. Our best chance at making this happen is to be consistent with the rest of the ecosystem, so that when the problem of error swallowing problem is solved - via Zones or some other solution - developers can leverage those hooks to apply whatever policy they like.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[zenparsing]

I believe that taking a different approach to handling errors in Observables and Promises will be highly controversial in committee

I see this used way to often as an excuse to shut down a debate. The committee isn't really that good at design, to be honest.

Anyway, let me put it another way. I can't implement this change on the Express app that my team works on because AFAICT any error in an event listener in our pub-sub system will crash the process. We just can't do that.

[ljharb]

Wait, why would it crash the process? Unhandled rejections don't do that by default.

[zenparsing]

What else could it do? unhandledrejection/rejectionhandled don't make sense, because you can't later attach a handler, like promises.

You could have an observableerror event I suppose, but (unless I'm not seeing something) that's no better for this use case. We want to be able to catch and funnel exceptions that occur within a handler through Express's error routing mechanics, not deal with it using an out-of-band global error event handler.

[ljharb]

Ah, I'd misunderstood - so you're saying that with the changes proposed here, there'd be no way to react to/recover from the error?

[jhusain]

I believe that taking a different approach to handling errors in Observables and Promises will be highly controversial in committee

I see this used way to often as an excuse to shut down a debate.

As I stated, "I believe that we will have to meet a pretty high bar to justify semantic differences in the way that Observables and Promises handle errors." Beyond being a pretty bland observation, that statement was an invitation to meet that bar. Do you think that the arguments that have been made in this thread will be compelling enough to change the committee's stance on error swallowing? On the contrary after having several misgivings myself, this debate has convinced me of the wisdom of that decision. The committee isn't really that good at design, to be honest.

Anyway, let me put it another way. I can't implement this change on the Express app that my team works on because AFAICT any error in an event listener in our pub-sub system will crash the process. We just can't do that.

Once Observables are in the language, fixing this issue is just a PR away. Express can integrate with Observables just as other frameworks have: your request handler could return an Observable and express could subscribe to it. This would allow Express to receive errors that occurred during processing. I think it's reasonable to expect that libraries will move to support new types in the ecosystem.

Should the committee be making decisions about the next 10 years based on what frameworks do right now - before they've even had a chance to integrate? Reasonable people can disagree, but I'm not convinced that this would be good design.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[zenparsing]

I apologize for letting some frustration bubble over there.

It's not so much about frameworks, and I regret couching it in those terms. Here's a simplified version of the use case:

function eventHubMiddleware() {
  let listeners = [];

  let events = new Observable(sink => {
    listeners.push(sink);
    return () => {
      let index = listeners.indexOf(sink);
      if (index >= 0) {
        listeners.splice(index, 1);
      }
    };
  });

  return (req, res, next) => {
    req.publish = event => {
      listeners.forEach(listener => listener.next(event));
    };
    next();
  };
}

app.use('a', (req, res, next) => {
  try {
    req.publish(new SomeEvent());
  } catch (e) {
    // We have a problem if I can't catch here. It basically means
    // that we can't use Observable for this event hub abstraction
    next(e);
  }
});

[RangerMauve]

@jhusain

frameworks will likely call app code which returns an Observable and then the framework code will subscribe to it

Could you show an example of how subscribing to an observable would allow a framework to catch errors? If an error gets thrown earlier up the chain, how would it be caught by the subscription that's at the end of the chain? Or would the framework need to provide the Observable that's at the top of the chain?

[benjamingr]

@ljharb

Wait, why would it crash the process? Unhandled rejections don't do that by default.

Actually, they will - it's just a matter of getting the appropriate GC hooks from V8.

[ljharb]

@benjamingr unhandled rejections will eventually crash on GC, sure, but in this case that'd be when the Observable is GC'd, certainly not right away.

[zenparsing]

In this case it's not the same because a handle to the Observable does not give you any opportunity to handle the error later.

[benlesh]

FWIW, I just had to help the web team at Netflix with an issue directly related to this issue. They were sharing a multicast observable between three consumers, one consumer's subscriber errored and the other consumers were no longer notified, as the source multicast observable was terminated due to the error rethrown in the down stream handler unwinding the stack back to where the loop was that was notifying the observer list for the multicast. The loop breaks and the source is torn down.

It makes the abstraction leaky to keep this behavior.

I used to fight against this sort of error trapping (ala promises). But I was completely wrong I think. Promises might not have to do it because they schedule asynchronous results, but I believe observables should definitely swallow errors if they dont have an error handler, or if the error is thrown in the next handler (or error or complete).

In fact, I feel like I've argued with @domenic about this, and I was wrong.

[zenparsing]

I'll keep thinking about it, but at the present I feel like swallowing or crashing the process is not going to work for our use case at all.

[zenparsing]

In order to deal with this change (where exceptions thrown from "next" are not allowed to propagate to the caller), we'd basically have to wrap our next() code in a try-catch block and if an exception occurs, add the error information to the supplied event object.

Something like this:

// Runs in the context of an Express HTTP server!
serverEvents.on('newsletterSubscriptionsLoad').subscribe(event => {
  try {
    // Do all the things in here
  } catch (error) {
    // Communicate the error back to the dispatcher
    event.errors.push(error);
  }
});

This is both error-prone (because wrapping an entire handler is easy to forget) and awkward. It's basically boilerplate to work around an unreasonable data-flow restriction.

It really seems like not being able to handle errors "locally" is a non-starter. Promises are a completely different beast because as you pass the promise down the call stack it gives callers the ability to handle errors "locally" (but asynchronously, of course).

In some cases (like DOM event listeners on the client) it might totally make sense to swallow errors, but the current Observable API can easily be used to create such abstractions.

[benlesh]

It really seems like not being able to handle errors "locally" is a non-starter.

How could would handle them locally, though? That would only really work in the case of a synchronous observable.

function test(source$) {
  try {
    source$.subscribe(() => { throw new Error('oops'); });
  } catch (err) {
    console.log('handling error:' + err.message);
  }
}

const sync$ = new Observable(observer => {
  observer.next('hi');
});

const async$ = new Observable(observer => {
  const id = setTimeout(() => observer.next('hi'));
  return () => clearTimeout(id); // or cancel token equivalent (not discussed here)
});

test(sync$); // error is handled
test(async$);  // error will not be handled

At best you could use something like window.onerror to "handle" that asynchronously thrown error, but you'd be way better off with something like .forEach().then() to handle it because at least then you'll have some idea of where it came from. Either way, if it's a multicast observable, in the second test, you're going to get that nasty behavior I mentioned in my previous comment.

To be clear, I don't love the idea of error trapping. But I think it's probably the right thing to do.