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

Force ViewObservable be subscribed and unsubscribed in the UI thread #880

Merged

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Feb 15, 2014

According to #869 , unsubscribe can run in any thread. However, that will cause some concurrent issues in rxjava-android.

This PR schedules the unsubscribe action to run in the UI thread to solve the problem.

@cloudbees-pull-request-builder

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

@cloudbees-pull-request-builder

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

@benjchristensen
Copy link
Member

Seems like a good change. Do you agree with the assessment in #869 that we can't guarantee a Subscription is called from the correct thread?

benjchristensen added a commit that referenced this pull request Feb 15, 2014
Force ViewObservable be subscribed and unsubscribed in the UI thread
@benjchristensen benjchristensen merged commit cec2bfd into ReactiveX:master Feb 15, 2014
@zsxwing
Copy link
Member Author

zsxwing commented Feb 16, 2014

Yes, I agree with it. Since we can not give the guarantee, it's better to require users write a thread-safe Subscription.

@benjchristensen
Copy link
Member

Cool, then yes, I also agree that Subscription implementations need to be thread-safe since they don't have the same guarantees as onNext, onError and onCompleted.

benjchristensen added a commit to benjchristensen/RxJava that referenced this pull request Feb 17, 2014
Working with @headinthebox based on discussions at ReactiveX#869 and ReactiveX#880 (comment) we determined that there are times when `unsubscribeOn` behavior is needed.

The `subscribeOn` operator can not mix `subscribe` and `unsubscribe` scheduling behavior without breaking the `lift`/`Subscriber` behavior that allows unsubscribing synchronous sources. The newly added `unsubscribeOn` operator will not work with synchronous unsubscribes, but it will work for the targeted use cases such as UI event handlers.
benjchristensen added a commit to benjchristensen/RxJava that referenced this pull request Feb 17, 2014
Working with @headinthebox based on discussions at ReactiveX#869 and ReactiveX#880 (comment) we determined that there are times when `unsubscribeOn` behavior is needed.

The `subscribeOn` operator can not mix `subscribe` and `unsubscribe` scheduling behavior without breaking the `lift`/`Subscriber` behavior that allows unsubscribing synchronous sources. The newly added `unsubscribeOn` operator will not work with synchronous unsubscribes, but it will work for the targeted use cases such as UI event handlers.
@benjchristensen
Copy link
Member

@zsxwing Take a look at #890 to see if it would change anything you did in this pull request.

@mttkay
Copy link
Contributor

mttkay commented Feb 18, 2014

I was wondering, can we simply make the component refs volatile instead? I mean, you control the subscription, so scheduling on the UI thread just to maintain correctness w.r.t. visibility seems wasteful.

@@ -39,6 +43,7 @@ public OperatorCompoundButtonInput(final CompoundButton button, final boolean em

@Override
public void call(final Subscriber<? super Boolean> observer) {
ViewObservable.assertUiThread();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is ViewObservable? Did that land in another PR?

I would prefer to not have this call here. What we want to do instead is, if at all, check this in the fromFragment / fromActivity helpers (see discussion in #754 which asks to remove this assertion entirely)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make sure that subscribe happens in the UI thread or there would be some concurrent issues. So I add assertUiThread here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we just add it in the helpers, users still can subscribe in other threads after they get the Observable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ViewObservable is from #783

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "some concurrent issues"? We've had no issues so far

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that create an Observable in the UI thread and subscribe it on other thread. For example, Schedulers.newThread()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g., ViewObservable.clicks(..).subscribeOn(Schedulers.newThread()).subscribe(...)

@zsxwing
Copy link
Member Author

zsxwing commented Feb 18, 2014

can we simply make the component refs volatile instead?

Yes, I think this is better, and we can remove the assertUiThread for OperationObserveFromAndroidComponent.

@zsxwing
Copy link
Member Author

zsxwing commented Feb 18, 2014

@mttkay , I feel uncomfortable that the Observables from OperationObserveFromAndroidComponent can not be subscribed more than once. Any thoughts?

@mttkay
Copy link
Contributor

mttkay commented Feb 18, 2014

I had a local branch where I did all the changes, but I had to drop it like 2 times due to all the things that happened on master to the core APIs, that's why I suggested to hold off with changes to this. Looking at master, it seems things have stabilized and the release will go out soon.

My suggestion again is:

  • declare all refs that need to be cleared volatile; we merely need to ensure visibility when unsubscribing
  • remove the UI thread assertion from the operator, and either leave it out altogether or I guess make it a static call in AndroidObservable.fromFragment and AndroidObservable.fromActivity so that we fail instantly if we're attempting to bind a UI object from a background thread

Does that make sense?

@mttkay
Copy link
Contributor

mttkay commented Feb 18, 2014

w.r.t. subscribing more than once: what would you expect instead? I haven't thought about this to be honest

@zsxwing
Copy link
Member Author

zsxwing commented Feb 18, 2014

e.g.,

Observable o = AndroidObservable.fromFragment(...)
s = o.subscribe(...)
s.unsubscribe()

s1 = o.subscribe(..) // not work

@mttkay
Copy link
Contributor

mttkay commented Feb 18, 2014

what doesn't work exactly?

@zsxwing
Copy link
Member Author

zsxwing commented Feb 18, 2014

remove the UI thread assertion from the operator, and either leave it out altogether or I guess make it a static call in AndroidObservable.fromFragment and AndroidObservable.fromActivity so that we fail instantly if we're attempting to bind a UI object from a background thread

So the following will fail if it runs in a background thread?

ViewObservable.clicks(...).subscribeOn(AndroidSchedulers.mainThread()).subscribe(...)

@zsxwing
Copy link
Member Author

zsxwing commented Feb 18, 2014

Observable o = AndroidObservable.fromFragment(...)
s = o.subscribe(...)
s.unsubscribe() // componentRef will be set to null

s1 = o.subscribe(..) // not work as componentRef is null

@zsxwing
Copy link
Member Author

zsxwing commented Feb 18, 2014

Another case is:

Observable o = AndroidObservable.fromFragment(...)
s = o.subscribe(observer1)
s1 = o.subscribe(observer2) // observer1 will not receive any further message

@mttkay
Copy link
Contributor

mttkay commented Feb 18, 2014

I see what you mean; because componentRef is bound at time of construction.

I agree in general it should behave in the same way as the underlying sequence; so if the underlying sequence is re-subscribable (not all are, depending on the subscription I guess), we should probably reflect that.

I'm not sure how you would solve that though. Any ideas?

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

Successfully merging this pull request may close these issues.

None yet

4 participants