Add Schedulers.reset() for better testing

[ZacSweers]

Resolves #3985

This adds a reset() method to Schedulers, with the main benefit being improved testing support. This does slightly tweak the internal API of Schedulers to use a getInstance() approach to allow lazy init. This way we don't have to replace the singleton instance during reset() and allow it to lazily re-evaluate upon next usage. Otherwise, if you change your scheduler hook, you'd always have to make sure you set it before you call Schedulers.reset().

Will run perf tests overnight in case, I'm not sure how much of a tradeoff moving to an internal getInstance() approach costs, if anything.

CC @zsxwing

[ZacSweers]

I think the failing test might be flaky, it doesn't fail for me locally.

[akarnokd]

Please use atomics instead:

private static final AtomicReference<Schedulers> INSTANCE = new AtomicReference<Schedulers>();

private static Schedulers getInstance() {
    for (;;) {
         Schedulers current = INSTANCE.get();
         if (current != null) {
            return current;
         }
         current = new Schedulers();
         if (INSTANCE.compareAndSet(null, current)) {
             return current;
         } else {
             current.shutdown();
         }
    }
}

[vanniktech]

Just curious what's the advantage of this?

[akarnokd]

Synchronized holds the potential of getting blocked if somebody else is already in it. If the Schedulers is not constantly and concurrently reset all the time, the atomic approach is a single non-blocking volatile read of a non-null instance value.

[ZacSweers]

shutdown() is currently a static method. Should current.shutdown() be just shutdown() or should shutdown() be changed to an instance method?

[akarnokd]

Two problems:

• shutdown() is public API and that would be an incompatible change
• you'd have to expose Schedulers as an instance somehow call an instance method on it.

[ZacSweers]

What do you propose?

[akarnokd]

Leave the shutdown() as it is now.

[ZacSweers]

Alright. Do you think there's any risk of deadlock since we'd be calling it from getInstance() and it calls getInstance() internally?

[akarnokd]

No.

[ZacSweers]

updated in fe2157c

[akarnokd]

👍

[ZacSweers]

Thanks! I don't suppose this could be squeezed into the requires review of #3970 could it?

[ZacSweers]

That test doesn't fail for me locally, I'm not really sure what do do about it. Any ideas?

[akarnokd]

I've extended the timeout in some tests in PR #3987. Once it appears it was enough, I'll merge that and this PR should work.

[ZacSweers]

Cool, I'll rebase after that's merged then 👍

[akarnokd]

Non need to rebase but to rerun the travis job. Did it for you.

[ZacSweers]

Ah I pushed the rebase as you commented. Oh well ¯_(ツ)_/¯

[ZacSweers]

Cool, looks like the timeout tweaks worked

[artem-zinnatullin]

Code is ok, so 👍

But

Resetting the schedulers is dangerous
during application runtime and also bad code could invoke it in
the middle of an application life-cycle and really break applications
if not used cautiously.

Why we're adding more and more APIs to break things and allow people use bad practices?

[ZacSweers]

It's two, and they're largely targeted at testing. I don't think having a reset is bad practice and, like most code, only breaks things if you make poor decisions with it. I could just as easily add a bad scheduling hook or buggy global error handler.

Having to otherwise use DI to inject schedulers everywhere gets tedious fast, and inadvertantly discourages people from actually using the conventional static APIs. With this, it can be wrapped up in a nice test rule and enforces a clean state before and after tests. Using the plugins API also allows you to control the schedulers used across modules and 3rd party libraries as well.

For me, it's a missing and much needed API. I don't think requiring DI everywhere you use a scheduler just to test is a scalable approach considering how ubiquitous they are. Could you imagine injecting, say, Timber everywhere you wanted to use it?

That's my speel for why I think this is useful.

[akarnokd]

I wonder why people with such testing needs don't roll their own global "MySchedulers" class which let's them easily switch scheduler types and use that like observeOn(MySchedulers.forComputation()). Of course this means you have to remember to use the Scheduler-overloaded operators everywhere.

[ZacSweers]

I also just copied that doc from the other reset. I don't know that it's as dangerous though, it just forces a re-init. If it's dangerous, it's because resetting rx plugins is dangerous. Since they're not explicitly bound, I wouldn't mind making the doc warning a little less... Severe? :P

[ZacSweers]

3rd party libraries don't use your app's global class :/. Plus that case still requires you to hook in a delegate scheduler in tests early enough. We're adding a global scheduler of sorts for background work and still planning to have a reset on the schedulers.

[akarnokd]

Libraries should expose the option to customize the Scheduler they run on. Do you know a library that doesn't allow such customization?

[ZacSweers]

Most, actually. They just use the built-in Schedulers.whatever() or AndroidSchedulers.mainThread(). I don't see how a wrapper with static methods solves this either, you still need some mechanism of resetting the underlying scheduler.

Is there any harm in allowing reset here to facilitate just having developers go through the standard API?

[ZacSweers]

But here's a quick example off the top of my head: https://github.com/mcharmas/Android-ReactiveLocation

[akarnokd]

Otherwise, I think the whole plugin system in RxJava 1.x is getting cumbersome. In 2.x, I did a much simpler and direct approach for overriding stuff.

For example, this is how the schedulers get initialized:

https://github.com/ReactiveX/RxJava/blob/2.x/src/main/java/io/reactivex/schedulers/Schedulers.java#L47

This is where they are returned:

https://github.com/ReactiveX/RxJava/blob/2.x/src/main/java/io/reactivex/schedulers/Schedulers.java#L57

Changing what scheduler is returned from Schedulers is easy via

