fix: Make rollback best effort. (#1515)

* fix: Make rollback best effort.

* Pretty

* Fix from review

google-cloud-firestore/src/main/java/com/google/cloud/firestore/Transaction.java

@@ -29,6 +29,8 @@
 import com.google.protobuf.Empty;
 import io.opencensus.trace.Tracing;
 import java.util.List;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
@@ -40,6 +42,7 @@
  */
 public final class Transaction extends UpdateBuilder<Transaction> {
 
+  private static final Logger LOGGER = Logger.getLogger(Transaction.class.getName());
   private static final String READ_BEFORE_WRITE_ERROR_MSG =
       "Firestore transactions require all reads to be executed before all writes";
 
@@ -121,15 +124,29 @@ ApiFuture<List<WriteResult>> commit() {
   /** Rolls a transaction back and releases all read locks. */
   ApiFuture<Void> rollback() {
     Tracing.getTracer().getCurrentSpan().addAnnotation(TraceUtil.SPAN_NAME_ROLLBACK);
-    RollbackRequest.Builder reqBuilder = RollbackRequest.newBuilder();
-    reqBuilder.setTransaction(transactionId);
-    reqBuilder.setDatabase(firestore.getDatabaseName());
+    RollbackRequest req =
+        RollbackRequest.newBuilder()
+            .setTransaction(transactionId)
+            .setDatabase(firestore.getDatabaseName())
+            .build();
 
     ApiFuture<Empty> rollbackFuture =
-        firestore.sendRequest(reqBuilder.build(), firestore.getClient().rollbackCallable());
-
-    return ApiFutures.transform(
-        rollbackFuture, beginTransactionResponse -> null, MoreExecutors.directExecutor());
+        firestore.sendRequest(req, firestore.getClient().rollbackCallable());
+
+    ApiFuture<Void> transform =
+        ApiFutures.transform(rollbackFuture, resp -> null, MoreExecutors.directExecutor());
+
+    return ApiFutures.catching(
+        transform,
+        Throwable.class,
+        (error) -> {
+          LOGGER.log(
+              Level.WARNING,
+              "Failed best effort to rollback of transaction " + transactionId,
+              error);
+          return null;
+        },
+        MoreExecutors.directExecutor());
   }
 
   /**

google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java

@@ -16,6 +16,7 @@
 
 package com.google.cloud.firestore;
 
+import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
@@ -83,6 +84,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;
 import org.mockito.ArgumentCaptor;
@@ -1195,10 +1197,10 @@ public RequestResponsePair(
    * `sendRequest()` is called.
    */
   static class ResponseStubber {
-    int requestCount = 0;
-
     List<RequestResponsePair> operationList = new ArrayList<>();
 
+    List<Object> actualRequestList = new CopyOnWriteArrayList<>();
+
     void put(GeneratedMessageV3 request, ApiFuture<? extends GeneratedMessageV3> response) {
       operationList.add(new RequestResponsePair(request, response));
     }
@@ -1209,8 +1211,7 @@ void initializeStub(
       for (final RequestResponsePair entry : operationList) {
         Answer<ApiFuture<? extends GeneratedMessageV3>> answer =
             invocationOnMock -> {
-              ++requestCount;
-              assertEquals(entry.request, invocationOnMock.getArguments()[0]);
+              actualRequestList.add(invocationOnMock.getArguments()[0]);
               return entry.response;
             };
         stubber = (stubber != null) ? stubber.doAnswer(answer) : doAnswer(answer);
@@ -1223,10 +1224,10 @@ void initializeStub(
     }
 
     public void verifyAllRequestsSent() {
-      assertEquals(
-          String.format("Expected %d requests, but got %d", operationList.size(), requestCount),
-          operationList.size(),
-          requestCount);
+      assertArrayEquals(
+          "Expected requests, but got actual requests",
+          operationList.stream().map(x -> x.request).toArray(),
+          actualRequestList.toArray());
     }
   }
 

google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java

@@ -16,6 +16,7 @@
 
 package com.google.cloud.firestore;
 
+import static com.google.api.core.ApiFutures.immediateFailedFuture;
 import static com.google.cloud.firestore.LocalFirestoreHelper.IMMEDIATE_RETRY_SETTINGS;
 import static com.google.cloud.firestore.LocalFirestoreHelper.SINGLE_FIELD_PROTO;
 import static com.google.cloud.firestore.LocalFirestoreHelper.TRANSACTION_ID;
@@ -88,7 +89,7 @@
 public class TransactionTest {
 
   private final ApiFuture<GeneratedMessageV3> RETRYABLE_API_EXCEPTION =
-      ApiFutures.immediateFailedFuture(
+      immediateFailedFuture(
           new ApiException(
               new Exception("Test exception"), GrpcStatusCode.of(Status.Code.UNKNOWN), true));
 
@@ -244,7 +245,7 @@ public void rollbackOnCallbackApiFutureErrorAsync() {
 
     ApiFuture<String> transaction =
         firestoreMock.runAsyncTransaction(
-            t -> ApiFutures.immediateFailedFuture(new Exception("Expected exception")), options);
+            t -> immediateFailedFuture(new Exception("Expected exception")), options);
 
     try {
       transaction.get();
@@ -262,7 +263,7 @@ public void rollbackOnCallbackApiFutureErrorAsync() {
 
   @Test
   public void noRollbackOnBeginFailure() {
-    doReturn(ApiFutures.immediateFailedFuture(new Exception("Expected exception")))
+    doReturn(immediateFailedFuture(new Exception("Expected exception")))
         .when(firestoreMock)
         .sendRequest(
             requestCapture.capture(), ArgumentMatchers.<UnaryCallable<Message, Message>>any());
@@ -288,7 +289,7 @@ public void noRollbackOnBeginFailure() {
 
   @Test
   public void noRollbackOnBeginFailureAsync() {
-    doReturn(ApiFutures.immediateFailedFuture(new Exception("Expected exception")))
+    doReturn(immediateFailedFuture(new Exception("Expected exception")))
         .when(firestoreMock)
         .sendRequest(
             requestCapture.capture(), ArgumentMatchers.<UnaryCallable<Message, Message>>any());
@@ -493,7 +494,7 @@ public void retriesCommitBasedOnErrorCode() throws Exception {
             new ResponseStubber() {
               {
                 put(begin(), beginResponse("foo1"));
-                put(commit("foo1"), ApiFutures.immediateFailedFuture(e));
+                put(commit("foo1"), immediateFailedFuture(e));
                 put(rollback("foo1"), rollbackResponse());
                 put(begin("foo1"), beginResponse("foo2"));
                 put(commit("foo2"), commitResponse(0, 0));
@@ -503,40 +504,41 @@ public void retriesCommitBasedOnErrorCode() throws Exception {
             new ResponseStubber() {
               {
                 put(begin(), beginResponse("foo1"));
-                put(commit("foo1"), ApiFutures.immediateFailedFuture(e));
+                put(commit("foo1"), immediateFailedFuture(e));
                 put(rollback("foo1"), rollbackResponse());
               }
             });
   }
 
   @Test
   public void retriesRollbackBasedOnErrorCode() throws Exception {
-    final ApiException commitException = exception(Status.Code.ABORTED, true);
-
+    final ApiException retryable = exception(Status.Code.ABORTED, true);
+
+    // Regardless of exception thrown by rollback, we should never retry
+    // calling rollback. Rollback is best effort, and will sometimes return
+    // ABORT error (which a transaction will retry) when transaction no longer
+    // exists on Firestore server side. Attempting to retry will in some cases
+    // simply exhaust retries with accumulated backoff delay, when a new
+    // transaction could simply be started (since the old transaction no longer
+    // exists server side).
     verifyRetries(
         /* expectedSequenceWithRetry= */ e -> {
-          final ApiFuture<GeneratedMessageV3> rollbackException =
-              ApiFutures.immediateFailedFuture(e);
           return new ResponseStubber() {
             {
               put(begin(), beginResponse("foo1"));
-              put(commit("foo1"), ApiFutures.immediateFailedFuture(commitException));
-              put(rollback("foo1"), rollbackException);
-              put(rollback("foo1"), rollbackResponse());
+              put(commit("foo1"), immediateFailedFuture(e));
+              put(rollback("foo1"), immediateFailedFuture(retryable));
               put(begin("foo1"), beginResponse("foo2"));
               put(commit("foo2"), commitResponse(0, 0));
             }
           };
         },
         /* expectedSequenceWithoutRetry= */ e -> {
-          final ApiFuture<GeneratedMessageV3> rollbackException =
-              ApiFutures.immediateFailedFuture(e);
           return new ResponseStubber() {
             {
               put(begin(), beginResponse("foo1"));
-              put(commit("foo1"), ApiFutures.immediateFailedFuture(commitException));
-              put(rollback("foo1"), rollbackException);
-              put(rollback("foo1"), rollbackResponse());
+              put(commit("foo1"), immediateFailedFuture(e));
+              put(rollback("foo1"), immediateFailedFuture(retryable));
             }
           };
         });