Skip to content

Conversation

@hiranya911
Copy link
Contributor

No description provided.

List<AuthStateListener> listenersCopy = null;
if (!newToken.equals(oldToken)) {
long refreshDelay = googleOAuthToken.getExpiryTime() - clock.now()
- TimeUnit.MINUTES.toMillis(5);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this code below to where it's actually used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (currentToken.compareAndSet(oldToken, newToken)) {
listenersCopy = ImmutableList.copyOf(authStateListeners);
tokenRefresher.scheduleRefresh(TOKEN_REFRESH_INTERVAL_MILLIS);
if (refreshDelay > 0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<= 0 would likely indicate a bug, right? Should we at least log a message? If we were confident it should never happen, I'd throw an Exception... but I guess a developer-provided credentials class could generate tokens that expire in less than 5 minutes or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a log statement

@hiranya911
Copy link
Contributor Author

Made the suggested changes.

tokenRefresher.scheduleRefresh(refreshDelay);
} else {
Log.w("FirebaseApp", "Token validity period is less than 5 "
+ "minutes. Not scheduling a proactive refresh event.");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, if we see this log message, something is probably broken and having as much information as possible will help figure it out sooner. So I would do something like:

Log.w("FirebaseApp", "Token expiry (" + googleOAuthToken.getExpiryTime() + ") is less than 5 minutes in the future. Not scheduling a proactive refresh.");

This would probably be enough to determine if: 1) the expiration time is in the past for some reason, 2) the expiration time is super short for some reason, 3) the expiration time is fine so maybe the local clock is wrong.

Not a big deal, but consider tweaking before merging...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Updated the log statement as suggested.

@hiranya911 hiranya911 assigned hiranya911 and unassigned mikelehen Apr 28, 2017
@hiranya911 hiranya911 merged this pull request into io2017 Apr 28, 2017
@hiranya911 hiranya911 deleted the hkj-refresh-interval branch April 28, 2017 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants