From 8f41b37fd766b18113401c7324e319b52b38cc96 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 14 Jan 2020 13:19:02 -0800 Subject: [PATCH] refactor: use loop for transactions (#875) --- dev/src/index.ts | 77 +++-------------------------------------- dev/src/transaction.ts | 76 +++++++++++++++++++++++++++++++++------- dev/src/write-batch.ts | 9 +++++ dev/test/transaction.ts | 24 ++++++++----- dev/test/write-batch.ts | 19 ++++++++++ 5 files changed, 112 insertions(+), 93 deletions(-) diff --git a/dev/src/index.ts b/dev/src/index.ts index 8f83c12da..949cb87bc 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -789,7 +789,7 @@ export class Firestore { const defaultAttempts = 5; const tag = requestTag(); - let attemptsRemaining: number; + let maxAttempts: number; if (transactionOptions) { validateObject('transactionOptions', transactionOptions); @@ -798,84 +798,17 @@ export class Firestore { transactionOptions.maxAttempts, {optional: true, minValue: 1} ); - attemptsRemaining = transactionOptions.maxAttempts || defaultAttempts; + maxAttempts = transactionOptions.maxAttempts || defaultAttempts; } else { - attemptsRemaining = defaultAttempts; + maxAttempts = defaultAttempts; } + const transaction = new Transaction(this, tag); return this.initializeIfNeeded(tag).then(() => - this._runTransaction(updateFunction, {requestTag: tag, attemptsRemaining}) + transaction.runTransaction(updateFunction, maxAttempts) ); } - _runTransaction( - updateFunction: (transaction: Transaction) => Promise, - transactionOptions: { - requestTag: string; - attemptsRemaining: number; - previousTransaction?: Transaction; - } - ): Promise { - const requestTag = transactionOptions.requestTag; - const attemptsRemaining = transactionOptions.attemptsRemaining; - const previousTransaction = transactionOptions.previousTransaction; - - const transaction = new Transaction(this, requestTag, previousTransaction); - let result: Promise; - - return transaction - .begin() - .then(() => { - const promise = updateFunction(transaction); - result = - promise instanceof Promise - ? promise - : Promise.reject( - new Error( - 'You must return a Promise in your transaction()-callback.' - ) - ); - return result.catch(err => { - logger( - 'Firestore.runTransaction', - requestTag, - 'Rolling back transaction after callback error:', - err - ); - // Rollback the transaction and return the failed result. - return transaction.rollback().then(() => { - return result; - }); - }); - }) - .then(() => { - return transaction - .commit() - .then(() => result) - .catch(err => { - if (attemptsRemaining > 1) { - logger( - 'Firestore.runTransaction', - requestTag, - `Retrying transaction after error: ${JSON.stringify(err)}.` - ); - return this._runTransaction(updateFunction, { - previousTransaction: transaction, - requestTag, - attemptsRemaining: attemptsRemaining - 1, - }); - } - logger( - 'Firestore.runTransaction', - requestTag, - 'Exhausted transaction retries, returning error: %s', - err - ); - return Promise.reject(err); - }); - }); - } - /** * Fetches the root collections that are associated with this Firestore * database. diff --git a/dev/src/transaction.ts b/dev/src/transaction.ts index 2030ef123..482ba71cb 100644 --- a/dev/src/transaction.ts +++ b/dev/src/transaction.ts @@ -18,6 +18,7 @@ import * as proto from '../protos/firestore_v1_proto_api'; import {DocumentSnapshot, Precondition} from './document'; import {Firestore, WriteBatch} from './index'; +import {logger} from './logger'; import {FieldPath, validateFieldPath} from './path'; import { DocumentReference, @@ -70,17 +71,9 @@ export class Transaction { * @param firestore The Firestore Database client. * @param requestTag A unique client-assigned identifier for the scope of * this transaction. - * @param previousTransaction If available, the failed transaction that is - * being retried. */ - constructor( - firestore: Firestore, - requestTag: string, - previousTransaction?: Transaction - ) { + constructor(firestore: Firestore, requestTag: string) { this._firestore = firestore; - this._transactionId = - previousTransaction && previousTransaction._transactionId; this._writeBatch = firestore.batch(); this._requestTag = requestTag; } @@ -398,12 +391,69 @@ export class Transaction { } /** - * Returns the tag to use with with all request for this Transaction. + * Executes `updateFunction()` and commits the transaction with retry. + * * @private - * @return A unique client-generated identifier for this transaction. + * @param updateFunction The user function to execute within the transaction + * context. + * @param requestTag A unique client-assigned identifier for the scope of + * this transaction. + * @param maxAttempts The maximum number of attempts for this transaction. */ - get requestTag(): string { - return this._requestTag; + async runTransaction( + updateFunction: (transaction: Transaction) => Promise, + maxAttempts: number + ): Promise { + let result: T; + let lastError: Error | undefined = undefined; + + for (let attempt = 0; attempt < maxAttempts; ++attempt) { + if (lastError) { + logger( + 'Firestore.runTransaction', + this._requestTag, + `Retrying transaction after error:`, + lastError + ); + } + + await this.begin(); + + try { + const promise = updateFunction(this); + if (!(promise instanceof Promise)) { + throw new Error( + 'You must return a Promise in your transaction()-callback.' + ); + } + result = await promise; + } catch (err) { + logger( + 'Firestore.runTransaction', + this._requestTag, + 'Rolling back transaction after callback error:', + err + ); + await this.rollback(); + return Promise.reject(err); // User callback failed + } + + try { + await this.commit(); + return result; // Success + } catch (err) { + lastError = err; + this._writeBatch._reset(); + } + } + + logger( + 'Firestore.runTransaction', + this._requestTag, + 'Exhausted transaction retries, returning error: %s', + lastError + ); + return Promise.reject(lastError); } } diff --git a/dev/src/write-batch.ts b/dev/src/write-batch.ts index 6b2e161ee..4191a7471 100644 --- a/dev/src/write-batch.ts +++ b/dev/src/write-batch.ts @@ -652,6 +652,15 @@ export class WriteBatch { return true; } + + /** + * Resets the WriteBatch and dequeues all pending operations. + * @private + */ + _reset() { + this._ops.splice(0); + this._committed = false; + } } /** diff --git a/dev/test/transaction.ts b/dev/test/transaction.ts index b4dba86a8..453440595 100644 --- a/dev/test/transaction.ts +++ b/dev/test/transaction.ts @@ -517,10 +517,14 @@ describe('transaction operations', () => { it('enforce that gets come before writes', () => { return expect( - runTransaction((transaction, docRef) => { - transaction.set(docRef, {foo: 'bar'}); - return transaction.get(docRef); - }, begin()) + runTransaction( + (transaction, docRef) => { + transaction.set(docRef, {foo: 'bar'}); + return transaction.get(docRef); + }, + begin(), + rollback() + ) ).to.eventually.be.rejectedWith( 'Firestore transactions require all reads to be executed before all writes.' ); @@ -575,10 +579,14 @@ describe('transaction operations', () => { it('enforce that getAll come before writes', () => { return expect( - runTransaction((transaction, docRef) => { - transaction.set(docRef, {foo: 'bar'}); - return transaction.getAll(docRef); - }, begin()) + runTransaction( + (transaction, docRef) => { + transaction.set(docRef, {foo: 'bar'}); + return transaction.getAll(docRef); + }, + begin(), + rollback() + ) ).to.eventually.be.rejectedWith( 'Firestore transactions require all reads to be executed before all writes.' ); diff --git a/dev/test/write-batch.ts b/dev/test/write-batch.ts index 1b125347f..692d40abb 100644 --- a/dev/test/write-batch.ts +++ b/dev/test/write-batch.ts @@ -356,6 +356,25 @@ describe('batch support', () => { return promise; }); + it('can reset a committed batch', async () => { + const documentName = firestore.doc('col/doc'); + + const batch = firestore.batch(); + batch.set(documentName, {foo: FieldValue.serverTimestamp()}); + batch.update(documentName, {foo: 'bar'}); + batch.create(documentName, {}); + batch.delete(documentName); + await batch.commit(); + + batch._reset(); + + batch.set(documentName, {foo: FieldValue.serverTimestamp()}); + batch.update(documentName, {foo: 'bar'}); + batch.create(documentName, {}); + batch.delete(documentName); + await batch.commit(); + }); + it('can commit an unmodified batch multiple times', () => { const documentName = firestore.doc('col/doc');