Skip to content

2.0 Design: support current Observer? #2798

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
akarnokd opened this issue Mar 4, 2015 · 7 comments
Closed

2.0 Design: support current Observer? #2798

akarnokd opened this issue Mar 4, 2015 · 7 comments
Milestone

Comments

@akarnokd
Copy link
Member

akarnokd commented Mar 4, 2015

The Reactive-Streams/j.u.c.Flow's base observer is the Subscriber class, but many of our tests mock over rx.Observer. Changing them to Subscriber doesn't work by default because this mock doesn't request anything from upstream. There are several options to fix this:

  • Use TestSubscriber to wrap the mocked Subscriber (TS allows specifying the inital request, defaults to Long.MAX_VALUE)
  • Maybe Mockito has infrastructure for this, I haven't found it yet.
  • Keep supporting the Observer interface with a default unbounded Subscriber wrapper.
@akarnokd akarnokd added this to the 2.0 milestone Mar 4, 2015
@benjchristensen
Copy link
Member

In my experimenting I kept the Observer and make it default to Long.MAX_VALUE.

Example here: https://github.com/benjchristensen/RxJava/blob/5076bd1ef2db6ee567024d277b7f871474d88cf3/src/main/java/io/reactivex/Observable.java#L347

We need this not just for Observer but the various other subscribe overloads that take functions.

@akarnokd
Copy link
Member Author

akarnokd commented Mar 5, 2015

That makes sense, but now we can extend Subscriber and use a default method to request max and there is no need to support overloads tailored for Observer.

@benjchristensen
Copy link
Member

How has your thinking on this evolved? I don't think we need a concrete Observer or Subscriber if we change how #2780 (Resource Cleanup) works so that we don't need a SafeSubscriber that always calls unsubscribe, then we can just have interfaces.

The Subscriber interface already exists from Reactive Streams, and would be the base Publisher.subscribe(Subscriber s) behavior.

I think it might then make sense to also have an Observer that does NOT implement Subscriber, and have Observable.subscribe(Observer o) and internally it becomes a Subscriber with request(Long.MAX_VALUE) behavior. That may not work though because it requires an allocation every time (to wrap), so perhaps Observer is an abstract class that implements Subscriber with default request(Long.MAX_VALUE) behavior?

@akarnokd
Copy link
Member Author

My stance.

If one needs an interface Observer, it is quite easy to implement:

interface MyObserver<T> extends Subscriber<T> {
    default void onSubscribe(Subscription s) {
        s.request(Long.MAX_VALUE);
    }
}

The reason this isn't the one in 2.x because I've been trying to avoid default methods for the sake of RetroLambda. I'm not sure if it can handle that or not, especially inside libraries.

@benjchristensen
Copy link
Member

RetroLambda

RetroLambda won't be useful for v2 because we're not just using lambdas, but we're using JDK 8 APIs. So don't make design decisions based on that. This version won't be usable on Android until it supports JDK 8 APIs.

@artem-zinnatullin
Copy link
Contributor

Yeah, even if you won't use JDK 8 APIs but will target JDK 8 — bytecode won't be compatible with Android build tools :(

Caused by: com.android.dx.cf.iface.ParseException: bad class file magic (cafebabe) or version (0034.0000)

I've been trying to avoid default methods for the sake of RetroLambda. I'm not sure if it can handle that or not, especially inside libraries.

Retrolambda can handle default methods, but since you're going to target Java 8, I guess, you can use default methods in v2 without worries about Android :)

Though it's possible to apply Retrolambda on a jar (manually or RxJava has to ship v2 jar already retrolamded), but anyway, if you will use at least one JDK 8 API — it won't help.

@akarnokd
Copy link
Member Author

Closing this as we went with Java 6+ and the tests were adjusted to work with Observer and Subscriber mocks.

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

No branches or pull requests

3 participants