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: #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 ***@***.***> 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 <#119 (comment)>, 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 ***@***.***> 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]

👎 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.

[zenparsing]

@isiahmeadows I believe there are use cases (some Observable operators) where error and complete need to be delivered synchronously, but I can't remember exactly what they are.

[jhusain]

-- post pretty heavily edited because it was sent accidentally prior to completion.

By definition, noncooperating consumers don't care if another consumer fails to process a Promise result and as such should not be impacted. Please explicitly concede or contest this point in your response. Sure, add "multicast" to that list of reasons.

It's the only reason. This is easy to demonstrate this with a thought experiment: if we decided to add a "subscribe"-like method to Promise tomorrow, would the Promise swallow errors thrown from handlers? Yes. Why? Because promises are multicast. This is important, because you have offered the reified completion value of promises as a justification for why Promises can catch errors, but Observable's cannot. Please unambiguously concede this argument doesn't hold water so we can move on to the other arguments.

The only explanation for EventTarget semantics is that the specification authors thought that producer interference was not ergonomic. Maybe so, but we're not designing EventTarget, we're designing a low-level primitive that can be used to express things like EventTarget.

You are trying to design a low-primitive to support a particular implementation of a pattern for which no precedent has been demonstrated outside of your own code. I am trying to design a low-level primitive which ergonomically meets the motivating use cases which have been presented to the committee and community: * compositional push programming * multicast

We must reach the following conclusion: a global error sink is necessary in order to be able to write push and pull programs which are semantically equivalent. Sorry, I'm not really following that line of reasoning.

I agree this is worthy of clarification and will provide some.

Local error handling of producer errors is possible by providing an error handler to the Observer. Yeah, I knew you were going to say that. 😄 And no, it is not reasonable to expect that every observer wrap its callback code with try/catch in order to prevent program crashes when there is a handler somewhere on the call stack that wants to handle it.

AFAIK Host Report Errors will not cause a program crash. More importantly we've established that local error handling is possible, but you don't think it is ergonomic. I think it's important to establish these distinctions. For the record I see nothing wrong with try/catch blocking code to detect consumer errors. Do you acknowledge that if an Observable happens to notify asynchronously, this is the only option for detecting consumer errors? If so, isn't the try/catch in handler code unavoidable without leaking the details of the Observable implementation?

Please justify why enabling a particular implementation of an admittedly uncommon use case at the expense of ergonomics in common use cases is the right tradeoff. Have we seen evidence that the current design offers bad ergonomics for the "common" case? More below. I think those of us who would like to see common use cases like multicasting and push stream composition addressed by this proposal are entitled to a response. The only case I've seen in this thread where the error-silencing semantics is needed is multicast. But Observable (the base API represented by this spec) is not multicast! And multicast abstractions (like Subject and EventTarget) can easily be written to do the error silencing thing that you want. Are we trying to limit the expressivity of Observable in order to fix a problem with RxJS's Subject implementation?

No. We're trying to design an interface which can be ergonomically consumed without leaking the details of its implementation. The interface of Observable is much more significant than its implementation, because it affects many more developers. If the contract offers no guarantee against throwing, that's a clear signal to implementers that they can bubble errors from consumers. It's also a clear signal to consumers that they must guard agains throwing, unless they have some special knowledge about the underlying implementation. You argue the Observable API should be this... class Observable { // ...snip subscribe(Observer): Subscription | Error } Others argue it should be this... class Observable { // ...snip subscribe(Observer): Subscription } Do you deny the latter is simpler? Do you concede that requiring a try/catch on the off-chance that an Observable may notify synchronously introduces additional consumer burden?

Thanks for the code example. I think we can all agree that caching, in general, creates all kinds of headaches for testing (I deal with that all the time at work). Beyond that, I think you are somewhat incorrect in stating that the user can ignore Promise errors by simply not attaching an error handler. We are slowly moving to a world where unhandled rejections are considered program errors, and dealt with as such (e.g. terminating the process in Node). In the code you provided, the observer should absolutely have a no-op "error" method if it wants to ignore errors.

