Only retry transactions on non-permanent errors (#628)

Author: Brian Chen

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java

@@ -375,7 +375,7 @@ public void testUpdateTransactionally() {
     ArrayList<Task<Void>> readTasks = new ArrayList<>();
     // A barrier to make sure every transaction reaches the same spot.
     TaskCompletionSource<Void> barrier = new TaskCompletionSource<>();
-    AtomicInteger started = new AtomicInteger(0);
+    AtomicInteger counter = new AtomicInteger(0);
 
     FirebaseFirestore firestore = testFirestore();
     DocumentReference doc = firestore.collection("counters").document();
@@ -388,9 +388,9 @@ public void testUpdateTransactionally() {
       transactionTasks.add(
           firestore.runTransaction(
               transaction -> {
+                counter.incrementAndGet();
                 DocumentSnapshot snapshot = transaction.get(doc);
                 assertNotNull(snapshot);
-                started.incrementAndGet();
                 resolveRead.trySetResult(null);
                 waitFor(barrier.getTask());
                 transaction.update(doc, map("count", snapshot.getDouble("count") + 1.0));
@@ -400,10 +400,12 @@ public void testUpdateTransactionally() {
 
     // Let all of the transactions fetch the old value and stop once.
     waitFor(Tasks.whenAll(readTasks));
-    assertEquals(3, started.intValue());
-    // Let all of the transactions continue and wait for them to finish.
+    // There should be 3 initial transaction runs.
+    assertEquals(3, counter.get());
     barrier.setResult(null);
     waitFor(Tasks.whenAll(transactionTasks));
+    // There should be a maximum of 3 retries: once for the 2nd update, and twice for the 3rd update
+    assertTrue(counter.get() <= 6);
     // Now all transaction should be completed, so check the result.
     DocumentSnapshot snapshot = waitFor(doc.get());
     assertEquals(8, snapshot.getDouble("count").intValue());
@@ -546,15 +548,17 @@ public void testReadingADocTwiceWithDifferentVersions() {
     FirebaseFirestore firestore = testFirestore();
     DocumentReference doc = firestore.collection("counters").document();
     waitFor(doc.set(map("count", 15.0)));
+    AtomicInteger counter = new AtomicInteger(0);
     Exception e =
         waitForException(
             firestore.runTransaction(
                 transaction -> {
+                  counter.incrementAndGet();
                   // Get the doc once.
                   DocumentSnapshot snapshot1 = transaction.get(doc);
-                  assertEquals(15, snapshot1.getDouble("count").intValue());
-                  // Do a write outside of the transaction.
-                  waitFor(doc.set(map("count", 1234.0)));
+                  // Do a write outside of the transaction. Because the transaction will retry, set
+                  // the document to a different value each time.
+                  waitFor(doc.set(map("count", 1234.0 + counter.get())));
                   // Get the doc again in the transaction with the new version.
                   DocumentSnapshot snapshot2 = transaction.get(doc);
                   // The get itself will fail, because we already read an earlier version of this
@@ -563,8 +567,6 @@ public void testReadingADocTwiceWithDifferentVersions() {
                   return null;
                 }));
     assertEquals(Code.ABORTED, ((FirebaseFirestoreException) e).getCode());
-    DocumentSnapshot snapshot = waitFor(doc.get());
-    assertEquals(1234, snapshot.getDouble("count").intValue());
   }
 
   @Test
@@ -622,6 +624,28 @@ public void testCannotHaveAGetWithoutMutations() {
     assertEquals("Every document read in a transaction must also be written.", e.getMessage());
   }
 
+  @Test
+  public void testDoesNotRetryOnPermanentError() {
+    final FirebaseFirestore firestore = testFirestore();
+    AtomicInteger count = new AtomicInteger(0);
+    // Make a transaction that should fail with a permanent error
+    Task<Void> transactionTask =
+        firestore.runTransaction(
+            transaction -> {
+              count.incrementAndGet();
+              // Get and update a document that doesn't exist so that the transaction fails
+              DocumentSnapshot doc =
+                  transaction.get(firestore.collection("nonexistent").document());
+              transaction.update(doc.getReference(), "foo", "bar");
+              return null;
+            });
+
+    // Let all of the transactions fetch the old value and stop once.
+    Exception e = waitForException(transactionTask);
+    assertEquals(Code.INVALID_ARGUMENT, ((FirebaseFirestoreException) e).getCode());
+    assertEquals(1, count.get());
+  }
+
   @Test
   public void testSuccessWithNoTransactionOperations() {
     FirebaseFirestore firestore = testFirestore();

firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java

@@ -24,6 +24,7 @@
 import com.google.common.base.Function;
 import com.google.firebase.database.collection.ImmutableSortedMap;
 import com.google.firebase.database.collection.ImmutableSortedSet;
+import com.google.firebase.firestore.FirebaseFirestoreException;
 import com.google.firebase.firestore.auth.User;
 import com.google.firebase.firestore.local.LocalStore;
 import com.google.firebase.firestore.local.LocalViewChanges;
@@ -38,6 +39,7 @@
 import com.google.firebase.firestore.model.SnapshotVersion;
 import com.google.firebase.firestore.model.mutation.Mutation;
 import com.google.firebase.firestore.model.mutation.MutationBatchResult;
+import com.google.firebase.firestore.remote.Datastore;
 import com.google.firebase.firestore.remote.RemoteEvent;
 import com.google.firebase.firestore.remote.RemoteStore;
 import com.google.firebase.firestore.remote.TargetChange;
@@ -262,6 +264,9 @@ public <TResult> Task<TResult> transaction(
             asyncQueue.getExecutor(),
             userTask -> {
               if (!userTask.isSuccessful()) {
+                if (retries > 0 && isRetryableTransactionError(userTask.getException())) {
+                  return transaction(asyncQueue, updateFunction, retries - 1);
+                }
                 return userTask;
               }
               return transaction
@@ -272,11 +277,11 @@ public <TResult> Task<TResult> transaction(
                         if (commitTask.isSuccessful()) {
                           return Tasks.forResult(userTask.getResult());
                         }
-                        // TODO: Only retry on real transaction failures.
-                        if (retries == 0) {
-                          return Tasks.forException(commitTask.getException());
+                        Exception e = commitTask.getException();
+                        if (retries > 0 && isRetryableTransactionError(e)) {
+                          return transaction(asyncQueue, updateFunction, retries - 1);
                         }
-                        return transaction(asyncQueue, updateFunction, retries - 1);
+                        return Tasks.forException(e);
                       });
             });
   }
@@ -589,4 +594,16 @@ private boolean errorIsInteresting(Status error) {
 
     return false;
   }
+
+  private boolean isRetryableTransactionError(Exception e) {
+    if (e instanceof FirebaseFirestoreException) {
+      // In transactions, the backend will fail outdated reads with FAILED_PRECONDITION and
+      // non-matching document versions with ABORTED. These errors should be retried.
+      FirebaseFirestoreException.Code code = ((FirebaseFirestoreException) e).getCode();
+      return code == FirebaseFirestoreException.Code.ABORTED
+          || code == FirebaseFirestoreException.Code.FAILED_PRECONDITION
+          || !Datastore.isPermanentError(((FirebaseFirestoreException) e).getCode());
+    }
+    return false;
+  }
 }

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/Datastore.java

@@ -189,8 +189,18 @@ public Task<List<MaybeDocument>> lookup(List<DocumentKey> keys) {
    * @see #isPermanentWriteError for classifying write errors.
    */
   public static boolean isPermanentError(Status status) {
+    return isPermanentError(FirebaseFirestoreException.Code.fromValue(status.getCode().value()));
+  }
+
+  /**
+   * Determines whether the given error code represents a permanent error when received in response
+   * to a non-write operation.
+   *
+   * @see #isPermanentWriteError for classifying write errors.
+   */
+  public static boolean isPermanentError(FirebaseFirestoreException.Code code) {
     // See go/firestore-client-errors
-    switch (status.getCode()) {
+    switch (code) {
       case OK:
         throw new IllegalArgumentException("Treated status OK as error");
       case CANCELLED:
@@ -217,7 +227,7 @@ public static boolean isPermanentError(Status status) {
       case DATA_LOSS:
         return true;
       default:
-        throw new IllegalArgumentException("Unknown gRPC status code: " + status.getCode());
+        throw new IllegalArgumentException("Unknown gRPC status code: " + code);
     }
   }