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

Activity

1. artem-zinnatullin commented on Oct 10, 2015

   

   artem-zinnatullin

   on Oct 10, 2015

   ContributorAuthor

   More actions

   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!
   }
2. artem-zinnatullin commented on Oct 10, 2015

   

   artem-zinnatullin

   on Oct 10, 2015

   ContributorAuthor

   More actions

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

   changed the title [-]OperatorFinally calls action twice if action throws exception and exception is swallowed[/-] [+]OperatorFinally calls action twice if action throws exception. Exception thrown by action is swallowed[/+] on Oct 10, 2015
4. akarnokd commented on Oct 12, 2015

   

   akarnokd

   on Oct 12, 2015

   Member

   More actions

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

   added

   Bug

   on Oct 12, 2015
6. artem-zinnatullin commented on Oct 12, 2015

   

   artem-zinnatullin

   on Oct 12, 2015

   ContributorAuthor

   More actions

   We can throw it as Error. Leaving the app in an unknown state is very bad idea, especially in something like finallyDo().
7. akarnokd commented on Oct 12, 2015

   

   akarnokd

   on Oct 12, 2015

   Member

   More actions

   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.
8. akarnokd commented on Feb 9, 2016

   

   akarnokd

   on Feb 9, 2016

   Member

   More actions

   Can this be closed?
9. artem-zinnatullin commented on Feb 9, 2016

   

   artem-zinnatullin

   on Feb 9, 2016

   ContributorAuthor

   More actions

   @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.
10. akarnokd commented on Apr 2, 2016

    

    akarnokd

    on Apr 2, 2016

    Member

    More actions

    Fix posted in #3823.
11. akarnokd commented on Apr 14, 2016

    

    akarnokd

    on Apr 14, 2016

    Member

    More actions

    Closing via #3823
12. akarnokd

    closed this as completedon Apr 14, 2016