Remove tryOrOnError from subscription #1135
Comments
|
@Blesh 100% disagree, as the proposed behavior is the source of developer confusion, inconsistent with RxJava, and is the improper dual implementation of Enumerable's pull-based error model. In the real world, users handle error side-effects in the subscription's someObservable.do((event) => {
// a bunch of side effects that might throw
}).subscribe(noop, logError);This is less efficient (creates another Observable + Operator + Subscriber between the final subscription and the source) and encourages developers to move side-effects from the Subscription (where they should be isolated) into the Observable. Since Observable is the 1-1 push-based dual of Enumerable's pull-based model, it's helpful to ask what would happen if we modeled this as an Enumerable. As you can see, errors thrown consuming the output of an Enumerable do get forwarded on to the final error handler: const range = Enumerable.range(0, 10);
const enumerator = range.getEnumerator();
let error, errorHappened = false;
try {
while (enumerator.hasNext()) {
onNext(enumerator.getCurrent());
}
} catch (e) {
error = e;
errorHappened = true;
} finally {
// either call onError or onCompleted, but not both
if (errorHappened) {
onError(error);
} else {
onCompleted();
}
dispose();
}
function onNext(x) {
if (x === 5) { throw x; }
}
function onError(e) { console.error('error: ', e); }
function onCompleted() { console.log('completed without error'); }
function dispose() { console.log('terminated'); }As you can see, if an error is throw by the consumer of each value, the error is forwarded on to the error handler. |
|
/cc @zenparsing @jhusain |
|
also /cc @mattpodwysocki @headinthebox |
@trxcllnt I was thinking about the example you provided (same as above) after I discussed this with you last night, and I wonder if there might be a misinterpretation of "dual". Iterable and Observable might be duals, but how to get data out of them aren't "duals", right? I mean, for Observable, you've got |
|
Regardless of the chosen semantics, I think that the overloads of "subscribe" need to match. In other words, Previously I argued for the RxJS4 semantics, but I think I might be changing my mind, based on practical considerations. It seems like with the Rx4 behavior, you have to do a bunch of try-catch'ing in the "next" handler for certain combinators (where you forward the error to the observer's "error" handler in the catch block), but you wouldn't have to do that if we adopted @trxcllnt 's suggestion. I admit I haven't thought it all the way through though. |
|
@trxcllnt @zenparsing we thought about this long and hard for RxJS v4 and you should protect user code, but anything else should not be protected because it is at the edge of the monad. See our design guidelines on that matter cc @headinthebox |
|
@mattpodwysocki I saw this line in the guidelines:
What sorts of unexpected behavior? Are there examples? |
|
@mattpodwysocki not according to @headinthebox's comment on this issue a few months ago (which is in alignment with my argument above). |
|
@trxcllnt and @headinthebox is very much mistaken. Here's a repro on dotnetfiddle |
|
For me it's pretty simple: subscribe.error (2nd callback) is for errors from subscription, when subscribe.success (1st callback) is for "ok" events from subscription. Handling success events and handling error events is 2 different universes - if some error will happen in handling success event, it just can't be reason to call error handler, because we can't say "subscription returned error" - no, subscription returned "success", we just weren't able to handle it without errors. |
|
@mattpodwysocki whether @headinthebox is mistaken about how Rx.NET works isn't the point, the fact that he agrees with this behavior is. |
|
@trxcllnt I have a writeup from Bart de Smet which goes into much deeper discussion as to why this is a bad idea and will post later. |
|
@e-oz raises an interesting point. I changes the semantics of the error handler. Before if it was hit it meant "The producer had a problem", now it means "either the producer OR the consumer had a problem". |
|
@mattpodwysocki I see Bart's comment in my email (but not here?), and have two questions:
@Blesh yes, the point is the final source
// handle all errors from the source observable only.
// Subscriber's `error` handler is invoked if `catch` can't recover.
.catch((e) => /*{... recover or re-throw}*/)
.subscribe(
(x) => { if (x === 'foo') { throw x; }},
// handle all uncaught errors:
// 1. from the Observable
// 2. from `next` or `complete`
(e) => { console.error(e); }
( ) => { throw 'error while completing'; }
);edit: To clarify, without this behavior, engineers move side-effects into a source
.catch((e) => { /* ... recover gracefully ... */ })
.do((val) => { /*... a bunch of side-effects ... */ })
.subscribe(null, logUncaughtErrors, null) |
|
Yeah, it's a sticky problem.
Could add a |
|
@trxcllnt I don't think it makes sense to send errors thrown from the complete handler to the error handler, since according to the protocol only one of those handlers should ever be called. |
I fully agree with this one. Naive observation: why can't developers just use a try catch block inside the Observable.of('hi').subscribe(
() => {
try {
throw 'fart';
} catch (err) {
console.error('Bad stuff just happened');
}
}, (err) => {
// THIS SHOULD NEVER BE HIT.
}
);What's so fundamentally wrong about the above? |
|
@staltz There's definitely nothing wrong with it conceptually, but I guess the question is whether it's more practical to route errors thrown from the With RxJS 4 semantics, when writing operators, you find yourself wrapping the // RxJS 4 Semantics
({
forEach(fn) {
var thisArg = arguments[1];
return new Promise((resolve, reject) => {
if (typeof fn !== "function")
throw new TypeError(fn + " is not a function");
let subscription = this.subscribe({
next(value) {
try {
return fn.call(thisArg, value);
} catch (e) {
subscription.unsubscribe();
reject(e);
}
},
error: reject,
complete: resolve,
});
});
},
});
// RxJava Semantics
({
forEach(fn) {
var thisArg = arguments[1];
return new Promise((resolve, reject) => {
if (typeof fn !== "function")
throw new TypeError(fn + " is not a function");
this.subscribe({
next(value) { return fn.call(thisArg, value) },
error: reject,
complete: resolve,
});
});
},
});And there's actually still a bug with the RxJS 4 implementation, because in es-observable we don't support synchronous unsubscription (yet). |
Normally I'd agree, but if that's how it works in RxJava and @headinthebox advocates, I'm not going to deviate. @staltz the point is there should be one place to handle uncaught errors (the onError handler) because you shouldn't be forced to repeat the same code in two places (inside onNext and onError). |
Why have the same error catching code for two very different sources of error? In
|
|
@staltz yes, but that's what the source
.catch((e) => { /* ... recover gracefully ... */ })
.do((val) => { /*... a bunch of side-effects ... */ })
.subscribe(null, logAllUncaughtErrorsWhetherThrownFromMyObservableOrInsertingDOMElementsIDontCareJustLogItHappenedSoICanFigureItOutNextWeek, null) |
But that's the problem, We're not talking about the impossibility of handling errors thrown from the observer. I think you understand the errors from producer vs consumer explanation. We're talking about the (in)convenience of such limitation, and the desire to have a "capture-all" handler, no matter where it came from. IMHO I wouldn't bend the Observable contract [1] just to get a small convenience. [1] That is, both onComplete and onError could be called on the observer. |
|
I have been thinking about this recently as well when simplifying the RxMobile code base. I'm with @staltz on this. |
|
@headinthebox There are three options on the table:
Which one are you arguing for? |
Observable.of('hi').subscribe(() => {
throw 'haha!';
}, (err) => {
// THIS SHOULD NEVER BE HIT.
});That’s right: it should not hit, just like it doesn’t in Rx.NET: Observable.Return(42).Subscribe(
x => { throw new Exception("Boom!"); }, // Thrown on producing thread
ex => Console.WriteLine(ex.Message), // NEVER GETS HIT
() => { Console.WriteLine("Done!"); }
);If OnNext somehow could reach into OnError, it’d mean that an ongoing OnError message is running overlapped with the (failing) OnNext message, which violates the grammar: two marbles are sitting on top of each other rather than next to each other. Observers are purely sequential. Also, what would it mean for an OnCompleted (a terminal) message to throw? Trigger OnError as well? And what about OnError throwing? Causing a stack overflow in OnError because it keeps calling itself? Note that operators can catch a user exception in their observer (e.g. with filtering behavior) and forward the error to the downstream observer. We’re talking about two different observers here. The end of the chain has nowhere else to put the exception other than the thread it’s running on, which could be backed by a scheduler, which could have an unhandled exception handler directly or indirectly (e.g. the task pool in .NET, or the dispatcher in WPF). In effect, redirecting errors from a handler to itself (in case of event handlers) or a sibling thereof (in case of observers, any of the three “channels”), prevents making forward progress in the pipeline which could be detected – in a rather unfortunate way – by the underlying runtime in the form of a stack overflow, or go unnoticed on an event loop type scheduler as an infinite loop. Also, bubbling up the exception to the producer of the message provides valuable information to it: the consumer failed for the reason conveyed in the exception object and can stop producing messages (continuing would mean leaving a gap in the sequence and possibly get incorrect results, e.g. for an aggregate that’s skipping a value). Granted, routing down OnError would have a similar effect because of auto-dispose behavior on terminal messages, but without conveying the error condition to the producer, and with the undesirable properties elaborated on above (including violating the observer grammar which is a basic assumption we rely on everywhere). |
|
@zenparsing @headinthebox I'm opting for number 2 for the reasons @bartdesmet states above. The edge of a monad is a dangerous place to be routing onError to |
|
@zenparsing - My take is 2, which is always what it's been in Rx.NET. 1 and 3 violate the observer grammar, having overlapped notifications on the same observer. An |
I'm not sure if that's strictly true. If the
True. On the other hand, how do you deal with the ICantHandleStockGoingDownException, other than |
|
A stack trace like: is overlap. Dealing with |
|
Couldn't that just as easily be: const enumerator = Enumerable.range(0, 10)
// throws at 3 and 7
.do((x) => if (x === 3 || x === 7) { throw x; })
// recovers from 3, throws at 7
.catch((x) => (x === 3) ? Enumerable.return(x) : Enumerable.throw(x))
.getEnumerator();
// This block can't tell the difference between the Enumerator throwing at 7 and onNext throwing at 5
while (enumerator.moveNext()) {
// this will throw at 5
let value;
try {
value = enumerator.getCurrent();
} catch (e) {
onError(e);
break;
}
onNext(value);
}
onCompleted();
dispose();
function onNext(x) {
if (x === 5) { throw x; }
}
function onError(e) { console.error('unhandled error: ', e); }
function onCompleted() { console.log('completed without error'); }
function dispose() { console.log('terminated'); }I mean, |
|
@Blesh |
|
To clarify: we're only wrapping the final Subscriber's
While I agree about the direction of handler exceptions vs. data-flow, I don't understand how catching exceptions from the final Subscriber's |
|
@trxcllnt Side question: do you agree, though, that the behavior of |
|
@zenparsing heh, definitely. |
|
@trxcllnt - In an operator like In the "final subscribe", you're leaving the world of being in the comfort zone of a declarative internal DSL (consisting of Rx operators chained together using fluent syntax) that's about data flow processing and you're saying "yup, I can handle the outcome of this in imperative land with handlers" just like you do with callbacks of any kind in UI frameworks etc. From there on, we don't have a say about your error handling policies anymore; we're no longer processing data on your behalf, we're giving you the result, which happens to be through a callback mechanism. @zenparsing - You make a very good point to further solidify this argument; if the strange behavior of redirecting an error has to apply for observers, we have to wrap observers (but only at the end of the chain somehow?) to catch errors coming out of OnNext and OnCompleted and stuff them into the OnError channel. So the observable interface now has different modes of operation (used within an operator or at the end of the chain) determining wrapping logic to decide on whether or not to put in a catch, and it no longer operates on the bare observer passed in (granted, we tie a knot there ourselves in many cases to deal with auto-detach behavior, but that's a whole different topic on our historical choice of using Finally, this has really become a discussion on composition. If we bake a behavior like catching exceptions in here and there, users can't take them out without rewriting those pieces of the framework (which gets especially hard if they're in base classes you can't just swap out), and maybe we'd end up seeing alternative implementations that look the same at the surface and confuse people because they have different runtime behavior. If we omit things, people can compose the additional behavior in if they want to (and they should feel free to create an extension method to simplify this plumbing if one gets addicted to that alternate reality). Of course one can disagree with defaults e.g. for operators; I can't tell the number of times people in large scale event processing systems (like the one I'm working on in Bing) think they want very specific error behavior for every operator, not just "to catch or not to catch" but going down rabbit holes of "Wow, you can kill the whole reactive computation by null-ref'ing from a |
| Materialize and dematerialize were added precisely to allow for user specialization of errors. |
Umm... What is that? Sounds spooky. : ) |
|
@zenparsing materialize just gives you a stream of Notifications instead: Next, Next, Error, etc. dematerialize takes a stream of notifications and makes it into a regular stream. |
|
cc/ @abersnaze @stealthcode from RxJava, who are interested in this conversation. |
|
Note that materialize and dematerialize used by be called reify and deify at some point. See https://en.wikipedia.org/wiki/Reification_(computer_science) for more information on the concept. Effectively it represents method calls like static IObservable<Notification<T>> Materialize<T>(this IObservable<T> source);
static IObservable<T> Dematerialize<T>(this IObservable<Notification<T>> source);For example, an |
|
One more thought to reduce possible confusion: in the context of this discussion (errors in callbacks for the "final subscribe"), materialize has little use, because it acts like any other operator and does not care about errors occurring downstream in handlers. It only deals with errors it receives from its source upstream. Effectively, operators are always upstream from that "final" set of handlers that are discussed here. In the context of an operator producing an error, materialize plays a role to treat errors as data objects wrapped in notification objects. Note though that its use is limited because it does not enable an upstream operator to produce many OnError calls that are turned into such error record notifications. For example, if a Where operator encounters an error while evaluating the predicate for message A, it sends an OnError downstream, which may be a Materialize operator that turns it into of a notification object. However, the mere fact of the OnError being sent causes the Where to shut down, for if it wouldn't it would break the observer grammar for its consumer. It'd effectively change the grammar from OnNext* (OnError|OnCompleted)? to (OnNext|OnError)* OnCompleted?. So a subsequent message B will never be processed by the Where operator. Unless the original sequence has notifications as elements from the get-go and things like predicates passed to operators are expressed in terms of operations on the notification objects to unwrap them (still without causing exceptions, and making each operation on elements much more clunky to deal with the discriminated union that is the notification), materialize still has limited mileage. It comes in handy from time to time though, for example when serializing a sequence of observer calls by treating them as notification objects that can be serialized and deserialized and dematerialized on the receiving end to reconstitute the sequence. |
|
When onNext throws, should we still auto-unsubscribe? |
|
@headinthebox it should still use the AutoDetachObserver behavior, so yes that remains in effect |
Add test to assert that error happening in the next() handler should not propagate to the error() handler. For issue ReactiveX#1135.
|
@Blesh Does |
|
@zenparsing ... no. I missed that bit of nuance. I'll add it. |
|
Here is a small "test" that show the .NET behavior namespace RxTests
{
class MainClass
{
public static void Main (string[] args)
{
var d = (CompositeDisposable) Observable.Interval (TimeSpan.FromSeconds (1))
.Subscribe(x => {
if(x > 3) {
throw new Exception("...");
} else {
Console.WriteLine(x);
}}, e => { Console.WriteLine(e.Message); });
d.Add(Disposable.Create(()=>{ Console.WriteLine("?????");}));
Console.ReadKey ();
}
}
}Output uncaught exception and 0
1
2
3
????? |
|
@headinthebox correct, it should conform to the |
Add test to assert that error happening in the next() handler should not propagate to the error() handler. For issue ReactiveX#1135.
Add test to assert that error happening in the next() handler should not propagate to the error() handler. For issue #1135.
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Somehow we missed this, but in order to comply with the es-observable spec, and in order not to confuse RxJS 4 users more than we already have, errors thrown in the next handler shouldn't jump to the error handler:
It should just throw.
The text was updated successfully, but these errors were encountered: