Raising an exception in `Observable.choose` fails to be caught about fsharp.control.reactive

[mrakgr]

Just like for the switchMap issue, the F# library function most likely isn't dealing with OnComplete (in that case) and OnError (in this case) properly. Back when I opened these issues I had just started learning reactive combinators so it did not feel right for me try and poke around the library, but now I'll finish the Rx GUI examples that I've been doing and if nobody has gotten around to it yet I'll deal with this and the switchMap issue myself.

I'll admit that in this particular issue having to catch exceptions in choose is not natural, as you would not expect throw exception in it, but it still it does most likely go against the library design guidelines. Actually there is a paper on Rx specs, but I haven't properly studied it yet. I want to do that as well before trying to make a fix.

[panesofglass]

Can you tell what might be happening?

[mrakgr]

Ok, now that I am done with what I talked about let me get to this. As it turns out, the fix is really easy. All one needs to do is to change this...

    let choose f source =
        Observable.Create (fun (o : IObserver<_>) ->
            subscribeSafeWithCallbacks 
                (f >> Option.iter o.OnNext)
                o.OnError
                o.OnCompleted
                source)

...to this.

let choose f source =
    Observable.Create (fun (o : IObserver<_>) ->
        subscribeSafeWithCallbacks
            (fun x -> try f x |> Option.iter o.OnNext with ex -> o.OnError ex)
            o.OnError
            o.OnCompleted
            source)

I've tried it by hand and it works. Now, the next problem is how to write a test for this. Before I can even start that I need to figure out how to build the project. Just doing the build in VS IDE is not working for me.

[mrakgr]

I figured out how to build it. There is the build.ps1 script which is runnable from Powershell. I needed to do that first.

[mrakgr]

    [<Test>]
    member __. ``throwing an exception in choose lead to the OnError event firing`` () =
        let choose f source =
            Observable.Create (fun (o : IObserver<_>) ->
                subscribeWithCallbacks
                    (f >> Option.iter o.OnNext) // Incorrect
                    //(fun x -> try f x |> Option.iter o.OnNext with ex -> o.OnError ex) // Correct
                    o.OnError
                    o.OnCompleted
                    source)
        Observable.ofSeq [1;2;3] |> choose (fun _ -> failwith "qwe") |> ``should be`` 0 true false

I am having some trouble writing the tests. This actually succeeds, but it should not as the exception will flow out.

Here is how I am testing it on my own side.

open System
open System.Reactive.Linq
open FSharp.Control.Reactive
open FSharp.Control.Reactive.Observable

let choose f source =
    Observable.Create (fun (o : IObserver<_>) ->
        subscribeSafeWithCallbacks
            (f >> Option.iter o.OnNext)
            o.OnError
            o.OnCompleted
            source)

[<EntryPoint>]
let main argv =
    use __ = 
        Observable.ofSeq [1;2;3]
        |> choose (fun x -> failwith "qwe")
        |> Observable.subscribe
            (printfn "%A")
            //(printfn "%A")
    0

If you switch...

        |> Observable.subscribe
            (printfn "%A")
            //(printfn "%A")

With...

        |> Observable.subscribeWithError
            (printfn "%A")
            (printfn "%A")

Even with the current incorrect implementation, the onError here will get fired rather than flow out. I am guessing this is why the error happens in the test. What should I do here then?

[mrakgr]

let ``should be`` expectedNext expectedError expectedCompleted (observable:'a IObservable) =
    let next = ref 0
    let error = ref false
    let completed = ref false

    let subscription = observable |> Observable.subscribeWithCallbacks (fun _ -> incr next) (fun _ -> error := true) (fun () -> completed := true)

    Assert.That(!next, Is.EqualTo expectedNext)
    Assert.That(!error, Is.EqualTo expectedError)
    Assert.That(!completed, Is.EqualTo expectedCompleted)

Since should be has a fairly simple implementation, I can pick it apart a little and do the test like so...

    [<Test>]
    member __. ``throwing an exception in choose leads to the OnError event firing and does not lead to the exception flowing out even with a regular subscribe`` () =
        let o = Observable.ofSeq [1;2;3] |> choose (fun _ -> failwith "qwe")
        let error_flows_out = ref false
        try o |> Observable.subscribe (printfn "%i") |> ignore
        with _ -> error_flows_out := true
        Assert.That(!error_flows_out, Is.EqualTo false)
        o |> ``should be`` 0 true false

This should be it.

The only thing I am wondering now is why there are two versions of the tests, one for XUnit and one for NUnit. Am I expected to duplicate them here? It would be better to just erase the ones that are not being used.

[panesofglass]

Great question about tests. The migration to Xunit was not completed. I'd love help getting those cleaned up. Or just go back to NUnit. Either is fine with me.