CODEWITHSUNDEEP
c#system.reactive
JustinM
JustinM

Reputation: 1013

Why isn't Observable.Finally being called?

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);
}

Upvotes: 2

Views: 559

Answers (2)

Theodor Zoulias
Theodor Zoulias

Reputation: 43419

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.

Upvotes: 0

Shlomo
Shlomo

Reputation: 14350

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
    {
    }

}

Upvotes: 2

Related Questions