Skip to content

Android HandlerThreadScheduler fix (closes #1241) #1242

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.

Sign up for GitHub

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
wants to merge 2 commits into from
Closed

Android HandlerThreadScheduler fix (closes #1241) #1242

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8404c2
fix for events not being delivered on handler threads in android
vinc3m1 May 21, 2014
cf9c2b5
don't post runnable if unsubscribed
vinc3m1 May 23, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions ...va-contrib/rxjava-android/src/main/java/rx/android/schedulers/HandlerThreadScheduler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,24 +68,22 @@ public boolean isUnsubscribed() {

@Override
public Subscription schedule(final Action0 action, long delayTime, TimeUnit unit) {
if (innerSubscription.isUnsubscribed()) {
// don't schedule, we are unsubscribed
return Subscriptions.empty();
}
final Runnable runnable = new Runnable() {
@Override
public void run() {
if (isUnsubscribed()) {
return;
}
action.call();
}
};
handler.postDelayed(runnable, unit.toMillis(delayTime));
return Subscriptions.create(new Action0() {

@Override
public void call() {
handler.removeCallbacks(runnable);

}

});
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the returned Subscription to innerSubscription? innerSubscription also needs to be a CompositeSubscription

Copy link
Author

Choose a reason for hiding this comment

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

Hm, wouldn't that cause the original problem to occur again? If innerSubscription gets unsubscribed, it would cancel the runnable, effectively doing the same thing as before this change.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's the expected behavior. If innerSubscription gets unsubscribed, all scheduled actions should be cancelled. You can take a look at NewThreadScheduler.NewThreadWorker. Could you test with NewThreadScheduler to see if there is same issue?

Copy link
Author

Choose a reason for hiding this comment

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

I will try to run a test tomorrow but it's rather hard to reproduce as it requires the new thread to be slightly blocked. But I read the code from NewThreadExecutor and it doesn't cancel any scheduled tasks that I can tell. It does call executor.shutdown() but that still executes all previously submitted tasks.

Copy link
Member

Choose a reason for hiding this comment

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

NewThreadScheduler uses executor.shutdown() which allows already submitted tasks to run but prevents new tasks being scheduled. I think this is the wrong behavior there and I should have used executor.shutdownNow() instead; a fix is underway. Since the Handler scheduler can't be stopped and thus stopping all tasks, you need to keep track of the worker's submitted tasks.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should have typed EventLoopsScheduler.EventLoopWorker

}

Expand Down