Skip to content

Take Operator Error Handling #217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
benjchristensen opened this issue Mar 31, 2013 · 8 comments
Closed

Take Operator Error Handling #217

benjchristensen opened this issue Mar 31, 2013 · 8 comments

Comments

@benjchristensen
Copy link
Member

The take operator from pull request #212 and merged in #215 does not handle Observer.onNext throwing exceptions.

This unit test reveals it:

        @Test
        public void testTakeWithErrorInObserver() {
            final AtomicInteger count = new AtomicInteger();
            final AtomicReference<Exception> error = new AtomicReference<Exception>();
            Observable.from("1", "2", "three", "4").take(3).subscribe(new Observer<String>() {

                @Override
                public void onCompleted() {
                    System.out.println("completed");
                }

                @Override
                public void onError(Exception e) {
                    error.set(e);
                    System.out.println("error");
                    e.printStackTrace();
                }

                @Override
                public void onNext(String v) {
                    int num = Integer.parseInt(v);
                    System.out.println(num);
                    // doSomething(num);
                    count.incrementAndGet();
                }

            });
            assertEquals(2, count.get());
            assertNotNull(error.get());
            if (!(error.get() instanceof NumberFormatException)) {
                fail("It should be a NumberFormatException");
            }
        }

It silently fails with a count of 2 and never prints an exception or throws anything.

@benjchristensen
Copy link
Member Author

@johngmyers can you take a look at this please since you have been involved in this code?

@benjchristensen
Copy link
Member Author

See #216 for a discussion related to this.

@johngmyers
Copy link
Contributor

In the supplied unit test, the take operator has already delivered three onNext() notifications, so it has completed. It can't deliver an onError() after onComplete(). Change the test to a take(4) and it passes.

So in my opinion it's an invalid test.

Note that none of the trusted operators handle asynchronous Observer.onNext() notifications throwing exceptions. It is Observer.subscribe() that is catching the exception and delivering it through onNext() and that only works because the notifications are synchronous.

@johngmyers
Copy link
Contributor

For an amusing test, replace the take(3) with materialize().

@johngmyers
Copy link
Contributor

So in my opinion it's an invalid test.

Let me amend that statement. The test was bug-dependent: it depended on the very bug that was fixed in order to pass. Furthermore, if the test had applied take(3) to an asynchronous trusted Observer generating the same sequence, it would not have passed.

@benjchristensen
Copy link
Member Author

Operators must work on both synchronous and asynchronous sequences, including error handling, and I understand that this issue exists on all kinds of operators. This issue is to demonstrate that there are challenging implementation details and nuanced behavior even when an operator implementation "looks correct". The discussion at #216 will carry this forward ...

In the supplied unit test, the take operator has already delivered three onNext() notifications, so it has completed. It can't deliver an onError() after onComplete(). Change the test to a take(4) and it passes.

With take(3) it does not successfully deliver 3 onNext notifications. It delivers 2 and attempts the 3rd and fails and then when onError is invoked it thinks it has terminated based solely on the count but it has not because the 3rd onNext failed and should have instead emitted an onError.

Whether this gets fixed in the operator or part of #216 is an open question. I think it should be done outside of the operator because too much error-prone boilerplate will need to exist in every single operator to try and get every use case handled.

I'll hold this issue open until #216 is finalized but theoretically no code changes should be needed for the take operator directly.

@benjchristensen
Copy link
Member Author

I just tested the unit test above and it works.

1
2
error
java.lang.NumberFormatException: For input string: "three"
    at java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
    at java.lang.Integer.parseInt(Integer.java:492)
    at java.lang.Integer.parseInt(Integer.java:527)
    at rx.TestIssue$1.onNext(TestIssue.java:32)
    at rx.TestIssue$1.onNext(TestIssue.java:1)
    at rx.operators.SafeObserver.onNext(SafeObserver.java:121)
    at rx.operators.OperationTake$Take$ItemObserver.onNext(OperationTake.java:140)
    at rx.operators.OperationToObservableIterable$ToObservableIterable.onSubscribe(OperationToObservableIterable.java:55)
    at rx.Observable.subscribe(Observable.java:186)
    at rx.operators.OperationTake$Take.onSubscribe(OperationTake.java:110)
    at rx.operators.OperationTake$1.onSubscribe(OperationTake.java:59)
    at rx.Observable.subscribe(Observable.java:196)
    at rx.TestIssue.testTakeWithErrorInObserver(TestIssue.java:16)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

for

@Test
    public void testTakeWithErrorInObserver() {
        final AtomicInteger count = new AtomicInteger();
        final AtomicReference<Throwable> error = new AtomicReference<Throwable>();
        Observable.from("1", "2", "three", "4").take(3).subscribe(new Observer<String>() {

            @Override
            public void onCompleted() {
                System.out.println("completed");
            }

            @Override
            public void onError(Throwable e) {
                error.set(e);
                System.out.println("error");
                e.printStackTrace();
            }

            @Override
            public void onNext(String v) {
                int num = Integer.parseInt(v);
                System.out.println(num);
                // doSomething(num);
                count.incrementAndGet();
            }

        });
        assertEquals(2, count.get());
        assertNotNull(error.get());
        if (!(error.get() instanceof NumberFormatException)) {
            fail("It should be a NumberFormatException");
        }
    }

@benjchristensen
Copy link
Member Author

Unit test added in #353

rickbw pushed a commit to rickbw/RxJava that referenced this issue Jan 9, 2014
billyy pushed a commit to billyy/RxJava that referenced this issue Jan 13, 2014
jihoonson pushed a commit to jihoonson/RxJava that referenced this issue Mar 6, 2020
…tiveX#217)

* Feature ReactiveX#215 ignore and record exceptions

* Feature ReactiveX#215 ignore and record exceptions

* 215 Cleanup and changed test

* 215 removed super
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants