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

Ensuring Runnables posted with delay to a Handler are removed when unsub... #1321

Merged
merged 4 commits into from
Jun 12, 2014

Conversation

dpsm
Copy link
Contributor

@dpsm dpsm commented Jun 4, 2014

...scribed.

This patch ensures the delayed runnables posted to a Handler are properly
removed when Subscription.unsubscribe() is called on the Observable.

The original code returns the subscription from schedule() but is not used
by the callers who instead add the Worker itself as a subsciption.

Signed-off-by: David Marques dpsmarques@gmail.com

…subscribed.

This patch ensures the delayed runnables posted to a Handler are properly
removed when Subscription.unsubscribe() is called on the Observable.

The original code returns the subscription from schedule() but is not used
by the callers who instead add the Worker itself as a subsciption.

Signed-off-by: David Marques <dpsmarques@gmail.com>
@benjchristensen
Copy link
Member

/cc @akarnokd @zsxwing for review please.

@benjchristensen
Copy link
Member

Thanks @dpsm for the contribution and fixes.

@cloudbees-pull-request-builder

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

@akarnokd
Copy link
Member

akarnokd commented Jun 4, 2014

Generally, the entire schedule(Action0, long, TimeUnit) is incorrect:

  1. it has to return a valid subscription that cancels the submitted action: this PR returns an empty subscription which won't even cancel a direct schedule.
  2. the action should to monitor its own cancellation token instead of the worker's overall subscription,
  3. it has to track scheduled tasks; this is covered in this PR
  4. it has to remove completed tasks from the tracking structure; in this PR, without it, the scheduler leaks subscriptions until terminated.

You need to introduce a new class similar to ScheduledAction which is itself a runnable and a Subscription: you can add it to the tracking composite and it can remove itself from the very same composite on completion, you can return it as the Subscription token and it can check itself before executing the actual action.

…iour.

Signed-off-by: David Marques <dpsmarques@gmail.com>
@cloudbees-pull-request-builder

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


});
}));

Copy link
Member

Choose a reason for hiding this comment

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

You need to add the scheduledAction to the composite:

mCompositeSubscription.add(scheduledAction);

…ubscribe.

Signed-off-by: David Marques <dpsmarques@gmail.com>
@cloudbees-pull-request-builder

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

@akarnokd
Copy link
Member

akarnokd commented Jun 5, 2014

The changes are now fine with me. @zsxwing ?

@zsxwing
Copy link
Member

zsxwing commented Jun 5, 2014

LGTM

@benjchristensen
Copy link
Member

@mttkay Are you okay with these changes and their impact on Android?

@@ -58,35 +61,29 @@ public InnerHandlerThreadScheduler(Handler handler) {

@Override
public void unsubscribe() {
innerSubscription.unsubscribe();
mCompositeSubscription.unsubscribe();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think RxJava uses the Android code style anywhere. We shouldn't adopt it here.

…ports.

Signed-off-by: David Marques <dpsmarques@gmail.com>
@cloudbees-pull-request-builder

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

@mttkay
Copy link
Contributor

mttkay commented Jun 7, 2014

👍

@dpsm
Copy link
Contributor Author

dpsm commented Jun 8, 2014

@benjchristensen given everyone's approval when could we get it into master? I am new to the project and want to contribute a lot more specially in the android side of things! Is there anywhere I can read about the roadmap or some idea on release process, SNAPSHOT builds, etc? Thanks!

benjchristensen added a commit that referenced this pull request Jun 12, 2014
Ensuring Runnables posted with delay to a Handler are removed when unsub...
@benjchristensen benjchristensen merged commit 56d76bb into ReactiveX:master Jun 12, 2014
@benjchristensen
Copy link
Member

You can read about the roadmap here: #1001

The plan is to split RxAndroid out into it's own top-level project at http://github.com/ReactiveX/RxAndroid in the next month or two.

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

6 participants