Skip to content
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

Synchronous adapter methods #172

Closed
johngmyers opened this issue Mar 5, 2013 · 7 comments
Closed

Synchronous adapter methods #172

johngmyers opened this issue Mar 5, 2013 · 7 comments

Comments

@johngmyers
Copy link
Contributor

It would be useful for Observable<T> to provide List<T> get() throws Exception and T getSingle() throws Exception methods for providing synchronous interfaces on top of Observable.

@benjchristensen
Copy link
Member

Many of these have recently been implemented or are being implemented.

You can see the recent releases at https://github.com/Netflix/RxJava/blob/master/CHANGES.md

@johngmyers
Copy link
Contributor Author

Silly me for expecting these to be documented in the wiki.

I don't like the way that toIterable() and friends wrap use Exceptions.propagate() on errors. They should either throw Exception or wrap in a subclass of RuntimeException that callers can use in a catch block just for Observer-thrown errors.

(And I think that Observable<T> should have been Observable<T, E extends Exception>)

@benjchristensen
Copy link
Member

No one has gotten around to updating the wiki with the docs ... these functions were just recently submitted as contributions.

The try/catch block is what is used on any of the blocking operators (forEach, toIterable, etc).

For example, see this unit test: https://github.com/Netflix/RxJava/blob/master/language-adaptors/rxjava-groovy/src/test/groovy/rx/lang/groovy/ObservableTests.groovy#L227

What don't you like about how toIterable throws exceptions? It is a blocking structure so will thrown an exception when iterable.next() is invoked and next from the Observable sequence was an onError instead of an onNext.

It can't throw an exception only on the toIterable() method itself since the data is coming asynchronously behind the scenes and we may not know about an error until n elements into the iteration at which point an exception is thrown.

As for Observable<T> being Observable<T, E extends Exception> I don't believe that could work to strongly type E on each Observable since composition results in multiple sequence all combining into a single sequence, and each of those sequence could emit a different type of Exception at any time thus the common E of combined sequences would end up being Exception or RuntimeException at best anyways.

(Note that many of these design decisions are modeled after Rx.Net which has gone through a long design cycle that we're leveraging for many of these API designs, and they do Observable<T> - see https://rx.codeplex.com for more information on the original)

Also, there is nothing that guarantees only a single Exception type will be thrown from even a single Observable sequence - for example, any variety of RuntimeExceptions can still be passed via onError, not just the strongly typed E if that was declared.

Error handlers such as public Observable<T> onErrorResumeNext(final Func1<Exception, Observable<T>> resumeFunction) allow you to use the type of the Exception to perform different logic if you desire.

A better way of doing that will exist once the Catch operator (#28) is implemented which will allow defining a catch for each Exception type and continuing with a different Observable (similar to currently implemented onErrorResumeNext but with a catch on a specific Exception instead on all of them).

@johngmyers
Copy link
Contributor Author

What I don't like about how toIterable wraps exceptions is that it wraps them in a plain RuntimeException, so the caller cannot in the condition of a catch block distinguish exceptions returned by the Observable from exceptions thrown by the RxJava framework itself. Furthermore, one cannot distinguish in the condition of a catch block amongst different types of exceptions returned by the Observable.

@benjchristensen
Copy link
Member

What are you suggesting as the alternative?

The java.util.Iterator interface doesn't throw checked Exceptions, so if onError receives an Exception instead of RuntimeException we still have to throw it somehow since we're now in a synchronous Iterator code flow.

Thus, if we receive a RuntimeException, we throw it as is, if we receive an Exception (which I would argue just shouldn't be used as checked exceptions have exactly this problem) we have to wrap it in a RuntimeException so we can throw it.

    public static RuntimeException propagate(Throwable t) {
        if (t instanceof RuntimeException) {
            throw (RuntimeException) t;
        } else {
            throw new RuntimeException(t);
        }
    }

I do not know how to handle the checked exceptions differently but if you can offer an alternative please provide a diff or pull request that demonstrates how you would implement the following code such that it can receive either Exception or RuntimeException and conform to the Iterable/Iterator interfaces:

Here is an example unit test that receives an Exception when calling next():

    @Test(expected = TestException.class)
    public void testToIteratorWithException() {
        Observable<String> obs = Observable.create(new Func1<Observer<String>, Subscription>() {

            @Override
            public Subscription call(Observer<String> observer) {
                observer.onNext("one");
                observer.onError(new TestException());
                return Subscriptions.empty();
            }
        });

        Iterator<String> it = toIterator(obs);

        assertEquals(true, it.hasNext());
        assertEquals("one", it.next());

        assertEquals(true, it.hasNext());
        it.next();
    }

This code is from: https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/operators/OperatorToIterator.java#L113

@johngmyers
Copy link
Contributor Author

Per pull #185, my proposed wrapper type is a subclass of RuntimeException, so it can be thrown by an Iterator.

@benjchristensen
Copy link
Member

Closing this out as per the long discussion in #185

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