A number of improvements to OperatorObserveFromAndroidComponent

[mttkay]

Could I get some eyes on this? @tehmou @zsxwing @benjchristensen

• move the UI thread assert out of the operator and into the helpers; this way, we don't fail the observer anymore with an exception, but the caller.
• do not loop unsubscribe through the main thread anymore. This unnecessarily defers releasing the references, and might in fact be processed only after Android creates the component after a rotation change. I had to make the references volatile for this to work.
• immediately unsubscribe in case we detect the componentRef has become invalid. This solves the problem that dangling observers would continue to listen to notifications with no observer alive anymore.

refs:
#754
#899

[cloudbees-pull-request-builder]

RxJava-pull-requests #871 SUCCESS
This pull request looks good

[zsxwing]

Looks good

[benjchristensen]

Is this ready to merge?

[mttkay]

I would like to try and see if we can even get rid of the component binding
and make this purely subscriber based.

Moreover, I still need to find out why we even appear to need a weak
binding whenever cache or replay are involved. A filed a separate issue for
that, I have a hunch that ReplaySubject leaks observers in some cases even
after unsubscribing.

Meanwhile, did anyone else give this a whirl?

Ben can we hold on with landing this until next week? I think I won't get
to this before the weekend
On Mar 6, 2014 7:02 AM, "Ben Christensen" notifications@github.com wrote:

Is this ready to merge?

Reply to this email directly or view it on GitHubhttps://github.com//pull/931#issuecomment-36827668
.

[mttkay]

@samueltardieu you mentioned you're using AndroidObservable a lot in one of your projects. Would appreciate your feedback on this PR and the discussions linked above, since this PR deprecates OperatorObserveFromAndroidComponent due to the many issues it has and the small number of problems it actually solves

A quick recap of those discussions:

• fromActivity currently forces you to unsubscribe in onDestroy which can be a source of bugs (context leaks)
• fromFragment is not really useful for retained fragments since you don't really need to handle references to a retained fragment; it's also not really useful for non-retained fragments (AFAICT) unless you keep a strong reference to the attached activity in the subscriber
• the test for isAdded (for fragments) isn't actually required in those cases where you unsubscribe in onDestroyView, and turned out to not be flexible/extensible enough for some users (see one of the two linked issues)
• both fromFragment/fromActivity return observables that cannot be subscribed to more than once, which is a pretty big issue

Man of these limitations are a direct result of me trying to find a one-size-fits-all solution for observing long running sequences from activities and fragments, but that turned out to be difficult to achieve. In summary this leads me to believe that

1. we should nail down what the different use cases for subscriptions in Android UI components are and address them separately
2. start by solving the most pressing issue, which is making it hard or impossible to accidentally leaking context refs through observers that are bound to a fragment or activity

This addresses 2. OperatorWeakBinding transforms a sequence by binding the subscriber weakly, and stops emitting items and unsubscribes from the sequence whenever it notices that the subscriber has been released.

This turned out to work very well with Activities: no need to unsubscribe anymore, and subscribing an activity or an inner observer through this operator makes sure we don't leak the activity.

For retained fragments, a weak subscriber binding seems to only be required when using cache or replay on the source sequence, and I haven't quite figured out yet why that is.

I also need to test a few edge cases with the many ways fragments can be used (for instance, in a fragment ViewPager, the fragment life cycle is a little peculiar.)

[samueltardieu]

We have 8 occurrences of AndroidObservable.fromActivity(), and one of AndroidObservable.fromFragment() (used on a non-retained fragment).

Long-running (or infinite) or slow-running observables are subscribed to in onResume() and explicitly unsubscribed in onPause, so we should have no leak there.

Short-running and finite observables are not explicitly unsubscribed, and we could on fromActivity() for doing the right thing, that is not calling onNext()
because it may use UI components that are no longer displayed. I am conscious that the context is leaked (as well as UI components through the
subscriber), but in our case there is never a long delay between two events, so the references are freed soon enough not to be a problem.

As far as the fragment is concerned, the subscription is created in onCreateView() and unsubscribed from in onDestroy() (will change this for onDestroyView(), but since
the fragment is not retained here this is not a concern), so using AndroidObservable.fromFragment() is more a precaution here than anything else. As in some activities,
the use of AndroidObservable.fromXXX is more of a convenience in order to avoid writing .observeOn(AndroidSchedulers.mainThread()).

I had not realized that those observables could not be subscribed more than once. While this is not a concern today, it might certainly have bitten me in the future, I agree
this is problematic.

I have tried running the cgeo tests with the content of this PR, and I could not notice anything wrong.

However, I am puzzled when you talk about OperatorWeakBinding (which is not in this PR, right?). Does that mean that a reference to the subscriber has to be kept
somewhere in the activity so that it does not get garbage collected prematurely?

[mttkay]

Thanks for your input. Sorry for the confusion -- I meant to reference this pull request: #938

It would obsolete any changes in here, but for the time being they're still valuable.

[mttkay]

@benjchristensen

this is ready to merge then. Let's assume that this operator will stick around until we've found a satisfying solution, whether that be #938 or something else.

[benjchristensen]

Merging then. Thanks.