FirstAsync throws on error, regardless of Catch<..., ...>(...) in the chain

[Ifhay]

Hello, it appears that the exceptions that are raised inside of the R3 chains are not handled correctly if the terminal operator is FirstAsync (also SingleAsync and possibly others).

The minimal reproduction code snippet is:

var task = Observable.ReturnUnit()
            .Select<Unit, Unit>(_ => throw new Exception("Test"))
            .Catch<Unit, Exception>(_ => Observable.ReturnUnit())
            .FirstAsync();

task.Wait();
Console.Write(task.IsFaulted ? "The task failed" : "The task succeeded");

It will throw an uncaught exception on the .FirstAsync(); line.

I believe that the intended behaviour is to:

1. catch any exceptions above Catch
2. if there is no Catch, then wrap the exceptions in the Task and make it faulty.

Thanks for the awesome library, btw

[neuecc]

Thank you.
Catch transforms Result's Error, while OnErrorResume just passes it through.
Operators that convert to Task like FirstAsync convert OnErrorResume into errors, which leads to this behavior.
While you can use IgnoreOnErrorResume to ignore it, in this case that would result in "Sequence contains no elements."
It might be difficult to satisfy all requirements with existing operators.
How about adding an operator like this?
CatchAll<T, TException>(this Observable<T> source, Func<TException, Observable<T>> errorHandler)

[neuecc]

Since using CatchAll makes the subsequent behavior constraints unclear when using OnErrorResume, I think it might be better to handle this as an idiom where we add OnErrorResumeAsFailure before Catch, like this:

var task = Observable.ReturnUnit()
    .Select<Unit, Unit>(_ => throw new Exception("Test"))
    .OnErrorResumeAsFailure()
    .Catch<Unit, Exception>(_ => Observable.ReturnUnit())
    .FirstAsync();

[Ifhay]

I think it might be better to handle this as an idiom where we add OnErrorResumeAsFailure before Catch

Yeah, that should work, thank you!

However, if at all possible, would it be acceptable to add overloads for all ...Async() methods (seems to be quite a few, unfortunately) or a new argument with default value that would change the behaviour of error handling?
Something along the lines of bool wrapErrorsIntoTask that would be false by default

[neuecc]

The overload suggestion is good, but what behavior do you want?
Currently, we convert OnErrorResume to Task errors, but would you prefer behavior that doesn't do this conversion?
Since this would be equivalent to IgnoreOnErrorResume, considering the complexity of arguments (adding an Action for handling ignored errors), it might be better to have users call IgnoreOnErrorResume directly.

[Ifhay]

I cannot say what decision would be the best architecture-wise.

There is a possibility that this is a non-problem altogether and it's just me, not quite having an intuition about Task-related aspects. 🙂

I think having to use IgnoreOnErrorResume is a good solution, albeit a not intuitive one (subjectively).

At this time I would consider this issue "resolved" if you simply added a doc string that mentions that ...Async() methods may produce uncaught exceptions and that the intended way of avoiding this is to use IgnoreOnErrorResume in the chain.

[neuecc]

Thank you.
While I believe we've created consistent behavior around Task, the documentation is lacking.
I'd like to close this by adding detailed explanations about the specifications in both the document comments and the ReadMe.