FIS error handling #911

ankitaj224 · 2019-10-14T21:42:53Z

For FirebaseInstallationServiceClient#createFirebaseInstallation

5xx errors, do not update the PersistedInstallationEntry status to REGISTER_ERROR
Store detailed 4xx server exceptions in the local storage. In the succeeding getId calls, return the error message to the clients

…Token() calls are triggered simultaneously.

…into listeners

* 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.

…into retry

…to retry

* Addressing Rayo's comment: Rename PersistedFidEntry to (#899)

…into retry

…errors 2. Store detailed 4xx server exceptions in the local storage. In the succeeding getId calls,return these exceptions to the clients

…to fis-errors

andirayo · 2019-10-14T22:07:30Z

Regarding the PR's description:
"""Incase of 500 errors, mark PersistedInstallationEntry status UNREGISTERED"""

I am confused by this sentence.
5XX errors are server-errors that the server usually recovers from after a retry.
I may be missing context, but usually the SDK should retry one time a request that it got a 5XX response for and then give up, but not change the status of the Installation.

ciarand · 2019-10-14T22:09:40Z

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

@@ -291,6 +292,31 @@ public void testGetId_PersistedInstallationOk_BackendError() throws Exception {
    assertThat(updatedInstallationEntry).hasRegistrationStatus(RegistrationStatus.REGISTER_ERROR);
  }

