OperatorFinally calls action twice if action throws exception. Exception thrown by action is swallowed

[artem-zinnatullin]

While I was implementing #3434 I've found two problems with OperatorFinally.

I'll call OperatorFinally.action as finallyAction for better readability.

1. If finallyAction is null — NullPointerException will be swallowed by SafeSubscriber, this can be solved via action != null check in the OperatorFinally (I'll make PR).
2. If finallyAction throws exception, lift calls onError() and OperatorFinally invokes finallyAction again (this may brake user-defined logic)! And second exception is swallowed by the SafeSubscriber (see problem 1).

I vote for solving both of these problems before 1.0.15/1.1.10.

[artem-zinnatullin]

Here are the tests:

@Test
public void nullFinallyActionShouldBeCheckedASAP() {
    try {
        Observable
                .just("value")
                .finallyDo(null);

        fail();
    } catch (NullPointerException expected) {

    }
}

@Test
public void ifFinallyActionThrowsExceptionShouldNotBeSwallowedAndActionShouldBeCalledOnce() {
    Action0 finallyAction = mock(Action0.class);
    doThrow(new IllegalStateException()).when(finallyAction).call();

    TestSubscriber<String> testSubscriber = new TestSubscriber<String>();

    Observable
            .just("value")
            .finallyDo(finallyAction)
            .subscribe(testSubscriber);

    testSubscriber.assertValue("value");

    verify(finallyAction).call();
    // Actual result:
    // Not only IllegalStateException was swallowed
    // But finallyAction was called twice!
}

[artem-zinnatullin]

I'll try to send PRs with fixes ASAP, null check is easy, but second problem looks tricky.

[akarnokd]

If the action throws, you can't do much but call RxJavaPlugins.getInstance().getErrorHandler().handleError() for it.

[artem-zinnatullin]

We can throw it as Error. Leaving the app in an unknown state is very bad idea, especially in something like finallyDo().

[akarnokd]

The sequence has already terminated at that point and this operator is there to have side-effects after it. The exception has nowhere to go and there is no guarantee a thrown error reaches its intended target because of a potential async boundary. So it either propagates up to a synchronous caller, gets thrown away by the ExecutorService or gets sent to the error handler.

[akarnokd]

Can this be closed?

[artem-zinnatullin]

@akarnokd won't be able to work on this for at least 2 days, feel free to run the tests I posted ^ and see what we can do here or I'll investigate this in the end of the week.

As far as I remember, the biggest problem is that action gets called twice if it throws exception and it may break app's logic in a dramatic manner.

[akarnokd]

Fix posted in #3823.

[akarnokd]

Closing via #3823