Simplifying FirebaseInstallations class by adding listeners. #847

ankitaj224 · 2019-09-25T20:22:14Z

No description provided.

ciarand · 2019-09-25T20:37:45Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+        boolean doneListening = l.onStateReached(persistedFidEntry);
+        if (doneListening) {
+          it.remove();
+          break;


I'm not sure whether we should keep the break or not. If we don't, listeners will finish earlier than they would normally which seems reasonable to me. WDYT?

I dint think of the break; until my testGetAuthToken_multipleCallsForceRefresh_fetchedNewTokenTwice() test failed.

Reason: Multiple getAuthToken(FOREC_REFRESH) calls - if first call is completed it triggers onStateReached for both calls that are in the list .

GetAuthTokenListener onStateReached condition will pass (Registered & Auth Toke is valid) and remove both listeners from the list.

However, the second call execution continues, generates new auth token and updates the shared pref with new values - resulting in discrepancy in what we report and we persist in FIS DB & local storage.

ignoring what the code actually does atm, what's the expected behavior of calling getAuthToken(FORCE_REFRESH) twice? Are we expecting the first Task to be completed with a soon-to-be invalid auth token and the second task will be completed with the newly-valid token? Or do we want both tasks to be completed with the same auth token?

AFIK we are expected to return the first Task on completion with a soon-to-be invalid auth token and the second task will be completed with the newly-valid token. I verified this flow with Di and yet to verify it with Rayo.

IMO it would be good to have both tasks be completed with the same auth token. But wont we complicate the logic ( like Force Refresh behaves differently when one call is made vs multiple calls, or when multiple calls with do-not-force-refresh and force-refresh options are called).

Your thoughts.

If that's the expected and desired outcome then I guess this is fine. I think it's a little error prone, but that's a product decision for your team and not a technical one that should be resolved here.

I think it would be clearer if we completed both tasks with the same result where possible, but to do that we'd have to keep the current auth token in memory and read it as part of our listener creation routine. I don't think that's prohibitively complicated (and it means we could simplify elsewhere by removing the break here and the conditional below), but that's a decision for your team.

This is one corner case. I am yet to discuss and decide the outcome with the team.

However, I tried incorporating your suggestion. Can you PTAL if the changes are correct now.

firebase-installations/src/main/java/com/google/firebase/installations/StateListener.java

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

…Token() calls are triggered simultaneously.

ciarand · 2019-09-26T22:27:29Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

+
+    // Validate that registration is complete with updated auth token
+    PersistedFidEntry updatedFidEntry = persistedFid.readPersistedFidEntryValue();
+    assertThat(updatedFidEntry).isEqualTo(UPDATED_AUTH_TOKEN_FID_ENTRY);


this assertion (assertThat(updatedFidEntry).isEqualTo(UPDATED_AUTH_TOKEN_FID_ENTRY)) comes out of nowhere for me. Why are we expecting it to be the UPDATED_AUTH_TOKEN_FID_ENTRY? Could we somehow make that link more explicit in the code? Reminds me of the second negative example in http://go/tott/529 - this is a lil too magical

Updated it to assert on the updated auth token value. Please review to see if it makes sense now. Else I ll try to be more descriptive.

ciarand · 2019-09-27T19:43:33Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+    String oldAuthToken =
+        persistedFid.readPersistedFidEntryValue().getAuthToken() == null
+            ? ""
+            : persistedFid.readPersistedFidEntryValue().getAuthToken();


you've got the right idea, but we can't do this file I/O here because it could be called from any thread (we haven't hopped onto the executor yet).

How about instead of doing the file I/O here we maintain two int fields; the "current revision" and the "desired revision".

On first boot both IDs are 0 (or 1, whatever)

Whenever getAuthToken(FORCE_REFRESH) is called we make sure that desired revision is current revision + 1

Whenever we hit the "should we refresh our auth token?" conditional in the runnable body we just have to check whether current revision == desired revision. If they're the same, we don't have to refresh. If they're different, we need to refresh and then set them ==.

this doesn't even have to be ints, it could probably just be a boolean ("shouldRefresh"?) that's accessed in the same synchronized block as the listeners additions

About the file I/O : shouldn't it be okay even if its called by any thread? As long as our onStateReached conditional can handle it correctly. I tested it with my test cases and it seemed to work fine. However, you are right about #847 (comment) .

I tried implementing your suggestion based on my understanding of the comment. Also, I have tested the changes. I will be honest, I dint understand this part

("shouldRefresh"?) that's accessed in the same synchronized block as the listeners additions

please let me know wdyt.

ciarand · 2019-09-27T20:11:21Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+        // 1. Invalid auth token
+        // 2. If FORCE_REFRESH with a pending listener
+        synchronized (lock) {
+          if ((authTokenOption == FORCE_REFRESH && !listeners.isEmpty())


we can't hold the synchronized lock while doing network I/O because it'll block arbitrary threads that are calling getId / getAuthToken

ciarand · 2019-09-27T20:12:29Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+        // 1. Invalid auth token
+        // 2. If FORCE_REFRESH with a pending listener
+        synchronized (lock) {
+          if ((authTokenOption == FORCE_REFRESH && !listeners.isEmpty())


I get why we have this !listeners.isEmpty() check, but it feels like it's a symptom of what we're trying to check (is anyone still waiting for a state change?) instead of the actual thing we want to check (whether our current auth token needs to be refreshed)

Makes sense. Updated. PTAL. Thanks

ciarand

tried to demo suggestions in code: https://github.com/firebase/firebase-android-sdk/pull/860/files

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

…into listeners

ankitaj224 · 2019-09-30T20:14:03Z

tried to demo suggestions in code: https://github.com/firebase/firebase-android-sdk/pull/860/files

Thanks for the demo. I picked up few suggestions from your demo.

ciarand · 2019-09-30T20:56:42Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

    when(mockUtils.createRandomFid()).thenReturn(TEST_FID_1);
-    when(mockClock.currentTimeMillis()).thenReturn(TEST_CREATION_TIMESTAMP_1);
+    when(mockUtils.currentTimeInSecs()).thenReturn(TEST_CREATION_TIMESTAMP_2);
+    when(mockUtils.isAuthTokenExpired(any())).thenReturn(false);


I think the tests would be clearer if we clearly scoped these isAuthTokenExpired stubs to a specific auth token. So instead of just accepting any(), we could say "this auth token is expired but this one is not"

Updated where appropriate for tests to be clear.

ciarand · 2019-09-30T21:03:50Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

+
+  @Test
+  public void testGetId_expiredAuthToken_refreshesAuthToken() throws Exception {
+    when(mockUtils.isAuthTokenExpired(any())).thenReturn(true);


why does this happen first thing in this test but like 3rd in the previous test?

No specific reason. Does the order matter?

ankitaj224 · 2019-09-30T21:42:26Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

                (unused) ->
                    InstallationTokenResult.builder()
                        .setToken(TEST_AUTH_TOKEN_4)
                        .setTokenExpirationInSecs(TEST_TOKEN_EXPIRATION_TIMESTAMP)
                        .build()))
        .when(backendClientReturnsOk)
        .generateAuthToken(anyString(), anyString(), anyString(), anyString());
+    when(mockUtils.isAuthTokenExpired(any())).thenReturn(false);


I skipped updating any() to a specific FID entry for mockUtils.isAuthTokenExpired() only here. This was to hide the details that don't improve the test. Hope that's fine.

that's fine, I think the rest of the cases have benefited from the change

ciarand · 2019-09-30T23:39:09Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

-      // Waiting for Task that registers FID on the FIS Servers
-      listener.await(AWAIT_TIMEOUT_IN_SECS, TimeUnit.SECONDS);
-      return supplier.get();
+  private final Runnable doRegistration() {


I wouldn't bother wrapping this in a method like this. Either inline it at the field declaration site (Runnable doRegistration = () -> { /* code here */ } or make doRegistration the actual runnable and just chuck a method reference on the executor (execute(this::doRegistration)).

Done. PTAL.

ciarand · 2019-09-30T23:44:56Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+          persistedFidEntry = persistedFid.readPersistedFidEntryValue();
+        }
+
+        // Notify the listeners only after force refreshing auth token if shouldRefreshAuthToken is


trouble is that this means the getId listeners won't be notified until registration happens, which is something we wanted to avoid.

The initial draft version didn't run into this because it called triggerOnStateReached(NOT_REGISTERED) instead of triggerOnStateReached(persistedFidEntry) which meant the listeners were simpler because each step of the state machine always gets called. Could we change it so that it's calling triggerOnStateReached(newState, persistedFidEntry) in order to get the step-by-step simplification benefits of the state machine?

shouldRefreshAuthToken will be false by default for getId. Hence, getId listener will be notified before registration happens. (Matches the initial draft version).

It is a matter of doing the checks here vs doing the checks in listeners.

With triggerOnStateReached(newState, persistedFidEntry) we have to change the definition of both the listeners (because interface changes). GetIdListener doesn't need newState.

ciarand · 2019-09-30T23:46:39Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

+          synchronized (lock) {
+            shouldRefreshAuthToken = false;
+          }
+          fetchAuthTokenFromServer(persistedFidEntry);


why do the fetch after setting shouldRefreshAuthToken to false instead of in reverse?

ciarand · 2019-10-08T18:19:54Z

...ndroidTest/java/com/google/firebase/installations/FirebaseInstallationsInstrumentedTest.java

                (unused) ->
                    InstallationTokenResult.builder()
                        .setToken(TEST_AUTH_TOKEN_4)
                        .setTokenExpirationInSecs(TEST_TOKEN_EXPIRATION_TIMESTAMP)
                        .build()))
        .when(backendClientReturnsOk)
        .generateAuthToken(anyString(), anyString(), anyString(), anyString());
+    when(mockUtils.isAuthTokenExpired(any())).thenReturn(false);


that's fine, I think the rest of the cases have benefited from the change

firebase-installations/src/main/java/com/google/firebase/installations/Utils.java

ciarand · 2019-10-08T18:43:05Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

-      return supplier.get();
-    };
-  }
+      // Always notify the GetIdListeners. For GetAuthTokenListeners, only notify if force


I am concerned that now a getId call won't complete as early as it could if there's a pending force-refresh call. Is that something we should be worried about?

We need not be worried. getId call will never be called with force-refresh.

Thanks for clarifying offline. I understand your concern now. I referred to your demo, made some modifications and updated the code. PTAL and let me wdyt.

Thank you.

ciarand

I think the onStateReached part might be a lil clearer using the state enum instead of a boolean, but this lgtm

* Fixing FISClient to correctly parse expiration timestamp.

* Updating getAuthToken to return creation timestamp along with auth token and expiration timestamp.

* Propagating the exceptions to the clients. * Adding javadocs for StateListener class.

ankitaj224 · 2019-10-10T20:37:19Z

/test check-changed

* Implemneting retry for FIS Client. * Simplifying FirebaseInstallations class by adding listeners. (#847) * Simplifying FirebaseInstallations class by adding listeners. * Addressing ciaran's comments to return same token if multiple getAuthToken() calls are triggered simultaneously. * Cleaning doRegistration method. * Fixing FISClient to correctly parse expiration timestamp. (#848) * Updating getAuthToken to return creation timestamp (#884) * Propagating the exceptions to the clients. (#856) * Addressing ciaran's comments. * Merge conflicts. * Nit fixes * Fixing verify format issue.

* Fixing FISClient to correctly parse expiration timestamp. (#848) * Updating getAuthToken to return creation timestamp (#884) * Propagating the exceptions to the clients. (#856) * Extract FID from FIS createInstallation response (#888) * Addressing Rayo's comment: Rename PersistedFidEntry to (#899) * Implementing retry once for FIS Client. (#895) * Simplifying FirebaseInstallations class by adding listeners. (#847) * Fixing FISClient to correctly parse expiration timestamp. (#848) * Updating getAuthToken to return creation timestamp (#884) * Propagating the exceptions to the clients. (#856)

Simplifying FirebaseInstallations class by adding listeners.

49708d2

ankitaj224 requested a review from ciarand September 25, 2019 20:22

googlebot added the cla: yes Override cla label Sep 25, 2019

google-oss-bot added the size/XL label Sep 25, 2019

ciarand reviewed Sep 25, 2019

View reviewed changes

Splitting multiple classes/interfaces into separate files.

a987647

ankitaj224 requested a review from ciarand September 25, 2019 21:28

Addressing ciaran's comments to return same token if multiple getAuth…

1d9b1ca

…Token() calls are triggered simultaneously.

ciarand reviewed Sep 27, 2019

View reviewed changes

ankitaj224 added 2 commits September 27, 2019 14:38

Addressing ciaran's comments to use a boolean to refresh auth token

28d0d85

Cleaning up instrumented test file.

126e372

ankitaj224 requested a review from ciarand September 27, 2019 22:06

ciarand suggested changes Sep 27, 2019

View reviewed changes

ankitaj224 added 3 commits September 29, 2019 00:10

Ciaran's comments on synchronization and runnable.

c39cdb8

Ciaran's comments on synchronization and runnable.

c21a59e

Merge branch 'listeners' of github.com:firebase/firebase-android-sdk …

7a46ab0

…into listeners

ankitaj224 requested a review from ciarand September 29, 2019 07:14

Fixing precheck failures.

314b3bb

ciarand reviewed Sep 30, 2019

View reviewed changes

Addressing Ciaran's comment on mock utils.

715d427

ankitaj224 requested a review from ciarand September 30, 2019 21:39

ankitaj224 commented Sep 30, 2019

View reviewed changes

ciarand suggested changes Sep 30, 2019

View reviewed changes

Cleaning doRegistration method.

bb322d0

ankitaj224 requested a review from ciarand October 1, 2019 01:03

ciarand reviewed Oct 8, 2019

View reviewed changes

Fixing nit comments.

1fd193b

ankitaj224 requested a review from ciarand October 8, 2019 18:59

Addressing ciaran's comment on satisfying immediate getId response.

e2d3631

ciarand approved these changes Oct 9, 2019

View reviewed changes

ankitaj224 added 2 commits October 9, 2019 10:23

Fixing FISClient to correctly parse expiration timestamp. (#848)

e68e599

* Fixing FISClient to correctly parse expiration timestamp.

Updating getAuthToken to return creation timestamp (#884)

58c8efd

* Updating getAuthToken to return creation timestamp along with auth token and expiration timestamp.

google-oss-bot added size/XXL and removed size/XL labels Oct 9, 2019

Propagating the exceptions to the clients. (#856)

56d1eab

* Propagating the exceptions to the clients. * Adding javadocs for StateListener class.

ankitaj224 merged commit 12ae11d into fis_sdk Oct 10, 2019

rlazo added ktx:contrib and removed ktx:contrib labels Oct 16, 2019

firebase locked and limited conversation to collaborators Nov 10, 2019

Simplifying FirebaseInstallations class by adding listeners. #847

Simplifying FirebaseInstallations class by adding listeners. #847

Uh oh!

Conversation

ankitaj224 commented Sep 25, 2019

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!

Uh oh!

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!

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!

ciarand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ankitaj224 commented Sep 30, 2019

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankitaj224 Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ankitaj224 Oct 8, 2019 •

edited

Loading