2.x: Add scheduler creation factories by ZacSweers · Pull Request #5002 · ReactiveX/RxJava

ZacSweers · 2017-01-18T11:31:39Z

Resolves #4993

This is a pretty vanilla copy from RxJava 1's implementation. Note that I had to tune NewThread scheduler to not be a singleton to support this.

We had talked about borrowing from project reactor's APIs for different overloads, let me know if you think we should add more fine-grained controls through these.

Resolves ReactiveX#4993 This is a pretty vanilla copy from RxJava 1's implementation. Note that I had to tune NewThread scheduler to not be a singleton to support this. We had talked about borrowing from project reactor's APIs for different overloads, let me know if you think we should add more fine-grained controls through these.

ZacSweers · 2017-01-18T11:32:47Z

One other thing I'm worried about - is Schedulers the best place for this? RxJava 1 had them in its RxJavaHooks class. My concern is that having these alongside the normal io()/computation()/etc factories will make them look overly inviting for use and accidental abuse.

akarnokd · 2017-01-18T11:36:35Z

src/main/java/io/reactivex/internal/schedulers/IoScheduler.java


        EVICTOR_THREAD_FACTORY = new RxThreadFactory(EVICTOR_THREAD_NAME_PREFIX, priority);
+
+        NONE = new CachedWorkerPool(0, null, WORKER_THREAD_FACTORY);


Any particular reason why this was moved to the end?

It requires WORKER_THREAD_FACTORY to be initialized now since it's a parameter

akarnokd · 2017-01-18T11:39:01Z

src/main/java/io/reactivex/internal/schedulers/SingleScheduler.java

