Why isn't Observable.Finally being called?

Question

I'm not understanding the Finally method. It doesn't fire in this situation.

[TestMethod]
public void FinallyHappensOnError()
{
    bool finallyActionHappened = false;
    try
    {
        Observable
        .Throw<Unit>(new DivideByZeroException())
        .Finally(() => finallyActionHappened = true)
        .Subscribe();
    }
    catch
    {
    }
    Assert.IsTrue(finallyActionHappened);
}

This one works using Do rather than Finally. I don't understand why it works with Do but not Finally.

[TestMethod]
public void CanRecordWhenSequenceFinishes()
{
    bool sequenceFinished = false;
    try
    {
        Observable.Throw<Unit>(new DivideByZeroException())
        .Do(
            onError: ex => { sequenceFinished = true; },
            onCompleted: () => sequenceFinished = true,
            onNext: _ => { })
        .Subscribe();
    }
    catch
    {

    }
    Assert.IsTrue(sequenceFinished);
}

score 2 · Answer 1 · answered Nov 10 '17 at 23:02

2

Your code (both ways) is a race condition. The race condition resolves the right way with .Do and the wrong way with .Finally. Why is less relevant than how to avoid it:

public async Task FinallyHappensOnError()
{
    bool finallyActionHappened = false;
    try
    {
        await Observable.Throw<Unit>(new DivideByZeroException())
            .Finally(() => finallyActionHappened = true);
    }
    catch
    {
    }
    Assert.IsTrue(finallyActionHappened);

}

or, if you don't want to use TPL/async/await:

[TestMethod]
public void FinallyHappensOnError()
{
    bool finallyActionHappened = false;
    try
    {
        Observable
        .Throw<Unit>(new DivideByZeroException())
        .Finally(() => finallyActionHappened = true)
        .Subscribe(
            _ => {},
            () => Assert.IsTrue(finallyActionHappened)
        );
    }
    catch
    {
    }

}

answered Nov 10 '17 at 23:02

Shlomo

14,102
3
28
43

I don't totally understand. Race between what and what? IS the test finishing before the Finally happens? I thought this would all be synchronous since there are no delays, thread schedulers, etc. – JustinM Nov 11 '17 at 05:34
Actually that Assert in the second piece of code isn't even being hit. I put a breakpoint on it and it isn't happening. – JustinM Nov 11 '17 at 05:47
Oh man am I confused. My original test code above fails. But if the second line of my test is `Subject obs = new Subject();` and then I change it to `Subscribe(obs)` then not only does the test pass, but the test never enter the Catch block; I put a breakpoint there and it isn't getting hit. – JustinM Nov 11 '17 at 05:59
1

I read up on the Finally statement. Lee Campbell says there is an anomaly in it - need an OnError action in the subscribe or else Finally doesn't run. If I put stubs into the Subscribe in my original test method then the test succeeds. See http://www.introtorx.com/Content/v1.0.10621.0/11_AdvancedErrorHandling.html and more background https://social.msdn.microsoft.com/Forums/en-US/bfafc584-fd9d-45b0-8a53-354b53813a1a/observablefinally-not-executing?forum=rx – JustinM Nov 11 '17 at 06:08

score 0 · Answer 2 · answered Dec 30 '20 at 18:54

The reason that the Finally action is not invoked in your example is because the observable sequence is subscribed by invoking the "naked" .Subscribe() overload that doesn't include the onError handler. When the onError handler is missing and the sequence fails, the exception is thrown synchronously on whatever thread encountered the exception. In your case you were "lucky" and the exception was thrown on the current thread, so you were able to catch it with the catch block. Otherwise the exception would be unhandled and would crash the process. You can test this condition by modifying your example like this:

Observable
    .Throw<Unit>(new DivideByZeroException(), ThreadPoolScheduler.Instance)
    .Finally(() => finallyActionHappened = true)
    .Subscribe();

Thread.Sleep(500); // Give some time to the unhandled exception to emerge

The moral story is that you should avoid subscribing to sequences without providing an onError handler, unless you are OK with bad things happening, like the Finally actions not invoked or the process get crashed. To "fix" your test you just need to provide an onError handler, like this:

[TestMethod]
public void FinallyHappensOnError()
{
    bool finallyActionHappened = false;
    Observable
        .Throw<Unit>(new DivideByZeroException())
        .Finally(() => finallyActionHappened = true)
        .Subscribe(value => { }, error => { }, () => { });
    Assert.IsTrue(finallyActionHappened);
}

Notice how the empty catch block is no longer needed. The swallowing of the exception is now performed by the empty onError handler.

Btw the assertion of this test succeeds only because the observable sequence completes synchronously during the subscription. In the general case of observable sequences having a longer life the assertion will fail, because the Assert.IsTrue will be called before the completion of the sequence. For this reason I would suggest to wait for the completion of the observable sequence, synchronously or asynchronously, before checking the assertion. Here is an example of a synchronous waiting, which also implies a full subscription with all three handlers attached.

Observable
    .Throw<Unit>(new DivideByZeroException())
    .Finally(() => finallyActionHappened = true)
    .DefaultIfEmpty().Wait();

The exception will be rethrown synchronously by the Wait operator, so you may want to try/catch it as in your original test.

Why isn't Observable.Finally being called?

2 Answers2

Linked