-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Should this spec route errors occurring in the next handler to the error handler? #47
Comments
Good question. On one hand, this clearly illustrates the desire to handle errors in one place (the subscription error handler). On the other hand, The "doAction" best practice seems better suited to an alternate promise-returning subscription method: myObservable.forEach(x => {
blahblahblah();
}).then(null, err => {
reportAnError(err);
}); Although such a function is currently blocked on async task cancellation stuff. |
Well a sticky wicket about this is if you gracefully handle the error in the next handler, you're forced to unsubscribe yourself: let errorHandler = (err) => {
/* do things here */
};
let subscription = myObservable.subscribe(x => {
try {
someSideEffect(x.foo.bar);
} catch (err) {
errorHandler(err);
subscription.unsubscribe(); // better unsub!
}
},
errorHandler); |
... that is I presume the spec states that if an unhandled exception happens in the next handler, it will cause unsubscription currently. So if you do handle the error, it's not going to unsubscribe, so you'll have to do that manually. |
@Blesh Well, you would probably just call But the larger problem, to me, is that you don't want to invoke methods on an observer which has failed and may be in an invalid state. |
@Blesh an important detail in your example is that if the Observable onNext's synchronously, the
I acknowledge and agree 100%... on paper. But having worked the last five years with teams and projects using Rx, I'm inclined to question the pragmatism of this behavior.
This is certainly helpful, but I see more here. By encouraging users to move throw-prone side-effects inside the monad, we're encouraging them to mix pure and impure computation. Suddenly, operators that expect to handle legitimate exceptions from the pure computations before it have to understand and account for errors that would normally only have occurred in the subscription.
Agreed. If With the behavior Ben's described, source.subscribe(
(x) => {
try { sideEffectsAhoy(x); }
catch(e) {
if(!(e instanceof RecoverableError)) {
throw e; // re-throw the error to invoke the `error` handler and unsubscribe from source
}
console.log("Recovered from " + e.toString());
}
},
(e) => { console.error("Encountered a nasty error :("); }
); |
@Blesh @zenparsing and of course, the default |
Haha... Yeah, of course. LOL missed that one because I was thinking about RxNext. This is another place where a Subscriber is better than an Observer, because you're going to have access to unsubscribe of a Subscriber. |
See #50. Again, I don't think you should call any method on an observer which has failed. It really is "dirty" and can't be trusted at that point. Instead, users should use "do" which returns a promise, and deal with errors by way of that promise. |
@zenparsing what do you mean by "trusted?" Observers are stateless, and thus don't have a concept of "failure." I'm curious as to what would constitute a "failed" Observer. |
@trxcllnt I don't think we can assume that arbitrary objects provided to the API are stateless. In fact, given JS's OO bent, I think we have to assume the opposite. |
@trxcllnt.. I think he means an observer which has already processed an error. |
@Blesh @zenparsing The
After an Observer's Do you have a real-world example in which invoking |
So it seems like the answer is "No, errors in the nextHandler should just throw, and not be sent to the error handler"? confirm? @jhusain? @trxcllnt? @zenparsing? |
@Blesh That would be my answer. |
Sounds okay to me honestly. Closing for now. |
@Blesh this is in direct conflict with our teams' internal needs, so I disagree 100%. |
Oh? I guess I had forgotten you had hit this issue. Reopening. |
Why should we change the underlying semantics of the type? The internal teams requirements can be accommodated with a method: We could just add a method listen. Observable.listen() would translates to observable.doAction(onNext, onError, onComplete).subscribe(...)? Changing the types semantics seems high risk. Curious why the decision was made in the first place. Any feedback @headinthebox @mattpodwysocki ? JH
|
I am a bit lost what exactly the issue is. Try the following in RxJava and it prints class MainJava {
public static void main(String[] args) throws InterruptedException {
Observable.from(1,2,3,4).subscribe(
x -> {
if(x == 10) {
throw new RuntimeException("boom");
}
},
e -> System.out.println(">>> "+e.getMessage()),
() -> {
throw new RuntimeException("wham");
});
}
} |
@headinthebox if that is the case in RxJava/Rx.NET, then that's the behavior I'm arguing for here. That is not the present behavior in RxJS, as illustrated in this jsbin example. |
Looks like that is the case for the SafeSubscriber in RxJava: But not the case for AnonymousSafeObserver of Rx.NET: Am I reading the code correctly? |
Still opposed to this - RxJava's semantics here seem questionable to me (although I understand the practicalities). |
Reopening this issue, because I ran across the following use case. The following code prints "error" to the console: function test() {
try {
[1,2,3].forEach(x => {
throw "error";
})
}
catch(e) {
console.log(e);
}
} The way the current Observable spec is written, the following code prints nothing to the console: async function test() {
try {
await Observable.from([1,2,3]).forEach(x => {
throw "error";
});
}
catch(e) {
console.log(e);
}
} This is definitely wrong because it is a refactoring hazard. Furthermore the Promise returned by forEach becomes a Promise.never, because after next throws no further notifications are delivered to the Observer. This seems like a big footgun. If we want to mimic the behavior of sync forEach, we have two choices:
I'm pretty uncomfortable with option #2. Seems to me that forEach and subscribe should be semantically equivalent in every way. For JavaScript, option #1 seems correct to me. |
@jhusain We removed I'm not sure I understand the objection to option 2, though. If we went with this change, then wouldn't it force our hand with respect to #109? DOM wants to be able to keep sending |
You make a valid point with respect to #109. I've previously advocated for the |
@jhusain I'm concerned here after our conversation yesterday with regards to error semantics closing versus not closing based upon error in the stream. If it's unhandled meaning a |
@zenparsing I'm not clear why #109 is an issue, since Observables are referentially transparent. Subscriptions can synchronously The idea that the subscription is cleaned up on error/complete is central to Rx's guaranteed deterministic memory management. |
To be clear, the behavior I'm advocating for here is:
p.s. we will likely have a way to configure this behavior in RxJS to allow errors to propagate globally instead of being caught, so Alex Liu and the node team can extract accurate core dumps (doing anything else risks corrupting VM state that's essential to postmortem investigative analysis). |
@zenparsing I see the wisdom of the approach you've taken. I'm content to remove autoclose behavior when Observers throw, but add it in for forEach. I will add my rationale to #109. |
I was talking with @trxcllnt today about the behavior the occurs when you throw in an next handler...
For example:
Currently, it will just throw, and will not go to the error handler.
This results in people using RxJS like this as a "best practice", which isn't at all a "best practice", because
do
anddoAction
will route thrown errors down the error path:Apparently teams at Netflix even have their own forks of RxJS that have patched the the onNext handler call to route to the onError handler if it throws.
The text was updated successfully, but these errors were encountered: