Improving the ThreadManager and TokenRefresher APIs #77

hiranya911 · 2017-09-25T19:57:26Z

This is a follow up to #74. It attempts to solve several issues in the current design:

ThreadManager interface returns a ScheduledExecutorService. This makes implementing the ThreadManager API cumbersome, since the default implementation provided in Java (ScheduledThreadPoolExecutor) is not very amenable to configuration.
The different uses of the getExecutor() and getThreadManager() methods are not well defined in the ThreadManager interface. At the moment it is something along the lines of:
- getThreadManager(): for RTDB
- getExecutor(): for everything else
Proactive token refresher always get started when calling initializeApp() (which starts long-lived threads). But only RTDB requires it in practice. This is not ideal when using the SDK only for auth operations.

To resolve these issue, this PR makes the following changes:

Clearly define the contract of the ThreadManager interface.
- getThreadMaanger(): Used to start long-lived threads (including RTDB, token refresher and anything else that would require long-lived threads in the future)
- getExecutor(): Used for short-lived tasks.
Return an ExecutorService from ThreadManager, whose default implementation (ThreadPoolExecutor) is much easier to configure.
Invert the control of the proactive token refresher. Instead of always starting it from the app, let RTDB (or any other interested component) start it on demand.

The downside of this change is that we end up using a dedicated executor for proactive token refresher (we can use it for other scheduled tasks that we may add in the future -- but for now it's only used for token refresher). In GAE this will end up occupying a separate background thread. This shouldn't be a problem for those who are only using RTDB, but GAE users who access both RTDB and auth ends up spending another thread. However, it is possible to provide a custom ThreadManager that uses request-scoped threads for short-lived tasks, which is one way to avoid this drawback.

…read factories used by the SDK

… in default thread managers to ensure clean JVM exit

…s for clarity

…base.

…to documentation and tests.

…-java into hkj-exec-cleanup

hiranya911 · 2017-09-27T22:50:13Z

I did some testing on GAE for this PR. Auto and manual scaling works as before. Basic scaling also works, and idle instance shutdown can be easily enabled with a custom ThreadManager:

static class BasicScalingThreadManager extends ThreadManager { @Override protected ExecutorService getExecutor(@NonNull FirebaseApp firebaseApp) { // Use a single-threaded executor which keeps threads alive for 1 minute. return new ThreadPoolExecutor(0, 1, 60L, TimeUnit.SECONDS, new SynchronousQueue<Runnable>(), getThreadFactory()); } @Override protected void releaseExecutor(@NonNull FirebaseApp firebaseApp, @NonNull ExecutorService executorService) { executorService.shutdownNow(); } @Override protected ThreadFactory getThreadFactory() { return com.google.appengine.api.ThreadManager.backgroundThreadFactory(); } }

My test GAE instance also has a 1 minute idle timeout. With this setup, when the app doesn't receive requests for 2 minutes (1 minute for the threads to die, and 1 more minute for the GAE idle timeout to kick in), the instance is stopped.

15:36:57.769 GET 404 270 B 7.2 s Unknown /_ah/start GET 404 270 B 7.2 s Unknown 15:37:06.974 GET 200 2 B 71.6 s Unknown /_ah/background GET 200 2 B 71.6 s Unknown 15:37:16.326 GET 200 119 B 231 ms curl/7.53.1 /users?uid=jALVj6oEyvXmwLZuMcUCOeRWxhS2&count=1 GET 200 119 B 231 ms curl/7.53.1 15:37:17.414 GET 200 119 B 230 ms curl/7.53.1 /users?uid=jALVj6oEyvXmwLZuMcUCOeRWxhS2&count=1 GET 200 119 B 230 ms curl/7.53.1 15:37:18.383 GET 200 119 B 213 ms curl/7.53.1 /users?uid=jALVj6oEyvXmwLZuMcUCOeRWxhS2&count=1 GET 200 119 B 213 ms curl/7.53.1 15:39:19.068 GET 200 2 B 93 ms Unknown /_ah/stop GET 200 2 B 93 ms Unknown 15:39:57.372 GET 200 119 B 13.3 s curl/7.53.1 /users?uid=jALVj6oEyvXmwLZuMcUCOeRWxhS2&count=1 GET 200 119 B 13.3 s curl/7.53.1 15:39:57.382 GET 404 270 B 8.3 s Unknown /_ah/start GET 404 270 B 8.3 s Unknown 15:40:06.735 GET 200 2 B 64 s Unknown /_ah/background GET 200 2 B 64 s Unknown 15:42:11.231 GET 200 2 B 93 ms Unknown /_ah/stop GET 200 2 B 93 ms Unknown

googlebot · 2017-09-27T22:50:17Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

hiranya911 · 2017-09-28T23:11:44Z

I've been conducting a long running test with this PR on App Engine (manual scaling). The test is coming up on 24 hours now and seem to be working fine.

googlebot · 2017-09-28T23:11:47Z

CLAs look good, thanks!

… minor changes

schmidt-sebastian · 2017-10-04T19:21:53Z

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

 private final Map<String, FirebaseService> services = new HashMap<>();
- private final ListeningScheduledExecutorService executor;
+
+ private volatile ScheduledExecutorService scheduledExecutor;


It seems like 'ScheduledExecutorService' doesn't spawn its threads until it gets used for the first time. If you were to initialize it here, the code is this class could be drastically simplified.

This will initialize the ThreadFactory though (i.e ThreadManager.getThreadFactory() will get called). On App Engine this attempts to start a no-op background thread to see if the background thread support is available. Probably not a big deal, but just wanted to get your opinion on it before I make the change.

I went through great lengths to make sure that we don't kick off random threads in the first place, so we should probably keep this as is. Thanks!

schmidt-sebastian · 2017-10-04T19:24:28Z

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

 */
 static class TokenRefresher implements CredentialsChangedListener {

+ private static final int STATE_READY = 0;


Can we use an Enum here and an AtomicReference below?

It may even be fine to just use a volatile enum: http://www.javamex.com/tutorials/synchronization_volatile.shtml

Done. Used enum. Kept the atomic reference for compare-and-set method.

schmidt-sebastian · 2017-10-04T19:26:17Z

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

+ // If the access token is null, or is about to expire (i.e. expires in less than 5 minutes),
+ // schedule a refresh event with 0 delay. Otherwise schedule a refresh event at the token
+ // expiry time, minus 5 minutes.
+ scheduleRefresh(refreshDelay);


Subtract the 5 minutes here

This happens in the call to getRefreshDelay() a couple of lines earlier. I reorg'ed the code and comments a bit to make this clear.

If you substract the 5 minutes here, then getRefreshDelay() will return exactly the delay that is specified in the token and then "refreshDelay > 0" check will work for all delays.

schmidt-sebastian · 2017-10-04T19:26:32Z

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

- long refreshDelay = accessToken.getExpirationTime().getTime()
- - System.currentTimeMillis() - TimeUnit.MINUTES.toMillis(5);
+ long refreshDelay = getRefreshDelay(accessToken);
 if (refreshDelay > 0) {


This doesn't work for intervals <= 5 minutes.

That is correct. However, in practice this doesn't really happen. New tokens minted by Google auth have a 1 hour TTL. If it does happen (due to some issue in the remote token server), we don't want to schedule refresh events aggressively, and cause a feedback loop. So we simply log a warning and let things run course. This is what we have done in the past releases too.

schmidt-sebastian · 2017-10-04T19:28:13Z

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

 */
 @NonNull
- protected abstract ScheduledExecutorService getExecutor(@NonNull FirebaseApp app);
+ protected abstract ExecutorService getExecutor(@NonNull FirebaseApp app);


It's unclear to me when what the difference between and Executor and a FirebaseExecutor is.

FirebaseExecutor is just a holder for the ListeningExecutorService and the underlying ExecutorService returned by the user code. By keeping a reference to the original ExecutorService around we can make sure that the argument passed to releaseExecutor() is same as the one returned by getExecutor().

I don't know if we want to go down that road - but what if we required the user to provide a ListeningExecutorService in the first place? Would that be cleaner?

It would make things simpler for us. But we would be putting the onus of managing these references on the user. I would also prefer not to tie our API to a Guava type.

schmidt-sebastian · 2017-10-04T19:30:46Z

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

+ * original ExecutorService. This reference is used when it's time to release/cleanup the
+ * original ExecutorService.
+ */
+ static final class FirebaseExecutor {


This feels like it could be replaced by one line of code in the base class :)

The problem here is once we wrap the ExecutorService in a ListeningExecutorService, there's no way to reference the original executor again unless we explicitly store a reference to the original object. There's no way to get the delegate from the ListeningExecutorService.

On the other hand we want to make sure that the argument passed to user's releaseExecutor() is the same object returned by user's getExecutor(). So it looks like this is the easiest solution available. Or have I overlooked something obvious?

Can this class be renamed to "FirebaseExecutors", with two members: "userExecutor" & "listeningExecutor"?

Done. I renamed the existing FirebaseExecutors to FirebaseThreadManagers, and renamed this to FirebaseExecutors.

schmidt-sebastian · 2017-10-04T19:31:34Z

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


 @Override
- protected ScheduledExecutorService getExecutor(FirebaseApp app) {
+ protected synchronized ExecutorService getExecutor(FirebaseApp app) {


Don't lock on the main object (and don't lock twice).

Ah! Missed it during the merge with #74. Thanks for pointing that out.

schmidt-sebastian · 2017-10-04T19:31:43Z

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

 @Override
- protected void releaseExecutor(FirebaseApp app, ScheduledExecutorService executor) {
+ protected synchronized void releaseExecutor(FirebaseApp app, ExecutorService executor) {
 synchronized (lock) {


schmidt-sebastian · 2017-10-04T19:32:35Z

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

+ * background ThreadFactory with specific keep-alive times can easily facilitate GAE idle
+ * instance shutdown. Note that this often comes at the cost of losing scheduled tasks and RTDB
+ * support. Therefore, for these features, manual-scaling is the recommended GAE deployment mode
+ * regardless of the ThreadManager implementation used.


schmidt-sebastian

I haven't yet looked at the tests. Will do after.

…ool in default JVM

schmidt-sebastian · 2017-10-05T04:52:35Z

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

+ */
+ final synchronized void start() {
+ // Allow starting only from the ready state.
+ if (!state.compareAndSet(State.READY, State.STARTED)) {


Nit: This might not be needed since state is only ever changed under lock.

I'm also using compareAndSet() to ensure that we cannot start the refresher once it has been stopped.

schmidt-sebastian · 2017-10-05T05:04:47Z

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

+ scheduleCalls.incrementAndGet();
+ }
+ };
+ // stop() is allowed here, but since we didn't start(), no measurable state change


Nit: I know there are good reasons to keep this as a separate test case, but I would personally combine it with the previous test.

schmidt-sebastian

Please let me know what you think about my feedback. And then please ping me to get around my habit of not checking for GitHub review requests :/

Sorry for the long turn around.

hiranya911 · 2017-10-05T20:36:01Z

Made some of the suggested changes. Responded to all open comments. On to the next round.

hiranya911 added 11 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

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

a803a4e

…s for clarity

Clean separation of long-lived and short-lived tasks of the SDK

458d0c2

Updated documentation; More tests; Starting token refresher from data…

b63b17a

…base.

Updated documentation and log statements

050bfa4

Removing test file

f06b9b3

hiranya911 mentioned this pull request Sep 25, 2017

Introducing the ThreadManager API #74

Merged

hiranya911 added 4 commits September 25, 2017 17:38

Initializing executor in FirebaseApp constructor. Minor improvements …

846c93c

…to documentation and tests.

Merged with the latest ThreadManager impl

d3ea8e9

Merge branch 'hkj-exec-cleanup' of github.com:firebase/firebase-admin…

e06ae1d

…-java into hkj-exec-cleanup

Fixed token refresher stop() logic

dfca1e7

hiranya911 added 2 commits September 28, 2017 16:24

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

b310b0a

… minor changes

Merging with latest base

ae32b93

hiranya911 requested a review from schmidt-sebastian October 4, 2017 19:12

hiranya911 assigned schmidt-sebastian Oct 4, 2017

schmidt-sebastian reviewed Oct 4, 2017

View reviewed changes

schmidt-sebastian suggested changes Oct 4, 2017

View reviewed changes

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

Cleaning up the TokenRefresher state machine; Using a cached thread p…

fc009ff

…ool in default JVM

hiranya911 changed the base branch from hkj-thread-mgt to m19 October 4, 2017 23:12

hiranya911 changed the base branch from m19 to hkj-thread-mgt October 4, 2017 23:12

Fixing some merge conflicts

a151749

hiranya911 changed the base branch from hkj-thread-mgt to m19 October 4, 2017 23:32

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

schmidt-sebastian reviewed Oct 5, 2017

View reviewed changes

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

Code clean up

65ef119

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

schmidt-sebastian approved these changes Oct 5, 2017

View reviewed changes

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

hiranya911 merged commit 122595a into m19 Oct 5, 2017

hiranya911 deleted the hkj-exec-cleanup branch October 5, 2017 21:58

Improving the ThreadManager and TokenRefresher APIs #77

Improving the ThreadManager and TokenRefresher APIs #77

Uh oh!

Conversation

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

Uh oh!

hiranya911 commented Sep 27, 2017

googlebot commented Sep 27, 2017

hiranya911 commented Sep 28, 2017

googlebot commented Sep 28, 2017

Choose a reason for hiding this comment

hiranya911 Oct 4, 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

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

schmidt-sebastian left a 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

schmidt-sebastian left a comment

Choose a reason for hiding this comment

hiranya911 commented Oct 5, 2017

Labels

3 participants

hiranya911 commented Sep 25, 2017 •

edited

Loading

hiranya911 Oct 4, 2017 •

edited

Loading