+     *                      system properties for configuring new thread creation. Cannot be null.
+     */
+    public SingleScheduler(ThreadFactory threadFactory) {
+        this.threadFactory = ObjectHelper.requireNonNull(threadFactory, "threadFactory was null");


Either this is unnecessary or you forgot to do this with the other new constructors.

Oops, meant to remove it from all these constructors and just do it in the factory methods, accidentally missed this one

akarnokd · 2017-01-18T11:40:21Z

src/main/java/io/reactivex/schedulers/Schedulers.java


+    /**
+     * Create an instance of the default {@link Scheduler} used for {@link Schedulers#computation()}.
+     * @return the created Scheduler instance


please add @since 2.0.5 - experimental as well for every new method.

akarnokd · 2017-01-18T11:41:31Z

src/main/java/io/reactivex/schedulers/Schedulers.java

+     * Create an instance of the default {@link Scheduler} used for {@link Schedulers#computation()}.
+     * @return the created Scheduler instance
+     */
+    @Experimental


I'm not sure we'd want to allow non-parameterized instances to be created.

Could you expand on that a bit?

What is the use case for creating yet another computation/io/newThread scheduler with the default settings? I thought you wanted to override the ThreadFactory.

akarnokd · 2017-01-18T11:42:41Z

src/main/java/io/reactivex/schedulers/Schedulers.java

+     * @return the created Scheduler instance
+     */
+    @Experimental
+    public static Scheduler newComputation(ThreadFactory threadFactory) {


The usual NPE message is threadFactory is null.

Gotcha, will update

akarnokd · 2017-01-18T11:48:29Z

One other thing I'm worried about - is Schedulers the best place for this? RxJava 1 had them in its RxJavaHooks class. My concern is that having these alongside the normal io()/computation()/etc factories will make them look overly inviting for use and accidental abuse.

Another problem is that people will try to override the scheduler-init with these method calls for RxJavaPlugins but referencing Schedulers will init the default schedulers regardless.

Indeed, RxJavaPlugins sounds like a better place for the methods, but keep only the ThreadFactory variants.

ZacSweers · 2017-01-18T11:49:15Z

Heading to bed right now but will update the PR in the morning with requested changes. Also let me know if there's anywhere I should put tests, as I wasn't sure what the right place would be.

ZacSweers · 2017-01-18T11:50:08Z

Indeed, RxJavaPlugins sounds like a better place for the methods, but keep only the ThreadFactory variants.

Sounds good to me, will do that in my updates tomorrow

akarnokd · 2017-01-18T11:54:07Z

Yes, tests that verify the custom thread factory actually worked by checking a custom thread name for each case.

codecov-io · 2017-01-18T12:09:51Z

Current coverage is 95.46% (diff: 79.48%)

Merging #5002 into 2.x will increase coverage by 0.02%

@@                2.x      #5002   diff @@
==========================================
  Files           592        592          
  Lines         37989      38009    +20   
  Methods           0          0          
  Messages          0          0          
  Branches       5772       5772          
==========================================
+ Hits          36257      36284    +27   
+ Misses          764        761     -3   
+ Partials        968        964     -4

Powered by Codecov. Last update 9c34eb1...2ae2414

Added scheduler because we're not in Scheduler anymore. Changed to "create" because "newNewThread" was weird

ZacSweers · 2017-01-19T07:59:54Z

Indeed, RxJavaPlugins sounds like a better place for the methods

8b4d461

but keep only the ThreadFactory variants.

8009333

Also tweaked the naming a bit. newNewThread was a little weird (went with create), and added Scheduler suffix since we're not in the Schedulers class anymore.

Yes, tests that verify the custom thread factory actually worked by checking a custom thread name for each case.

I tried setting this up matching some of the cdl-based approaches in the plugin tests but wan't able to get it working (just hangs). Pushed what I had in a1029b4, any insight?

ZacSweers · 2017-01-21T21:17:52Z

Heading on vacation for a couple weeks, but let me know what you think of getting the tests to work. I should have some time here and there to update the PR

akarnokd · 2017-01-21T21:27:16Z

I've already told you: use the runnable in the factory method on the Thread constructor.

ZacSweers · 2017-01-24T17:11:57Z

use the runnable in the factory method on the Thread constructor.

Aren't I just testing the test ThreadFactory implementations then, and not that the schedulers are hooked up properly to power the scheduler?

akarnokd · 2017-01-24T17:14:35Z

https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html

https://github.com/ReactiveX/RxJava/blob/2.x/src/main/java/io/reactivex/internal/schedulers/RxThreadFactory.java#L66

JakeWharton · 2017-01-24T19:16:59Z

src/test/java/io/reactivex/plugins/RxJavaPluginsTest.java

+
+    private static void verifyThread(Worker w, Predicate<Thread> threadPredicate) {
+        try {
+            try {


remove double try

JakeWharton · 2017-01-24T19:17:42Z

src/test/java/io/reactivex/plugins/RxJavaPluginsTest.java

+    private static void verifyThread(Worker w, Predicate<Thread> threadPredicate) {
+        try {
+            try {
+                final AtomicReference<Thread> value = new AtomicReference<>();


Maybe change to AtomicBoolean and just run the predicate inside the worker?

Then you don't even need to supply the Thread to the Predicate actually. It can verify whatever it wants.

JakeWharton · 2017-01-24T19:21:50Z

What's the problem with the tests? They seem to be verifying that the supplied factory was used.

JakeWharton · 2017-01-24T19:22:44Z

Oh you didn't pass the supplied Runnable when calling new Thread() inside the factory.

ZacSweers · 2017-01-25T02:32:49Z

Oh you didn't pass the supplied Runnable when calling new Thread() inside the factory.

Ah! This is what I was missing, thanks for pointing that out. I've updated tests (which should now all be passing), and also made them a bit more robust (full integration with schedulers) to simulate a more real world use case.

ZacSweers · 2017-01-25T02:42:43Z

Failing test looks like a flake...

ZacSweers · 2017-01-25T02:48:14Z

Wait no it's not. Repro'd locally, will fix

ZacSweers · 2017-01-25T06:16:55Z

Fixed, I think. I added manual shutdowns of schedulers after they're done. Only IO had this lifecycle issue over tests, but I did the shutdown in all to be safe.

ZacSweers · 2017-01-25T06:31:41Z

Ok this time it seems actually flaky, as the same commit passed in a different travis job against my fork - https://travis-ci.org/hzsweers/RxJava/builds/195074173

akarnokd · 2017-01-18T11:46:02Z

src/main/java/io/reactivex/schedulers/Schedulers.java

+        return new ComputationScheduler();
+    }
+
+    /**


These need some rewording emphasizing that the there is always a new Scheduler instance, unlike with the non-new method call.

akarnokd · 2017-01-18T11:49:32Z

src/main/java/io/reactivex/internal/schedulers/IoScheduler.java


        EVICTOR_THREAD_FACTORY = new RxThreadFactory(EVICTOR_THREAD_NAME_PREFIX, priority);
+
+        NONE = new CachedWorkerPool(0, null, WORKER_THREAD_FACTORY);


akarnokd · 2017-01-18T11:52:48Z

src/main/java/io/reactivex/schedulers/Schedulers.java

+     * Create an instance of the default {@link Scheduler} used for {@link Schedulers#computation()}.
+     * @return the created Scheduler instance
+     */
+    @Experimental


What is the use case for creating yet another computation/io/newThread scheduler with the default settings? I thought you wanted to override the ThreadFactory.

akarnokd · 2017-01-19T07:59:17Z

src/test/java/io/reactivex/plugins/RxJavaPluginsTest.java

+        ThreadFactory factory = new ThreadFactory() {
+            @Override
+            public Thread newThread(Runnable r) {
+                return new Thread("Test");


I think this doesn't allow executing tasks because you ignore r.

akarnokd · 2017-01-19T13:57:41Z

src/test/java/io/reactivex/plugins/RxJavaPluginsTest.java

+                return "Test".equals(thread.getName());
+            }
+        });
+    }


You have to shut down the scheduler (in a finally block) to avoid leaking threads.

akarnokd · 2017-01-19T14:38:29Z

src/main/java/io/reactivex/plugins/RxJavaPlugins.java


+    /**
+     * Create an instance of the default {@link Scheduler} used for {@link Schedulers#computation()}
+     * except using {@code threadFactory} for thread creation.


Please add notes to all of these methods something like this:

Note that the returned Scheduler must be shut down manually if the ThreadFactory doesn't create a daemon thread, otherwise the JVM may not quit.

The "Note that this takes precedence..." may be misleading. The number of threads is still based on the system configuration. I'd remove this sentence entirely

ZacSweers · 2017-01-25T08:23:18Z

Thanks!

akarnokd requested changes Jan 18, 2017

View reviewed changes

akarnokd added 2.x Enhancement labels Jan 18, 2017

akarnokd added this to the 2.1 milestone Jan 18, 2017

ZacSweers added 7 commits January 18, 2017 23:18

Add @since info

262f669

Change failure string to "is null"

681b977

Move to RxJavaPlugins

8b4d461

Remove no-arg overloads

8009333

Rename to make it clearer about creation

66111c5

Added scheduler because we're not in Scheduler anymore. Changed to "create" because "newNewThread" was weird

Add tests (WIP)

a1029b4

Remove unnecessary nullcheck

e5c4a9e

akarnokd changed the title ~~Add scheduler creation factories~~ 2.x: Add scheduler creation factories Jan 19, 2017

JakeWharton approved these changes Jan 24, 2017

View reviewed changes

ZacSweers added 2 commits January 25, 2017 08:20

Remove double try

90ae664

Fix tests, make them more robust with integration flow

6a08b92

Shut down custom schedulers when done

ea1ec6c

Merge branch '2.x' into schedulerFactories

00af140

akarnokd approved these changes Jan 25, 2017

View reviewed changes

akarnokd merged commit f53e029 into ReactiveX:2.x Jan 25, 2017

akarnokd mentioned this pull request Jan 25, 2017

2.0.5 release preparations #4983

Closed

ZacSweers deleted the schedulerFactories branch January 25, 2017 08:23

akarnokd mentioned this pull request Jan 25, 2017

2.x: option to fail for using blockingX on the computation scheduler #5020

Merged


		EVICTOR_THREAD_FACTORY = new RxThreadFactory(EVICTOR_THREAD_NAME_PREFIX, priority);

		NONE = new CachedWorkerPool(0, null, WORKER_THREAD_FACTORY);

Conversation

ZacSweers commented Jan 18, 2017

Uh oh!

ZacSweers commented Jan 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akarnokd commented Jan 18, 2017

Uh oh!

ZacSweers commented Jan 18, 2017

Uh oh!

ZacSweers commented Jan 18, 2017

Uh oh!

akarnokd commented Jan 18, 2017

Uh oh!

codecov-io commented Jan 18, 2017

Current coverage is 95.46% (diff: 79.48%)

Uh oh!

ZacSweers commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZacSweers commented Jan 21, 2017

Uh oh!

akarnokd commented Jan 21, 2017

Uh oh!

ZacSweers commented Jan 24, 2017

Uh oh!

akarnokd commented Jan 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakeWharton commented Jan 24, 2017

Uh oh!

JakeWharton commented Jan 24, 2017

Uh oh!

ZacSweers commented Jan 25, 2017

Uh oh!

ZacSweers commented Jan 25, 2017

Uh oh!

ZacSweers commented Jan 25, 2017

Uh oh!

ZacSweers commented Jan 25, 2017

Uh oh!

ZacSweers commented Jan 25, 2017

Uh oh!

Choose a reason for hiding this comment

ZacSweers commented Jan 19, 2017 •

edited

Loading