2.x: Evaluate Schedule initialization via Callable by peter-tackage · Pull Request #4585 · ReactiveX/RxJava

peter-tackage · 2016-09-22T22:24:17Z

This implements the solution proposed in #4572 - to initialize the Schedulers via a Callable, rather than directly via a value.

vanniktech · 2016-09-22T22:29:27Z

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

+    }
+
+    /**
+     * Wraps the call to the callable in try-catch and propagates thrown


copy paste error

vanniktech · 2016-09-22T22:30:54Z

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

     * @return the value returned by the hook
     */
-    public static Scheduler initComputationScheduler(Scheduler defaultScheduler) {
+    public static Scheduler initComputationScheduler(Callable<Scheduler> defaultScheduler) {


Should not it throw on a null callable? What's the point of calling with null?

This was done for consistency with the existing expectations in RxJavaPluginsTest.clearIsPassthrough(), specifically:

assertNull(RxJavaPlugins.initComputationScheduler(null)); assertNull(RxJavaPlugins.initIoScheduler(null)); assertNull(RxJavaPlugins.initNewThreadScheduler(null)); assertNull(RxJavaPlugins.initSingleScheduler(null));

Should this be changed to only return null if the Callable returns null (and throw if the Callable itself is null)?

Null should not be allowed as a return value from the Callable nor from the init Function.

This would be a change, because previously the Scheduler value for RxJavaPlugins.init* was allowed to be null, as per - assertNull(RxJavaPlugins.initSingleScheduler(null));.

I will add an additional set of tests for the new behavior (something along the lines of assemblyHookCrashes).

vanniktech · 2016-09-22T22:32:08Z

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

+     * @param t the callable, not null (not verified)
+     * @return the result of the callable call
+     */
+    static <T> T call(Callable<T> t) {


inline this method?

Not sure what you mean. I've tried to be consistent with the abstraction of the similar apply method. Do you think that should change?

codecov-io · 2016-09-22T22:41:09Z

Current coverage is 78.17% (diff: 88.88%)

Merging #4585 into 2.x will increase coverage by 0.08%

@@                2.x      #4585   diff @@
==========================================
  Files           552        552          
  Lines         36184      36297   +113   
  Methods           0          0          
  Messages          0          0          
  Branches       5584       5602    +18   
==========================================
+ Hits          28255      28375   +120   
+ Misses         5917       5912     -5   
+ Partials       2012       2010     -2

Powered by Codecov. Last update 24448b4...a273507

…he Callable

JakeWharton · 2016-09-23T03:37:07Z

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

+     * @return the callable result if the callable is nonnull, null otherwise.
+     */
+    static <T> T callOrNull(Callable<T> t) {
+        return t == null ? null : call(t);


null should never be allowed

vanniktech · 2016-09-24T20:09:46Z

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

+     * @throws NullPointerException if the callable returns null
+     */
+    static <T, R> R apply(Function<T, R> f, Callable<T> t) {
+        try {


I think you mean to check value here instead of t

Also you could just T value = Object.requireNonNull(t.call(), "messge")

Yep, good find.

Yes, that would be better. I didn't notice the return value in the signature.

akarnokd · 2016-09-24T20:44:54Z

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

+        SINGLE = RxJavaPlugins.initSingleScheduler(new Callable<Scheduler>() {
+            @Override
+            public Scheduler call() throws Exception {
+                return new SingleScheduler();


Just in case call() is called multiple times by the hook, these could actually return an field of an inner class (singleton holder pattern or what's the name):

static final class SingleHolder { static final Scheduler DEFAULT = new SingleScheduler(); } SINGLE = RxJavaPlugins.initiSingleScheduler(new Callable<Scheduler>() { @Override public Scheduler call() { return SingleHolder.DEFAULT; } });

peter-tackage · 2016-09-24T23:11:21Z

@akarnokd, Correct me if I'm wrong, but in order to avoid the evaluation of the default Scheduler instance when it is being overridden, I still need to change the onInit[single|io|new|computation]Handler signatures to be Function<Callable<Scheduler>, Scheduler> as per -

    static volatile Function<Callable<Scheduler>, Scheduler> onInitSingleHandler;

Otherwise the invocation of initSingleScheduler will cause the evaluation of the default value to pass it as a parameter to the overriding/transforming function: onInitSingleHandler.

akarnokd · 2016-09-25T07:17:44Z

Yes, you still need Callable as the indirection but the inner class will be evaluated only if call is actually invoked.

peter-tackage · 2016-09-25T11:09:14Z

I've added the remaining lazy initialization.

I've also fairly aggressively enforced non-null in the associated functions, for example:

public static Scheduler initIoScheduler(Callable<Scheduler> defaultScheduler) {
    ObjectHelper.requireNonNull(defaultScheduler, "Scheduler Callable can't be null");
    Callable<Scheduler>, Scheduler> f = onInitIoHandler;
    if (f == null) {
        return callRequireNonNull(defaultScheduler);
    }
    return applyRequireNonNull(f, defaultScheduler);
}

However, to me, this seems slightly out of place / over the top. Is that enforcement necessary or should the resultant null Scheduler be left unasserted and left to the eventual NullPointerException when the Scheduler is used? Either way, I'm happy to keep or remove that based upon review feedback.

JakeWharton · 2016-09-25T16:09:18Z

Aggressive input validation is never over the top. If you defer checking
then the stacktrace tells you nothing about what actually caused the broken
state.

On Sun, Sep 25, 2016, 7:09 AM Peter Tackage notifications@github.com
wrote:

I've added the remaining lazy initialization.

I've also fairly aggressively enforced non-null in the associated
functions, for example:

public static Scheduler initIoScheduler(Callable defaultScheduler) {
ObjectHelper.requireNonNull(defaultScheduler, "Scheduler Callable can't be null");
Callable, Scheduler> f = onInitIoHandler;
if (f == null) {
return callRequireNonNull(defaultScheduler);
}
return applyRequireNonNull(f, defaultScheduler);
}

However, to me, this seems slightly out of place / over the top. Is that
enforcement necessary or should the resultant null Scheduler be left
unasserted and left to the eventual NullPointerException when the Scheduler
is used? Either way, I'm happy to keep or remove that based upon review
feedback.

—
You are receiving this because you commented.

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

peter-tackage · 2016-09-26T07:36:00Z

@akarnokd, all done as far as I am concerned. Do I need to anything else for this to be merged?

akarnokd · 2016-09-26T07:50:36Z

I was waiting for you to settle with the implementation. Thanks for the contribution.

Evaluate Schedule initialization via Callable

0ab5956

akarnokd changed the title ~~Evaluate Schedule initialization via Callable~~ 2.x: Evaluate Schedule initialization via Callable Sep 22, 2016

akarnokd added the Enhancement label Sep 22, 2016

vanniktech reviewed Sep 22, 2016

View reviewed changes

Clarify docs that Schedulers are initialized by the return value of t…

5112117

…he Callable

JakeWharton reviewed Sep 23, 2016

View reviewed changes

akarnokd added this to the 2.0 RC4 milestone Sep 23, 2016

Peter Tackage added 4 commits September 24, 2016 20:59

Enforce non-null Callable Scheduler and Scheduler

85e14a9

Add remaining tests and tidy

1303f47

Expand relevant Javadoc

e521926

Make error messages more consistent

9ccf4ed

vanniktech reviewed Sep 24, 2016

View reviewed changes

Peter Tackage added 4 commits September 24, 2016 22:19

Correct Exception naming

38bb3c0

Add test for Exception message to verify root cause

174ce7a

Add tests for alternative initialization path

aeafcc7

Simplify statement

786090d

akarnokd reviewed Sep 24, 2016

View reviewed changes

Use holder pattern for default Scheduler instances

c191e31

Peter Tackage added 2 commits September 25, 2016 09:58

Use correct scheduler when verifying reset

fff28b2

Make onInitHandler functions lazy and enforce non null.

a273507

peter-tackage mentioned this pull request Sep 25, 2016

Add unit tests for SearchViewModel futurice/freesound-android#55

Closed

akarnokd approved these changes Sep 25, 2016

View reviewed changes

akarnokd merged commit 62612ab into ReactiveX:2.x Sep 26, 2016

peter-tackage deleted the fix/lazy-init-schedulers branch September 26, 2016 07:51

akarnokd mentioned this pull request Oct 2, 2016

2.x: Defer Scheduler initialization in hook init API #4572

Closed

peter-tackage mentioned this pull request Oct 5, 2016

2.x: Add lazy loading of default main Scheduler ReactiveX/RxAndroid#338

Merged

akarnokd mentioned this pull request Oct 6, 2016

2.0.0-RC4 release preparations #4679

Closed

akarnokd mentioned this pull request Jan 29, 2019

I don't know why Schedulers class NewThreadTask implements Callable #6391

Closed

Conversation

peter-tackage commented Sep 22, 2016

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!

peter-tackage Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

codecov-io commented Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 78.17% (diff: 88.88%)

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!

peter-tackage commented Sep 24, 2016

Uh oh!

akarnokd commented Sep 25, 2016

Uh oh!

peter-tackage commented Sep 25, 2016

Uh oh!

JakeWharton commented Sep 25, 2016

Uh oh!

peter-tackage commented Sep 26, 2016

Uh oh!

akarnokd commented Sep 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

peter-tackage Sep 22, 2016 •

edited

Loading

codecov-io commented Sep 22, 2016 •

edited

Loading