diff --git a/Firestore/Example/Tests/Integration/FSTTransactionTests.mm b/Firestore/Example/Tests/Integration/FSTTransactionTests.mm index 9f8567c2597..30046b09524 100644 --- a/Firestore/Example/Tests/Integration/FSTTransactionTests.mm +++ b/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(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"]; diff --git a/Firestore/core/src/core/transaction_runner.cc b/Firestore/core/src/core/transaction_runner.cc index 668662eb1ce..6ce1b1003ad 100644 --- a/Firestore/core/src/core/transaction_runner.cc +++ b/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& 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, 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)); diff --git a/Firestore/core/src/core/transaction_runner.h b/Firestore/core/src/core/transaction_runner.h index 80eb32e2271..7e3501ab09d 100644 --- a/Firestore/core/src/core/transaction_runner.h +++ b/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