Skip to content

Provide factories for creating the default scheduler instances. #3856

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

Merged
merged 1 commit into from
Apr 18, 2016
Merged

Provide factories for creating the default scheduler instances. #3856

merged 1 commit into from
Apr 18, 2016

Conversation

JakeWharton
Copy link
Contributor

Unlike other hooks, the RxJavaSchedulersHook has no access to the real Scheduler instances in order to do wrapping/delegation. With these factory methods, a hook can access what would otherwise be the instance used since there is often no other means of creating these specialized schedulers.

For Android this wrapping/delegation use-case is important for UI testing. We have a means to tell the testing framework when the app is idle and to do that we need to hook into the schedulers to know when they're empty. This is easy to do currently, but you cannot wrap the real instance and instead have to supply alternate implementations which might subtly alter the behavior under test.

These three methods are referenced in #3724, and I think providing the defaults is useful as well as eventually adding overloads which take ThreadFactory instances for each.

@artem-zinnatullin
Copy link
Contributor

Please correct me if I'm wrong but looks like as soon as you access one of the new methods, static final Schedulers.INSTANCE will be instantiated which will make impossible to use RxJavaSchedulersHook as you described. Have you checked described use case? Maybe add such tests as example if possible?

We have a means to tell the testing framework when the app is idle and to do that we need to hook into the schedulers to know when they're empty.

Just wanted to say for those who may find this PR later: empty/non-empty state of schedulers is not 100% source of truth for ui tests, you may have some background or time related (periodic/etc) code that does not affect the ui directly and may even freeze tests.

@JakeWharton
Copy link
Contributor Author

You are accessing the methods from the hook which was called from the static initializer of this class on first reference to Schedulers.io() or the like. I'll add a test.

@artem-zinnatullin
Copy link
Contributor

Yup, but there is still potential problem if somebody will try to create schedulers using these methods outside of the hook, for example in @BeforeClass/etc or probably even just for non-test needs -> may result in hours of trying to understand why hook is not overriding schedulers…

Maybe move these methods to RxJavaSchedulersHook?

@JakeWharton
Copy link
Contributor Author

That prevents package scoped methods from being used in the factory.

@artem-zinnatullin
Copy link
Contributor

That prevents package scoped methods from being used in the factory.

We can move schedulers to internal.schedulers to "defend" from that (otherwise somebody can always use reflection to break package scope) and make constructors public.

Ideal solution would be to return schedulers in RxJavaSchedulersHook by default instead of returning null like it does now, so calling super.getSomeScheduler() will give original instance that you can wrap/etc as you wish. Though, we'll have to handle null as default too.

// sorry, I'm very tired and want to sleep, so I may come to wrong conclusions.

*/
@Deprecated
public final class NewThreadScheduler extends Scheduler {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to just delete this type. It was public yes, but it was final, had a private constructor, instance() was package scoped, and Schedulers.newThread() was never guaranteed to return instance of it. This basically made it completely unusable from application code.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to that, we can just mention that in changelog.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but not sure when the delete could happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be fine for even patch release, NewThreadScheduler has no public fields nor methods + private constructor. Without reflection or unchecked casting it's unusable for users.

I vote for removing it right in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, it's relatively safe to remove now (even with a patch release).

@JakeWharton
Copy link
Contributor Author

The factories were moved to the hook.

@akarnokd
Copy link
Member

👍

@artem-zinnatullin
Copy link
Contributor

👍 now, but let me ask again, what about this:

Ideal solution would be to return schedulers in RxJavaSchedulersHook by default instead of returning null like it does now, so calling super.getSomeScheduler() will give original instance that you can wrap/etc as you wish. Though, we'll have to handle null as default too.

No need for new methods, no need to fight with instantiation order and package scope visibility. Yes, user won't be able to instantiate new scheduler of required type outside of the hook, but it's not possible at the moment, so nobody loses nothing.

@stevegury
Copy link
Member

👍

@akarnokd akarnokd merged commit a429815 into ReactiveX:1.x Apr 18, 2016
@JakeWharton JakeWharton deleted the jw/scheduler-factories branch August 20, 2016 23:22
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.

4 participants