I deliberately used a UI example, because it's common place to ignore errors without tearing the process down. Are we suddenly going to change browsers to crash when Promise handlers are omitted? I think not. In Node this behavior will no doubt be configurable, and tearing down the process might make complete sense there. The point is in either case the developer can omit the call "error" as a signal they don't intend to handle the error locally and let the global policy take over.

[ljharb]

By default, node plans to crash on unhandled errors when a Promise is GCed, but can't ever practically do that by default prior because a later-handled rejection is a critical use case for Promises. Users will be able to opt into immediately crashing by adding their own unhandled rejection callback, of course. Browsers would surely be limited by the same compat restrictions.

[jhusain]

@zenparsing Can you quickly answer a few questions? I've been reviewing the thread and I'm not clear on exactly what semantics you'd like to see for Observable at this point. Can you quickly summarize? I have the following questions:

• do you believe consumer errors should be sent to "error" and the Subscription should be closed?
• do you believe consumer errors should be allowed to propagate through subscribe in the event notifications are sent synchronously?
• What state should the consumer expect the subscription to be in if subscribe throws? Closed? The consumer has no Subscription in the event of a thrown error, so if an Observable doesn't explicitly guard against errors thrown from notifications wouldn't there necessarily be a memory leak?

For my part I contend that consumer errors should be caught and reported to HostReportErrors, and should not close the subscription. Furthermore I think "subscribe" should report producer errors to HostReportErrors in the event an "error" callback is not provided.

Thanks

[benlesh]

Couldn't we fix the issue by making appropriate adjustments to Subject so that it's multiple observers don't interfere with each other?

If we "fix" this even just for subjects, it would break your use case of wrapping subject.next(x) in a try/catch.

[zenparsing]

@jhusain I think the spec should stand as-is. That is:

• Consumer errors are not sent to error.
• Errors that occur in the subscriber (executor/whatever-you-want-to-call-it) function are sent to error if present, otherwise propagate normally.

if we decided to add a "subscribe"-like method to Promise tomorrow, would the Promise swallow errors thrown from handlers? Yes. Why? Because promises are multicast.

Sorry, but "subscribe-for-Promise" aight gonna happen.

You are trying to design a low-primitive to support a particular implementation of a pattern for which no precedent has been demonstrated outside of your own code.

Please, let's try to avoid ad hominem.

We're trying to design an interface which can be ergonomically consumed without leaking the details of its implementation

But you haven't proved how the proposed changes help this goal more than they hurt, other than providing abstract and opinionated notions about how Observable ought to be used. In all of the code examples I've looked at so far, the issue can be resolved by fixing the Subject/multicast implementation to do what exactly you advocate, without violence to the exiting low-level semantics. And that fix can be made one time, in one place, and multicast users never have to think about it again.

I invite you to help me understand why that's not sufficient.

[zenparsing]

@Blesh

If we "fix" this even just for subjects, it would break your use case of wrapping subject.next(x) in a try/catch.

Someone (trying to keep the personal "I" out of it) that wants to catch errors from a subject.next can just use a different "subject-like" abstraction that allows errors to propagate (e.g. CompositeError in the OP). Observable is a building-block and users should be able to build lots of different things with it.

[jhusain]

@jhusain <https://github.com/jhusain> I think the spec should stand as-is. That is: - Consumer errors are not sent to error. - Errors that occur in the subscriber (executor/whatever-you-want-to-call-it) function are sent to error if present, otherwise propagate normally. How do you differentiate between consumer errors and producer errors if

consumer errors are allowed propagate?

if we decided to add a "subscribe"-like method to Promise tomorrow, would the Promise swallow errors thrown from handlers? Yes. Why? Because promises are multicast. Sorry, but "subscribe-for-Promise" aight gonna happen.

I'm not suggesting it is. It was a thought experiment to demonstrate the irrelevance of reified completion values with respect to Promise error catching. I don't understand your resistance to conceding this point when there are other concerns to discuss.

You are trying to design a low-primitive to support a particular implementation of a pattern for which no precedent has been demonstrated outside of your own code. Please, let's try to avoid ad hominem.

