fix(firestore, ios): transaction atomicity failure fix (#3599)

invertase · May 12, 2020 · b261f51 · b261f51
1 parent cd5cb23
commit b261f51
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 14 deletions.
diff --git a/packages/firestore/e2e/Transaction.e2e.js b/packages/firestore/e2e/Transaction.e2e.js
@@ -335,5 +335,47 @@ describe('firestore.Transaction', () => {
       const snapshot = await docRef.get();
       snapshot.data().should.eql(jet.contextify(expected));
     });
+
+    it('should roll back any updates that failed', async () => {
+      const docRef = firebase.firestore().doc('v6/transactions/transaction/rollback');
+
+      await docRef.set({
+        turn: 0,
+      });
+
+      const prop1 = 'prop1';
+      const prop2 = 'prop2';
+      const turn = 0;
+      const errorMessage = 'turn cannot exceed 1';
+
+      const createTransaction = prop => {
+        return firebase.firestore().runTransaction(async transaction => {
+          const doc = await transaction.get(docRef);
+          const data = doc.data();
+
+          if (data.turn !== turn) {
+            throw new Error(errorMessage);
+          }
+
+          const update = {
+            turn: turn + 1,
+            [prop]: 1,
+          };
+
+          transaction.update(docRef, update);
+        });
+      };
+
+      const promises = [createTransaction(prop1), createTransaction(prop2)];
+
+      try {
+        await Promise.all(promises);
+        return Promise.reject(new Error('Did not throw an Error.'));
+      } catch (error) {
+        error.message.should.containEql(errorMessage);
+      }
+      const result = await docRef.get();
+      should(result.data()).not.have.properties([prop1, prop2]);
+    });
   });
 });
diff --git a/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.m b/packages/firestore/ios/RNFBFirestore/RNFBFirestoreTransactionModule.m
@@ -108,7 +108,7 @@ - (void)invalidate {
     }
 
     dispatch_semaphore_t semaphore = transactionState[@"semaphore"];
-    transactionState[@"abort"] = @(true);
+    transactionState[@"aborted"] = @(true);
     dispatch_semaphore_signal(semaphore);
   }
 }
@@ -119,7 +119,7 @@ - (void)invalidate {
     :(NSArray *)commandBuffer
 ) {
   @synchronized (transactions[[transactionId stringValue]]) {
-    __block NSMutableDictionary *transactionState = transactions[[transactionId stringValue]];
+    NSMutableDictionary *transactionState = transactions[[transactionId stringValue]];
 
     if (!transactionState) {
       DLog(@"transactionGetDocument called for non-existent transactionId %@", transactionId);
@@ -138,7 +138,6 @@ - (void)invalidate {
 ) {
   FIRFirestore *firestore = [RNFBFirestoreCommon getFirestoreForApp:firebaseApp];
   __block BOOL aborted = false;
-  __block BOOL completed = false;
   __block NSMutableDictionary *transactionState = [NSMutableDictionary new];
 
   id transactionBlock = ^id(FIRTransaction *transaction, NSError **errorPointer) {
@@ -172,7 +171,7 @@ - (void)invalidate {
 
     @synchronized (transactionState) {
       aborted = (BOOL) transactionState[@"aborted"];
-
+      
       if (transactionState[@"semaphore"] != semaphore) {
         return nil;
       }
@@ -187,10 +186,6 @@ - (void)invalidate {
         return nil;
       }
 
-      if (completed == YES) {
-        return nil;
-      }
-
       NSArray *commandBuffer = transactionState[@"commandBuffer"];
 
       for (NSDictionary *command in commandBuffer) {
@@ -223,12 +218,6 @@ - (void)invalidate {
   };
 
   id completionBlock = ^(id result, NSError *error) {
-    if (completed == YES) {
-      return;
-    }
-
-    completed = YES;
-
     @synchronized (transactionState) {
       if (aborted == NO) {
         NSMutableDictionary *eventMap = [NSMutableDictionary new];