+  @Test
+  public void testGetId_500ServerError_UnregisteredFID() throws Exception {
+    when(backendClientReturnsOk.createFirebaseInstallation(


where does this test stub a 500 error?

Sorry for the confusion. Added a comment & changed the test name. Does this makes sense?

ciarand · 2019-10-14T22:10:33Z

...ations/src/main/java/com/google/firebase/installations/local/PersistedInstallationEntry.java

@@ -41,6 +41,9 @@

  public abstract long getTokenCreationEpochInSecs();

+  @Nullable
+  public abstract String getFISException();


are we exposing this to clients? Having exception == String is weird. How about just calling it error message?

Yes, the stored error message. Changed it. PTAL.

Idea: getId is non-blocking call. After we return the id, we encounter some error during registration. We persist the error message & return it to the clients next time getId is called.

andirayo · 2019-10-14T22:04:48Z

firebase-installations/api.txt

@@ -44,6 +44,7 @@ package com.google.firebase.installations.local {
    method @NonNull public static com.google.firebase.installations.local.PersistedInstallationEntry.Builder builder();
    method @Nullable public abstract String getAuthToken();
    method public abstract long getExpiresInSecs();
+    method @Nullable public abstract String getFISException();


I think this should be #getFisException

Compare:
http://go/java-style#s5.3-camel-case

Done.thanks.

andirayo · 2019-10-14T22:10:08Z

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

-              .setExpiresInSecs(installationResponse.getAuthToken().getTokenExpirationTimestamp())
-              .setTokenCreationEpochInSecs(creationTime)
-              .build());
+      if (installationResponse.getFid() != null) {


fyi:
If a #createInstallation request gets a successful response, the field "fid" should always be set.

What happens here if the field "fid" is not set? What state will the SDK end up in?

This is to differentiate between 200, 5xx & 4xx errors.

200 - createInstallation successful response - fid is not empty
5xx - retry once - empty response & fid
4xx - throws exception

If that is what you are doing, then I would recommend your code to express that.
In this case, I would recommend that you check for the 200 HTTP-status instead of the "fid" field.

For that: I have to include Response Code as a field in InstallationResponse. Is that what you recommend?

since y'all control both sides of this exchange we should be able to use and trust the status codes. imo having canonical status codes as a classification for installation responses is a good idea.

andirayo · 2019-10-14T22:11:00Z

...ations/src/main/java/com/google/firebase/installations/local/PersistedInstallationEntry.java

@@ -90,6 +93,9 @@ public abstract Builder setRegistrationStatus(
    @NonNull
    public abstract Builder setTokenCreationEpochInSecs(long value);

+    @NonNull
+    public abstract Builder setFISException(@Nullable String value);


why can this be @nullable?

When PersistedInstallationEntry stores a successfully registered entry.

andirayo · 2019-10-14T22:13:10Z

...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java

@@ -128,7 +128,8 @@ public InstallationResponse createFirebaseInstallation(
            throw new FirebaseException(readErrorResponse(httpsURLConnection));


referring to the 4 lines above:
case 500: retryCount++;

Is it possible for us to treat all server-errors (i.e. 5XX) like this?

Done. PTAL.

ankitaj224 · 2019-10-15T00:10:30Z

Regarding the PR's description:
"""Incase of 500 errors, mark PersistedInstallationEntry status UNREGISTERED"""

I am confused by this sentence.
5XX errors are server-errors that the server usually recovers from after a retry.
I may be missing context, but usually the SDK should retry one time a request that it got a 5XX response for and then give up, but not change the status of the Installation.

Background:
My previous impl of FIS Android SDK would retry once on 500, then throw an exception. That would result in persisted installation entry status marked as REGISTER_ERROR.

After our discussion: On 5xx, we retry once & leave the persisted installation entry status as UNREGISTERED. As a result, no exception will be thrown on subsequent getId calls.

andirayo · 2019-10-15T00:11:02Z

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

-              .setExpiresInSecs(installationResponse.getAuthToken().getTokenExpirationTimestamp())
-              .setTokenCreationEpochInSecs(creationTime)
-              .build());
+      if (installationResponse.getFid() != null) {


If that is what you are doing, then I would recommend your code to express that.
In this case, I would recommend that you check for the 200 HTTP-status instead of the "fid" field.

...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java

ankitaj224 added 27 commits September 25, 2019 13:18

Simplifying FirebaseInstallations class by adding listeners.

49708d2

Splitting multiple classes/interfaces into separate files.

a987647

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

1d9b1ca

…Token() calls are triggered simultaneously.

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

28d0d85

Cleaning up instrumented test file.

126e372

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

Fixing precheck failures.

314b3bb

Addressing Ciaran's comment on mock utils.

715d427

Cleaning doRegistration method.

bb322d0

Fixing nit comments.

1fd193b

Addressing ciaran's comment on satisfying immediate getId response.

e2d3631

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.

Propagating the exceptions to the clients. (#856)

56d1eab

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

Implemneting retry for FIS Client.

c8367dc

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

72e335d

…into retry

Addressing ciaran's comments.

560fba1

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

3ac3da2

…to retry

Merge conflicts.

d9e5c22

Nit fixes

f57137c

Extract FID from FIS createInstallation response (#888)

1d51edf

* Addressing Rayo's comment: Rename PersistedFidEntry to (#899)

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

0c0b9c2

…into retry

Fixing verify format issue.

0407614

1. Mark PersistedInstallationEntry status UNREGISTERED incase of 500 …

2e7b881

…errors 2. Store detailed 4xx server exceptions in the local storage. In the succeeding getId calls,return these exceptions to the clients

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

615bdf4

…to fis-errors

ankitaj224 requested review from ciarand and andirayo October 14, 2019 21:42

ankitaj224 self-assigned this Oct 14, 2019

google-oss-bot added the size/M label Oct 14, 2019

googlebot added the cla: yes Override cla label Oct 14, 2019

ciarand reviewed Oct 14, 2019

View reviewed changes

andirayo approved these changes Oct 14, 2019

View reviewed changes

Addressing Ciaran & Rayo's comments.

dfa85e7

google-oss-bot added size/L and removed size/M labels Oct 14, 2019

ankitaj224 requested a review from ciarand October 15, 2019 00:06

andirayo approved these changes Oct 15, 2019

View reviewed changes

...rc/main/java/com/google/firebase/installations/remote/FirebaseInstallationServiceClient.java Outdated Show resolved Hide resolved

Acceptiong Rayo's suggestion on code improvement.

97b616c

ciarand approved these changes Oct 15, 2019

View reviewed changes

Introducing responseCode to validate if the request was successful.

31d441a

andirayo approved these changes Oct 16, 2019

View reviewed changes

ankitaj224 merged commit 42e442f into fis_sdk Oct 16, 2019

ankitaj224 deleted the fis-errors branch October 16, 2019 17:40

firebase locked and limited conversation to collaborators Nov 16, 2019

		@@ -128,7 +128,8 @@ public InstallationResponse createFirebaseInstallation(
		throw new FirebaseException(readErrorResponse(httpsURLConnection));

FIS error handling #911

FIS error handling #911

Uh oh!

Conversation

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

Uh oh!

Uh oh!

andirayo commented Oct 14, 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!

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!

ankitaj224 commented Oct 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ankitaj224 commented Oct 14, 2019 •

edited

Loading