Quite right. My apologies.

We're trying to design an interface which can be ergonomically consumed without leaking the details of its implementation But you haven't proved how the proposed changes help this goal more than they hurt, other than providing abstract and opinionated notions about how Observable *ought* to be used. In all of the code examples I've looked at so far, the issue can be resolved by fixing the Subject/multicast implementation to do what exactly you advocate, without violence to the exiting low-level semantics. And that fix can be made one time, in one place, and multicast users never have to think about it again.

What do you mean "fix"? Should consumer errors be allowed to propagate through the stack or not? If not, we should prohibit it. By allowing the possibility you make it possible for an implementation to intentionally allow this. This means that consumers have to provide an explicit, empty error callback to ignore errors. Otherwise we're leaking implementation details again. This isn't how Promises work, and developers will be surprised by this. Can you answer this directly? Why do you feel it is okay for consumers to have understand the implementation details of an Observable in order to consume it?

[zenparsing]

Apologies for the short reply, I need to leave the laptop for a while.

What do you mean "fix"? Should consumer errors be allowed to propagate through the stack or not?

As far as this spec goes, yes. It will make sense in some multicast scenarios to not allow errors to unwind the stack. It's not an either-or situation, though. The fact that (some) multicast use cases want to disallow propagation doesn't mean that we have to disallow it for all observables. Case-in-point: EventEmitter.

This means that consumers have to provide an explicit, empty error callback to ignore errors.

Yes, absolutely! This has always been my expectation. Just like in pull code you have to use try/catch to ignore errors:

try {
  for await (let value of asyncIterator) {}
} catch (e) {
  // Ignore
}

[jhusain]

Apologies for the short reply, I need to leave the laptop for a while. What do you mean "fix"? Should consumer errors be allowed to propagate through the stack or not? As far as this spec goes, yes. It will make sense in some multicast scenarios to not allow errors to unwind the stack. It's not an either-or situation, though. The fact that (some) multicast use cases want to disallow propagation doesn't mean that we have to disallow it for all observables. Case-in-point: EventEmitter.

The topic of EventEmitter has been covered thoroughly. We've established its possible to implement EventEmitter on an Observable that catches errors - though admittedly more complex. EventEmitter's propagating design is motivated by fast failure. The desired fail fast semantics of node developers is being addressed through platform hooks which Observable could use in the event of unhandled errors.

This means that consumers have to provide an explicit, empty error callback to ignore errors. Yes, absolutely! This has always been my expectation. Just like in pull code you have to use try/catch to ignore errors: try { for await (let value of asyncIterator) {} } catch (e) { // Ignore }

Given that push-pull equivalence is the basis for my argument that Observable should send Consumer errors to HostReportErrors, I think this is a fair point. Let's hypothetically say that throwing is acceptable for producer errors when an "error" handler is omitted. This doesn't solve the separate problem of distinguishing between consumer and producer errors. If we allow consumer errors to propagate and become producer errors, we still have no push/pull equivalence. No such thing can occur with iterators. I asked a question earlier that seems important. Let's say I want to intercept an error in consumer code. Given your proposed Observable semantics, do I have any choice whatsoever but to try/catch each handler in the observer? I can't use a try/catch around subscribe, because I will miss consumer errors in asynchronous notifications. Based on your proposed semantics, it seems as though the only reliable way of intercepting consumer errors is to try/catch each Observer method, no? To do anything else you would need to know the implementation details of the interface right?

[jhusain]

@zenparsing can you clarify your status on the committee for me? I was under the impression you were no longer championing proposals, but you seem to want to remain a champion of this proposal.

[dead-claudia]

@zenparsing

@isiahmeadows I believe there are use cases (some Observable operators) where error and complete need to be delivered synchronously, but I can't remember exactly what they are.

Opened #131 for clarification.

[zenparsing]

The topic of EventEmitter has been covered thoroughly.

Sorry, I still don't agree that it's acceptable to restrict the generality of Observable such that it eliminates EventEmitter semantics. To me, EventEmitter is a completely reasonable push stream abstraction.

If we allow consumer errors to propagate and become producer errors, we still have no push/pull equivalence. No such thing can occur with iterators.

Because of the caller/callee inversion when we go from pull to push. Regardless of push/pull, the call stack always points the same direction, and call stack semantics (such as error propagation) don't change.

[push]
========> (callstack)
--------> (data)

[pull]
========> (callstack)
<-------- (data)

Based on your proposed semantics, it seems as though the only reliable way of intercepting consumer errors is to try/catch each Observer method, no?

Well, those are the current semantics, not a proposed semantics 😄

If I understand your question, then yes, in accordance with normal call stack semantics, if a consumer throws an error then the producer must use try/catch to intercept it.

new Observable(sink => {
  setTimeout(() => {
    try { sink.next(1); }
    catch (e) { console.log('You goofed but who cares?'); }
  }, 5000);
}).subscribe({
  next(v) { throw new Error('oops') }
});

My impression is that more often, for async notifications, the producer will just let the error propagate down the call stack back to the host and let it become an unhandled exception.

can you clarify your status on the committee for me?

Consider me a concerned and informed citizen that doesn't want to see the champions of this proposal make a mistake. (Honestly, I don't know why I put so much effort into this!)