https://github.com/ReactiveX/RxJava/blob/2.x/src/main/java/io/reactivex/plugins/RxJavaPlugins.java#L305

by adding a function that can wrap or completely replace the scheduler.

I don't know if Netflix wants 1.2 at all, but if so, I'd be glad to rewrite the plugin system.

[akarnokd]

But here's a quick example off the top of my head: https://github.com/mcharmas/Android-ReactiveLocation

But where does it use RxJava schedulers forcefully?

[akarnokd]

Found one: ReverseGeocodeObservable.java

They should have provided a way to customize that scheduler anyway.

[artem-zinnatullin]

100% agreed with David. That should be the direction RxJava should go to:
teach users how to do either DI for schedulers or own MySchedulers class
with resetting API and teach library developers to accept Schedulers from
user.

Otherwise we'll stuck in a loop like this where we're adding one API that
affects different parts of the library and then we need to add another API
to "fix" problems of previous one.

On Fri, 3 Jun 2016, 15:35 David Karnok, notifications@github.com wrote:

Libraries should expose the option to customize the Scheduler they run
on. Do you know a library that doesn't allow such customization?

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#3986 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AA7B3IWb3BPQqz2qWzwylStcZG7flV4Nks5qIB-igaJpZM4ItOcB
.

[ZacSweers]

Not disagreeing that it would be preferable for libraries to expose a way, but in practice I've seen that most do not.

The 2.x plugin approach looks nice, though the lockdown bit seems like it imposed the same restrictions that 1.x plugins have. I don't know what the 1.2 plans are, but between having something like this now and waiting for a 1.2 or 2.x release, I'd much rather have a necessary evil now with an eye toward the future. I don't really subscribe to the slippery slope worry considering this is part of what the last one was supposed to help improve.

It's not a matter of people not understanding DI, it's that DI is not a scalable solution for this. It's unnecessary tedium and boilerplate, why even have the static API at all if it's not going to be used? Are you going to add DI to rx util libraries?

Between getting all library developers to update, waiting for 2.x, rewriting the plugin system, or doing this in a safe way now and banking on 2.x long term, I'd rather have the last option. It works for all cases now and has fairly minimal impact/safety concerns.

[ZacSweers]

Well let me know what you guys want to do. I feel like this is breaking down over the current plugins API, which we all agree is not perfect. The way I see it, any "MyPlugins" approach would still face the same issue in that you'd need to implement some way of internally delegating.

To me, this is a reasonable short-term solution for right now considering 1.2 and 2.x are far off and I think the framework should facilitate use of whatever patterns developers prefer rather than impose them. Would you be agreeable to doing this for now with the long term goal being to revamp the plugins API in the future?

I've updated the doc as well, since it's actually not that dangerous and only causes behavioral changes if the plugin hook has changed (which is already documented as such).

[ZacSweers]

Also @zsxwing seemed in favor of this change in the issue, do you want to maybe chime in here?

[mmallozzi]

While I do agree with Artem/David that a better long term solution here would be to pass in Scheduler objects where needed (setting things up for DI where developers choose it - the scalability of DI isn't the question here) rather than relying on static methods to get them, this PR seems like the best way forward for how things currently work in Rx. There has been lots of discussion elsewhere about how replacing a Scheduler for testing is very painful if you make use of the standard Schedulers static methods to get your Scheduler instances, and the reset methods for the plugin hook only work halfway because of the current static caching behavior. Because the current reset-for-testing functionality is fundamentally broken, I strongly recommend accepting this PR to fix it, and take this discussion into account for the design of upcoming versions of plugins or developer usage recommendations.

[zsxwing]

Because the current reset-for-testing functionality is fundamentally broken, I strongly recommend accepting this PR to fix it

Agreed. This PR is pretty useful for unit tests. 👍 for merging this one.

[artem-zinnatullin]

May sound crazy, but what about marking reset() as @Deprecated with explanation of better ways?

[ZacSweers]

I think @Experimental with a doc explaining that the API will be revamped in the future would make more sense. I don't have strong feelings for or against @Deprecated though and can switch to that if others want. Anything to help move this PR along :)

[artem-zinnatullin]

@Deprecated will produce compile warning and will be visible in IDE, pretty sure most of the people who will use Schedulers.reset() will copy-paste from SO or some blog post which will arrive as soon as we merge/release this and won't even know that it's @Experimental.

// I'm not blocking PR if that matters.

[ZacSweers]

Fair point. @akarnokd any thoughts? It seems like this has the requisite 👍's to move forward, can add deprecated if you think it's worthwhile.

[JakeWharton]

Do we really expect people to accidentally use this and be surprised?
Deprecating the method sounds like it punishes those who want to use it.
I'd deprecate create() and others long before this method.

On Mon, Jun 6, 2016, 5:19 PM Zac Sweers notifications@github.com wrote:

Fair point. @akarnokd https://github.com/akarnokd any thoughts? It
seems like this has the requisite 👍's to move forward, can add
deprecated if you think it's worthwhile.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3986 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAEEEahh6igqbI7KuZYAgrNavXgcEkqWks5qJI7wgaJpZM4ItOcB
.

[ZacSweers]

When you put it that way, I think I agree.

[akarnokd]

Don't deprecate it.

[artem-zinnatullin]

Do we really expect people to accidentally use this and be surprised?

Have seen/helped people asking how to solve problems after applying scheduler hooks for testing, they didn't realize (mostly) how it can break things.

create() is local evil, it doesn't break global state.

Alternative solution would be to move APIs like create(), reset() and so on to rx.Unsafe, like Java, Rust and so on, it's there but use it at your own risk.