Review of RxJava 2 usage in the project

[akarnokd]

Review of RxJava 2 usage in the project

[ZacSweers]

Thank you so much David! I'll get on these today. Would you mind if I added you as a reviewer on the subsequent PR(s)?

[akarnokd]

Sure.

[ZacSweers]

Working on a branch and had a few questions/comments.

The DisposableMaybeObserver could be created and setOnce'd before the call to lifecycle.subscribe()

Is this due to using subscribeWith()? If so, would the safer solution be to implement the full observer + disposable setting manually in onSubscribe()?

AutoDisposePlugins.java#L36: Such lockdown support may not be desirable on Android. The same feature was aimed at server/J2EE container use of RxJava.

True, but this was built to be a Java library first with android extensions :). Mostly added to keep the semantics familiar to RxJava.

dependencies.gradle#L65: Using a pretty old RxJava 2 version.

Good point. I tend to prefer making extension libraries depend on lower versions of libraries to avoid imposing burden on consumers that maybe haven't been able to update yet (we hit this often). I suppose it could also become a liability if new changes caused unknown behavior changes here. I'll think on this more.

AutoDisposingCompletableObserverImpl.java#L58: May not be a good idea to leave the delegate non-terminated in this case, if the lifecycle actually can get onComplete.

I should elaborate that comment in source a bit. The behavior is such that if the scope "completes", then the binding is considered "unbound" and free to execute to termination. The idea is that scopes might sometimes be required for an API, but consumers of that API may want to just indicate that they can be unbound. They may dynamically resolve to be unbound under certain conditions at runtime too, and I wanted to support making that explicit. Otherwise scopes could just be defined as Single. Does that get to what you were raising above? If so - what do you think of that approach?

[akarnokd]

Is this due to using subscribeWith()? If so, would the safer solution be to implement the full observer + disposable setting manually in onSubscribe()?

The problem is that this subscription happens before the associates setOnce even runs, thus the once-ness is enforced a bit late. I'd create the lifecycle observer type first, call setOnce and then lifecycle.subscribe() after.

If so - what do you think of that approach?

Risky. It would leave the consumer that is interested in handling onComplete without events, which could leak resources. This was a problem in RxLifecycle if I remember correctly.

[ZacSweers]

Could you expand on this bit some? I'm not sure I follow

consumer that is interested in handling onComplete without events

Particularly, the terminal emissions of the scope emissions are not visible outside of the autodisposing observer implementations, so I'm not sure which consumer would be interested in handling it. If the lifecycle onCompletes, the delegate is basically free to execute unbounded, and will never be manually disposed by AutoDispose.

[akarnokd]

I'm sorry I have to postpone further explanations till next week as I have an important submission deadline coming up.

[ZacSweers]

No problem at all!

[ZacSweers]

Done in #138