Plus, I got it right the first time. 😉

[jhusain]

The topic of EventEmitter has been covered thoroughly. Sorry, I still don't agree that it's acceptable to restrict the generality of Observable such that it eliminates EventEmitter semantics. To me, EventEmitter is a completely reasonable push stream abstraction.

Promises narrowed Node callback semantics by catching errors. The decision to have Observable narrow EE semantics by catching errors is consistent with this decision. When I pointed out the inconsistency between Promise and Observable error handling, you contended that Promises could catch errors because they allowed for local error handling. You explained that Promises enabled local error handling by having the "then" method return a rejected Promise in the event a callback throws. #119 (comment) I've since shown that Promise error catching behavior is necessitated solely by the fact that Promises are multicast, and therefore the return value of "then" is not an explanation for the inconsistency in error handling behavior between Promise and Observable. Furthermore I've demonstrated that local error handling is possible even if Observables catch errors. #119 (comment) Consumer errors can be handled locally by wrapping the body of the Observer's "next" method in a try/catch block, while producer errors can be handled locally by providing defining an "error" method on the Observer. #119 (comment) In absence of a new argument not based on the falsified claim that Observable error catching semantics are incompatible with local error handling, the only conclusion I can reach is that having Observables catch errors is consistent with the committee's decision to have Promises catch errors.

Based on your proposed semantics, it seems as though the only reliable way of intercepting consumer errors is to try/catch each Observer method, no? Well, those are the current semantics, not a proposed semantics 😄 If I understand your question, then yes, in accordance with normal call stack semantics, if a consumer throws an error then the producer must use try/catch to intercept it. new Observable(sink => { setTimeout(() => { try { sink.next(1); } catch (e) { console.log('You goofed but who cares?'); } }, 5000); }).subscribe({ next(v) { throw new Error('oops') } });

