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

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

Closed
JakeWharton opened this issue Sep 21, 2016 · 8 comments
Closed

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

JakeWharton opened this issue Sep 21, 2016 · 8 comments

Comments

@JakeWharton
Copy link
Contributor

An issue was filed on RxAndroid asking for the default "main thread" scheduler to be lazily initialized. This would allow support for unit testing on the JVM by swapping out the scheduler for one that works where Android's main thread doesn't exist.

The hook API provides a means of replacing the scheduler during initialization, but the problem is that in order to call these "init" methods the instance needs to be eagerly created.

What I'm proposing is that we change the "init" functions from Function<Scheduler, Scheduler> to Function<Scheduler, Callable<Scheduler>> which would allow ignoring the default scheduler instance by never delegating to Callable.call() to create it.

The implementation of the initializer would change to something like:

Scheduler initFoo(Callable<Scheduler> defaultScheduler);
Function f = initFunc;
if (f == null) {
  return defaultScheduler.call();
}
return apply(f, defaultScheduler);

Now we can make this change only in RxAndroid to support this case. I think that there's a large value in having as much symmetry between the two libraries as possible, however.

Is this something that would be acceptable to change in RxJava? Or are there any alternative APIs that anyone can think of for this?

@akarnokd
Copy link
Member

I don't think RxJava needs this change. We have standard Java ExecutorServices backing the Schedulers so their creation should always succeed in a reasonable JDK implementation.

Maybe you could combine AndroidSchedulers the plugin and use a holder class to defer the creation of the looper/handler if the function happens to be null the first time main is invoked. I can see this needs to change the callback to Callable and not rely on the default anymore.

class AndroidSchedulers {
    static final class DefaultHolder {
        static final Scheduler DEFAULT_MAIN = new HandlerScheduler(
            new Handler(Looper.getMainLooper())));
    }

    static volatile Callable<Scheduler> mainHook;

    public static Scheduler main() {
        Function f = mainHook;
        if (f == null) {
            return DEFAULT_HOLDER.DEFAULT_MAIN; // initialized once and lazily
        }
        try {
            return mainHook.call();
        } catch (Throwable ex) {
            throw ExceptionHelper.wrapOrThrow(ex);
        }
    }
}

@JakeWharton
Copy link
Contributor Author

That doesn't allow decorating the default scheduler which is a common use
of these init hooks on Android.

I know RxJava doesn't need this, what I'm asking is if the change would be
acceptable for symmetry. As an added bonus: it means you no longer have to
remember to call .shutdown() on the original scheduler when returning a
different one (which I bet 100% of people don't do and we don't check for).

On Wed, Sep 21, 2016, 3:40 AM David Karnok notifications@github.com wrote:

I don't think RxJava needs this change. We have standard Java
ExecutorServices backing the Schedulers so their creation should always
succeed in a reasonable JDK implementation.

Maybe you could combine AndroidSchedulers the plugin and use a holder
class to defer the creation of the looper/handler if the function happens
to be null the first time main is invoked. I can see this needs to change
the callback to Callable and not rely on the default anymore.

class AndroidSchedulers {
static final class DefaultHolder {
static final Scheduler DEFAULT_MAIN = new HandlerScheduler(
new Handler(Looper.getMainLooper())));
}

static volatile Callable<Scheduler> mainHook;

public static Scheduler main() {
    Function f = mainHook;
    if (f == null) {
        return DEFAULT_HOLDER.DEFAULT_MAIN; // initialized once and lazily
    }
    try {
        return mainHook.call();
    } catch (Throwable ex) {
        throw ExceptionHelper.wrapOrThrow(ex);
    }
}

}


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4572 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEEEaPxo9olTdBE7-0fk5UDkdvE-tvLks5qsN9XgaJpZM4KCUav
.

@akarnokd
Copy link
Member

That doesn't allow decorating the default scheduler which is a common use
of these init hooks on Android.

The OP wanted to avoid depending on Handler completely so he could test on desktop. The only workaround I see is a boolean flag that when set, invokes the Function with null instead of loading the default Android scheduler.

class AndroidSchedulers {
    static final class DefaultHolder {
        static final Scheduler DEFAULT_MAIN = new HandlerScheduler(
            new Handler(Looper.getMainLooper())));
    }

    static volatile boolean noDefault;
    static volatile Function<Scheduler, Scheduler> mainHook;

    public static Scheduler main() {
        Function f = mainHook;
        if (f == null) {
            return DEFAULT_HOLDER.DEFAULT_MAIN; // initialized once and lazily
        }
        try {
            if (noDefault) {
                return f.apply(null);
            }
            return f.apply(DEFAULT_HOLDER.DEFAULT_MAIN);
        } catch (Throwable ex) {
            throw ExceptionHelper.wrapOrThrow(ex);
        }
    }
}

@JakeWharton
Copy link
Contributor Author

My original proposal passes a Callable<Scheduler> to the function. That is what I'm proposing.

@akarnokd
Copy link
Member

Right, sounds reasonable now. How about a PR?

@peter-tackage
Copy link
Contributor

@akarnokd I'm the said OP. Happy to put together a PR for the change.

@akarnokd
Copy link
Member

Sure.

@akarnokd
Copy link
Member

akarnokd commented Oct 2, 2016

Closing via #4585

@akarnokd akarnokd closed this as completed Oct 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants