Introducing the ThreadManager API #74

hiranya911 · 2017-09-18T23:18:16Z

Adding the new ThreadManager interface which controls how an app obtains its thread pools and thread factories. Developers can specify their own implementations of this interface via FirebaseOptions. We provide 2 implementations (one for basic JVM and one for GAE), one of which will be used by default.

This PR also ensures:

Calling app.delete() terminates any and all threads started by the SDK.
When running in a regular JVM (non-GAE), we start all threads as daemons. This ensures thread clean up even if the user ignores to call app.delete().
Database uses the ThreadFactory provided by ThreadManager to start all of its threads (event target, run loop, socket reader/writer etc.)
The SDK doesn't use TaskExecutors for any thing. However, we continue to keep it in the code base for backward compatibility reasons.

…read factories used by the SDK

… in default thread managers to ensure clean JVM exit

hiranya911 · 2017-09-18T23:19:00Z

Related to #55 and #73

…s for clarity

hiranya911 · 2017-09-23T00:35:16Z

I've been running some tests with this PR on App Engine, and I'll document my observations here.

Manual Scaling

Works as usual. All async tasks (from auth, database and everything else) runs normally with the default ThreadManager chosen by the SDK. This is the preferred mode of deployment for accessing RTDB from App Engine environment.

Automatic Scaling

Works as usual. All async tasks, except for the ones started by database, runs normally with the default ThreadManager chosen by the SDK. The database code requires long-lived threads, and hence cannot be used. The SDK will throw exceptions if attempted. But other components like auth can be used normally.

Basic Scaling

This is similar to manual scaling. So everything, including database, will work as usual. However, the long-lived threads started by the database, and the proactive token refresher will prevent the instances from shutting down due to instance idle time (unless FirebaseApp.delete() is called). If this is important (it usually is when using basic-scaling), one must ensure that:

No database calls are made in the app, and
Proactive token refresher is disabled.

We will provide an API to achieve item 2 in the future. In the meantime, one can use a custom executor (specified via ThreadManager). There are many ways to do this, but I chose to do the following:

class MyExecutor extends ScheduledThreadPoolExecutor { public MyExecutor(ThreadFactory factory) { super(0, factory); // Following is to make the test more interesting. Not required. setKeepAliveTime(60L, TimeUnit.SECONDS); } public <V> ScheduledFuture<V> schedule(Callable<V> command, long delay, TimeUnit unit) { if (delay > 0) { throw new UnsupportedOperationException(); } return super.schedule(command, delay, unit); } public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) { if (delay > 0) { throw new UnsupportedOperationException(); } return super.schedule(command, delay, unit); } }

This essentially rejects all "scheduled" tasks, and only admits tasks meant for immediate execution (i.e. delay == 0). This transparently fails the proactive token refresher, but executes everything else normally.

I had my basic-scaled instance set to shutdown after 1 minute of idle time:

<basic-scaling> <max-instances>1</max-instances> <idle-timeout>1m</idle-timeout> </basic-scaling>

With the above custom executor, I observed that my instance is shutting down after 2 minutes of idle time (1 extra minute due to the 60s keep alive time set on the executor). So overall it was a successful test. I had similar results with both background and request-scoped threads on basic-scaling.

Once we provide a means to explicitly turn off proactive token refreshing, the custom executor implementation can be made as simple as new ScheduledThreadPoolExecutor(0, threadFactory).

schmidt-sebastian · 2017-09-25T19:34:42Z

src/main/java/com/google/firebase/FirebaseApp.java

 checkState(!deleted.get(), "FirebaseApp was deleted %s", this);
 }

+ private ListeningScheduledExecutorService getExecutorService() {


How often is this not called? Can we initialize the executor in the constructor?

Done. In performance-sensitive environments like GAE we lazy load the underlying executor implementation itself. So it's safe to always initialize the high-level executor in the FirebaseApp constructor.

schmidt-sebastian · 2017-09-25T19:34:52Z

src/main/java/com/google/firebase/FirebaseApp.java


 private final AtomicBoolean deleted = new AtomicBoolean();
 private final Map<String, FirebaseService> services = new HashMap<>();
+ private final AtomicReference<ListeningScheduledExecutorService> executorReference =


Since this is only ever accessed under lock, does it need to be an AtomicReference?

schmidt-sebastian · 2017-09-25T19:36:40Z

src/main/java/com/google/firebase/FirebaseApp.java

+ ScheduledExecutorService executor = executorReference.get();
+ if (executor != null) {
+ threadManager.releaseExecutor(this, executor);
+ executorReference.set(null);


I think it would be cleaner to call checkNotDeleted() in the schedule methods below and optionally not clean up this reference.

Now that the AtomicReference is gone, this is a lot simpler.

schmidt-sebastian · 2017-09-25T19:41:48Z

src/main/java/com/google/firebase/FirebaseOptions.java

 }

- /** 
+ @NonNull


Should this be public to follow the other getters more closely?

We already have getCredentials() which is package-protected. So there is some precedent here. I feel more comfortable keeping this hidden from other packages and also user code.

schmidt-sebastian · 2017-09-25T19:46:20Z

src/main/java/com/google/firebase/ThreadManager.java

+ /**
+ * Returns the main thread pool for an app. Implementations may return the same instance of
+ * <code>ScheduledExecutorService</code> for multiple apps. The returned thread pool is used by
+ * all components of an app except for the Realtime Database. Database has far stricter and


This is really hard to reason about. Would it be possible to only ever return a Thread Factory?

I agree. Semantics of this API are not well defined at the moment. I'm putting together #77 to address this issue. Once we do those changes we will have a clear separation:

getExecutor() : Returns a thread pool for short-lived tasks

getThreadFactory(): Returns a ThreadFactory for long-lived tasks (includes RTDB and other scheduled tasks)

To answer you specific question, now that we initialize the executor in FirebaseApp constructor, it is no longer possible to get away with returning null from getExecutor().

schmidt-sebastian · 2017-09-25T23:00:18Z

src/main/java/com/google/firebase/auth/internal/BaseCredential.java

+  accessToken.getExpirationTime().getTime());
+ return Tasks.forResult(googleToken);
+ } catch (IOException e) {
+ throw new RuntimeException(e);


Does the Task library not have its own way of passing along errors?

Done. On a related note both Task and FirebaseCredential are now deprecated, and are on their way out.

schmidt-sebastian · 2017-09-25T23:02:58Z

src/main/java/com/google/firebase/database/tubesock/ThreadConfig.java

+ this.threadFactory = checkNotNull(threadFactory);
+ }
+
+ ThreadInitializer getInitializer() {


It is somewhat hard to reason about (from the name alone) what the difference between a ThreadInitializer and a ThreadFactory is. Can you add comments (or rename the new ThreadInitializer)?

schmidt-sebastian · 2017-09-25T23:04:51Z

src/main/java/com/google/firebase/database/tubesock/WebSocket.java

 public class WebSocket {

+ private static final ThreadConfig DEFAULT_THREAD_CONFIG = new ThreadConfig(
+ ThreadInitializer.defaultInstance, Executors.defaultThreadFactory());


Should this get the thread initializer from Platform.java?

This constant is not used during normal execution. It is only used in the hidden Websocket(URL url) constructor, which is only called by some internal test classes we have implemented (a set of executable main methods!). So I believe this is ok.

We can discuss later whether we need this at all. I think this and the test client code that use it can all be removed.

schmidt-sebastian · 2017-09-25T23:12:08Z

src/test/java/com/google/firebase/ThreadManagerTest.java

+ assertEquals(Event.TYPE_RELEASE_EXECUTOR, event.type);
+ assertEquals(app, event.arguments[0]);
+ }
+


Can you add tests that exercise the "setup-teardown-setup-teardown" logic you implemented?

I think that's covered in the FirebaseExecutorsTest. Let me know if that's not what you were referring to.

schmidt-sebastian · 2017-09-25T23:13:23Z

src/test/java/com/google/firebase/ThreadManagerTest.java

+ private final int type;
+ private final Object[] arguments;
+
+ public Event(int type, Object... arguments) {


I would prefer if you explicitly called out the arguments.

…to documentation and tests.

hiranya911 · 2017-09-26T00:47:22Z

Made most of the suggested changes. A few more changes are on their way in #77. In particular some of the complexities you pointed out in the semantics of ThreadManager will get improved for the better.

schmidt-sebastian · 2017-09-28T21:59:13Z

src/main/java/com/google/firebase/ThreadManager.java

 * Returns the <code>ThreadFactory</code> to be used for creating any additional threads
- * required by the SDK. This is used mainly to create the run loop, event target and web socket
- * reader/writer threads of the Realtime Database client.
+ * required by the SDK. This is used mainly to create the long-lived worker threads for


missing "the"

schmidt-sebastian · 2017-09-28T22:02:50Z

src/main/java/com/google/firebase/database/tubesock/ThreadConfig.java


+ /**
+ * The {@link ThreadInitializer} implementation to be used for further configuring already
+ * created threads. In particular, this instance is used to rename the created threads, and turn


This suggests that it would work with threads that were created way back in the days. This class is used to configure newly created threads, while the factory is used to create new threads.

Updated the text.

schmidt-sebastian · 2017-09-28T22:18:55Z

src/main/java/com/google/firebase/ImplFirebaseTrampolines.java

+ return app.getThreadFactory();
+ }
+
+ public static <T> Task<T> submit(@NonNull FirebaseApp app, @NonNull Callable<T> command) {


submitCallable maybe? 'submit' seems generic in the context of this class.

schmidt-sebastian · 2017-09-28T22:20:25Z

src/main/java/com/google/firebase/ThreadManager.java

+ * @return A non-null {@link ScheduledExecutorService} instance.
+ */
+ @NonNull
+ protected abstract ScheduledExecutorService getExecutor(@NonNull FirebaseApp app);


Could we combine the two methods (this and above) and always return a ListeningScheduledExecutorService?

I would prefer to keep ThreadManager API decoupled from any Guava types. That way developers don't have to learn about Guava APIs to extend this API.

schmidt-sebastian · 2017-09-28T22:22:09Z

src/main/java/com/google/firebase/auth/internal/BaseCredential.java

+ GoogleOAuthAccessToken googleToken = new GoogleOAuthAccessToken(accessToken.getTokenValue(),
+  accessToken.getExpirationTime().getTime());
+ return Tasks.forResult(googleToken);
+ } catch (IOException e) {


Just Exception?

schmidt-sebastian · 2017-09-28T22:32:07Z

src/main/java/com/google/firebase/database/tubesock/ThreadConfig.java

+ private final ThreadInitializer threadInitializer;
+ private final ThreadFactory threadFactory;
+
+ public ThreadConfig(ThreadInitializer threadInitializer, ThreadFactory threadFactory) {


Nit: Swap the order here. First thread get 'factoried' and then initialized.

schmidt-sebastian · 2017-09-28T22:52:28Z

src/main/java/com/google/firebase/internal/FirebaseExecutors.java

+ private ScheduledExecutorService executorService;
+
+ @Override
+ protected synchronized ScheduledExecutorService getExecutor(FirebaseApp app) {


I would suggest not to synchronize on the public facing object (since this can also be done by consumers). Can you use a private member (like executorService)?

schmidt-sebastian · 2017-09-28T22:54:54Z

src/main/java/com/google/firebase/internal/FirebaseExecutors.java

+ .setDaemon(true)
+ .setThreadFactory(getThreadFactory())
+ .build();
+ logger.debug("Initializing default executor with {} max threads", cores);


I don't believe there is any guarantee that ScheduledThreadPoolExecutor will try to schedule each thread on a dedicated core, so using this number here may not make much sense.

Correct. I just used that as a max limit since ScheduledThreadPoolExecutor must be given some core thread pool count. I think we can go back to cached thread pool when we implement #77.

… minor changes

hiranya911 · 2017-09-28T23:26:47Z

Made the suggested changes. Over to @schmidt-sebastian for another round.

schmidt-sebastian

Thanks!

hiranya911 added 6 commits September 16, 2017 13:57

Implemented ThreadManager API for configuring the thread pools and th…

a2ee38c

…read factories used by the SDK

Giving all threads unique names; Updated documentation; Using daemons…

1cf4c3a

… in default thread managers to ensure clean JVM exit

Updated comments and documentation

8234ca1

Adding tests for options

a8fb84e

Test cases for basic ThreadManager API

913fda5

More test cases

c218f82

hiranya911 requested a review from mikelehen September 18, 2017 23:19

hiranya911 assigned mikelehen Sep 18, 2017

hiranya911 requested a review from schmidt-sebastian September 20, 2017 22:57

hiranya911 assigned schmidt-sebastian and unassigned mikelehen Sep 20, 2017

Made the executor service private in FirebaseApp; Refactored the test…

a803a4e

…s for clarity

hiranya911 mentioned this pull request Sep 25, 2017

Improving the ThreadManager and TokenRefresher APIs #77

Merged

schmidt-sebastian suggested changes Sep 25, 2017

View reviewed changes

Initializing executor in FirebaseApp constructor. Minor improvements …

846c93c

…to documentation and tests.

schmidt-sebastian reviewed Sep 28, 2017

View reviewed changes

schmidt-sebastian removed their assignment Sep 28, 2017

Updated documentation; Renamed submit() to submitCallable() and other…

b310b0a

… minor changes

hiranya911 assigned schmidt-sebastian Sep 28, 2017

schmidt-sebastian approved these changes Oct 4, 2017

View reviewed changes

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Oct 4, 2017

hiranya911 merged commit 8dadce6 into m19 Oct 4, 2017

hiranya911 deleted the hkj-thread-mgt branch October 6, 2017 17:32

   }
-  /**
+  @NonNull

Introducing the ThreadManager API #74

Introducing the ThreadManager API #74

Uh oh!

Conversation

hiranya911 commented Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

hiranya911 commented Sep 18, 2017

hiranya911 commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Manual Scaling

Automatic Scaling

Basic Scaling

Choose a reason for hiding this comment

hiranya911 Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 commented Sep 28, 2017

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Labels

3 participants

hiranya911 commented Sep 18, 2017 •

edited

Loading

hiranya911 commented Sep 23, 2017 •

edited

Loading

hiranya911 Sep 25, 2017 •

edited

Loading

hiranya911 Sep 25, 2017 •

edited

Loading

hiranya911 Sep 25, 2017 •

edited

Loading

hiranya911 Sep 25, 2017 •

edited

Loading