The current semantics make "subscribe" very difficult to explain to developers relative to "forEach." Ideally we would use the following simple, principled explanation: "subscribe" (like EventTarget and Symbol.iterator) doesn't close the stream in response to a consumer error, while "forEach" (like for...of) does. With the current semantics, this will seem to be true as long as notifications are dispatched asynchronously. Under the circumstances, it's easy to see how a developer might write the following code: observable.subscribe({ next(v) { /* do stuff that might fail, but shouldn't close the subscription */ } }); Unfortunately in the event a "next" notification is later dispatched synchronously, an error thrown from the next block may be treated as a producer error and cause the subscription to close. While deeply surprising, the decision of the Observable not to catch the consumer error is completely valid given the current semantics. It is simply an underlying implementation detail of the Observable. The fact that Observables can choose to close the subscription on consumer errors means that every developer who wishes to leave the subscription open when a notification code throws must explicitly wrap the "next" function in a try/catch block. observable.subscribe({ next(v) { // we always have to add the try to guard against the subscription being closed in the event of a consumer error try { /* do stuff that might fail */ } catch(e) { } } }); This makes the ergonomics of "subscribe" considerably worse, and its semantics very difficult to explain. Conversely the proposed semantics, in which Observables catch errors thrown from Observer notifications, gives us the clear explanation for "subscribe" and "forEach" stated earlier. The result is that the "subscribe" API is ergonomic to use in situations in which consumer errors are not intended to close the subscription: observable.subscribe({ next(v) { /* do stuff that might fail, but shouldn't close the subscription */ } });

My impression is that more often, for async notifications, the producer will just let the error propagate down the call stack back to the host and let it become an unhandled exception.

A consumer cannot be expected to know the details of the underlying implementation. The current semantics offer no guarantees that errors thrown from a notification won't close the subscription, which necessarily forces all consumers to be defensive.

can you clarify your status on the committee for me? Consider me a concerned and informed citizen that doesn't want to see the champions of this proposal make a mistake. (Honestly, I don't know why I put so much effort into this!)

If I understand correctly that you are no longer a committee member or champion, I intend to close this issue and move forward with error catching. I want to thank you for your participation in this thread, because working through this issue has helped me sort through my position. You are indeed informed, I appreciate your valuable input, and I hope you choose to continue contributing to this proposal. I'm well aware that closing this issue does not sidestep your concerns, and I appreciate that if you intend to block this proposal you can probably find a member to represent your concerns in committee. I would not be resentful if you chose to do so, because I recognize the existing semantics have some benefits, and I believe your arguments have been made in good faith. However as champion I must be able to rationalize my proposal, and I can't justify the current semantics. Thanks, J

[zenparsing]

Thanks, I will not be contributing any further to this effort and leave behind (yet another) TC39 effort with a bad taste in my mouth.

[trxcllnt]

fwiw @zenparsing I agree with you on this one, and even acknowledging the aggregate years of Rx experience of the people involved in this spec, I don't think any of us are clever enough to assert changes to the Observer grammar like this will ultimately improve on what @headinthebox (cc: @mattpodwysocki @bartdesmet etc.) came out with years ago.

RxJS5, which by default deviates from previous versions in many ways, only changes default behaviors that were already available before. One would hope the standardization process would be a way to introduce and recast a formally defined concept to the aesthetics of the language, instead of a way to claim untested, arbitrary modifications are somehow more correct, ex cathedra.

In conclusion, I declare that Carthage must be destroyed.

[jordalgo]

@jhusain - Pardon my confusion but what will be the new behavior if an error is thrown during subscribe ? That error will now be caught and reported to the global scope (HostReportErrors) ? Or is that only if there is no observer error handler to pipe the error to ?

[jhusain]

Errors thrown from observer notifications will be caught and reported to HostReportErrors, and will not close the subscription. This matches the behavior of Event Target and Iterable.iterate.

If the developer desires that the subscription be closed when there is an error in consumer code they can use forEach.

[jhusain]

For those who did not have the time to read this very long thread, @mattpodwysocki supports this change.

[benjamingr]

I just want to point out that I'm fairly happy with the resolution of this issue - although I'm not very pleased with the discussion itself.

I think that these semantics are not only the most reasonable - they manage to support both Node's use cases and the browsers' which I don't see how any different semantics would support.

[trxcllnt]

@jhusain why would the method that creates a Promise, a type without cancellation, be the method that auto-disposes on errors? Have I slipped into the twilight zone without realizing? This still is in direct conflict with the issues raised here, which are essential to members of the node community.

[trxcllnt]

@jhusain For those who did not have the time to read this very long thread, @mattpodwysocki supports this change.

From my understanding, @mattpodwysocki's statement implied it's the observer's job to try/catch errors that might happen in next, and not the Observable's job to try/catch next and forward caught errors to the Observer's error handler. He's been consistent on this in a number of discussions.

I don't believe @mattpodwysocki has weighed in on whether he's for Subscribers not cleaning up if the Observer throws, which is really the more fundamental issue at stake here.