Standardize transaction retries to attempts (#8314)

Author: Brian Chen

Firestore/Example/Tests/Integration/FSTTransactionTests.mm

@@ -649,6 +649,45 @@ - (void)testDoesNotRetryOnPermanentError {
   [self awaitExpectations];
 }
 
+- (void)testMakesDefaultMaxAttempts {
+  FIRFirestore *firestore = [self firestore];
+  FIRDocumentReference *doc1 = [[firestore collectionWithPath:@"counters"] documentWithAutoID];
+  auto counter = std::make_shared<std::atomic_int>(0);
+
+  [self writeDocumentRef:doc1 data:@{@"count" : @(15.0)}];
+
+  // Skip backoff delays.
+  [firestore workerQueue]->SkipDelaysForTimerId(TimerId::RetryTransaction);
+
+  XCTestExpectation *expectation = [self expectationWithDescription:@"transaction"];
+  [firestore
+      runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **error) {
+        ++(*counter);
+        // Get the first doc.
+        [transaction getDocument:doc1 error:error];
+        XCTAssertNil(*error);
+        // Do a write outside of the transaction to cause the transaction to fail.
+        dispatch_semaphore_t writeSemaphore = dispatch_semaphore_create(0);
+        int newValue = 1234 + counter->load();
+        [doc1 setData:@{
+          @"count" : @(newValue)
+        }
+            completion:^(NSError *) {
+              dispatch_semaphore_signal(writeSemaphore);
+            }];
+        // We can block on it, because transactions run on a background queue.
+        dispatch_semaphore_wait(writeSemaphore, DISPATCH_TIME_FOREVER);
+        return nil;
+      }
+      completion:^(id, NSError *_Nullable error) {
+        [expectation fulfill];
+        XCTAssertNotNil(error);
+        XCTAssertEqual(error.code, FIRFirestoreErrorCodeFailedPrecondition);
+        XCTAssertEqual(counter->load(), 5);
+      }];
+  [self awaitExpectations];
+}
+
 - (void)testSuccessWithNoTransactionOperations {
   FIRFirestore *firestore = [self firestore];
   XCTestExpectation *expectation = [self expectationWithDescription:@"transaction"];

Firestore/core/src/core/transaction_runner.cc

@@ -31,8 +31,8 @@ using util::AsyncQueue;
 using util::Status;
 using util::TimerId;
 
-/** Maximum number of times a transaction can be retried before failing. */
-constexpr int kRetryCount = 5;
+/** Maximum number of times a transaction can be attempted before failing. */
+constexpr int kMaxAttemptsCount = 5;
 
 bool IsRetryableTransactionError(const util::Status& error) {
   // In transactions, the backend will fail outdated reads with
@@ -54,11 +54,12 @@ TransactionRunner::TransactionRunner(const std::shared_ptr<AsyncQueue>& queue,
       update_callback_{std::move(update_callback)},
       result_callback_{std::move(result_callback)},
       backoff_{queue_, TimerId::RetryTransaction},
-      retries_left_{kRetryCount} {
+      attempts_remaining_{kMaxAttemptsCount} {
 }
 
 void TransactionRunner::Run() {
   queue_->VerifyIsCurrentQueue();
+  attempts_remaining_ -= 1;
 
   auto shared_this = this->shared_from_this();
   backoff_.BackoffAndRun([shared_this] {
@@ -96,9 +97,8 @@ void TransactionRunner::DispatchResult(
 
 void TransactionRunner::HandleTransactionError(
     const std::shared_ptr<Transaction>& transaction, Status status) {
-  if (retries_left_ > 0 && IsRetryableTransactionError(status) &&
+  if (attempts_remaining_ > 0 && IsRetryableTransactionError(status) &&
       !transaction->IsPermanentlyFailed()) {
-    retries_left_ -= 1;
     Run();
   } else {
     result_callback_(std::move(status));

Firestore/core/src/core/transaction_runner.h

@@ -67,7 +67,7 @@ class TransactionRunner
   core::TransactionUpdateCallback update_callback_;
   core::TransactionResultCallback result_callback_;
   remote::ExponentialBackoff backoff_;
-  int retries_left_;
+  int attempts_remaining_;
 };
 
 }  // namespace core