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

Adding ScheduledAction to public API #2592

Closed
wants to merge 3 commits into from

Conversation

akarnokd
Copy link
Member

@akarnokd akarnokd commented Feb 3, 2015

Resubmit of #2579.

I've copied the internal ScheduledAction into the public rx.schedulers package to allow custom Scheduler implementors in other projects to take advantage of its correct unsubscription management.

I've removed the internal ScheduledAction. Since relying on internal features are discouraged anyway, use places outside RxJava need to be updated. Deprecating it doesn't work because the class is final and the internals now return the new rx.schedulers.ScheduledAction and would result in ClassCastException anyway.

I've introduced a system-wide parameter io.reactivex.scheduler.interrupt-on-unsubscribe to enable interruption globally. In addition, each ScheduledAction has its own setInterruptOnUnsubscribe which allows enabling/disabling interrupts on an individual basis (overrides the default from the system-wide parameter above for the instance).

I've also added detailed documentation to it and (while I was in the mood) to the NewThreadWorker.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
zalegrala Zach Leslie

Verified

This commit was signed with the committer’s verified signature. The key has expired.
zalegrala Zach Leslie
@benjchristensen
Copy link
Member

Trying to think through the implications of this being part of the public API. Is it okay if I leave this out of 1.0.5 and we can follow up with 1.0.6 soon?

@akarnokd
Copy link
Member Author

akarnokd commented Feb 3, 2015

Sure.

* Note, however, if the {@code actualWorker} above didn't return a ScheduledAction, there is no
* good way of untracking the returned {@code Subscription} (i.e., when to call {@code outerParent.remove(s)}).
*/
public final class ScheduledAction implements Runnable, Subscription {
Copy link
Member

Choose a reason for hiding this comment

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

It's better to add @experimental or @beta for the new public class.

@benjchristensen
Copy link
Member

I was playing with this in Hystrix with @mattrjacobs and it seemed we could achieve the desired reuse by adding a static factor methods to Schedulers without needing to expose this implementation detail. The add, addParent stuff is very non-obvious and needs to be done "just right" or it doesn't work. All of that seems like it should be encapsulated behind a factory method.

Here is example code: https://github.com/Netflix/Hystrix/blob/19320db57cdb4b600021875133c7ec61b4c96b82/hystrix-core/src/main/java/com/netflix/hystrix/strategy/concurrency/HystrixContextScheduler.java#L160-171

Could we replace this with an API call such as Schedulers.submitTo(executor, action, subscription, shouldInterrupt);?

@akarnokd
Copy link
Member Author

Sounds like a good alternative, although it would require the subscription to be a CompositeSubscription. I'd introduce a SubscriptionCollection interface with add, remove and removeSilently so more optimized collections may be used.

@mattrjacobs
Copy link
Contributor

+1 on moving this complexity to RxJava and not leaving it to consumers to get right.

@akarnokd
Copy link
Member Author

Okay. I'll add the utility method and keep the ScheduledAction changes private.

@akarnokd
Copy link
Member Author

See #2761 for the new proposal.

@akarnokd akarnokd deleted the ScheduledActionAPI branch May 6, 2015 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants