From 7047a539ba33e90ea95740592b1399f6074753f7 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 21 Oct 2019 11:23:51 -0700 Subject: [PATCH 1/3] Retry all non-Firestore exceptions Our users are still seeing IndexedDB errors on iOS 13 (using IONIC/Cordova). One of the possible reasons is that some environments might not throw DOMExceptions, which would lead us to not retry. This PR flips our logic around and only skips retries if the underlying exception is a non-Firestore exception. Fixes https://github.com/firebase/firebase-js-sdk/issues/2232 --- packages/firestore/src/local/simple_db.ts | 8 ++++---- packages/firestore/test/unit/local/simple_db.test.ts | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/firestore/src/local/simple_db.ts b/packages/firestore/src/local/simple_db.ts index 9a815086661..c9e657fdb8b 100644 --- a/packages/firestore/src/local/simple_db.ts +++ b/packages/firestore/src/local/simple_db.ts @@ -294,23 +294,23 @@ export class SimpleDb { // caller. await transaction.completionPromise; return transactionFnResult; - } catch (e) { + } catch (error) { // TODO(schmidt-sebastian): We could probably be smarter about this and // not retry exceptions that are likely unrecoverable (such as quota // exceeded errors). const retryable = idempotent && - isDomException(e) && + error.name !== 'FirebaseError' && attemptNumber < TRANSACTION_RETRY_COUNT; debug( LOG_TAG, 'Transaction failed with error: %s. Retrying: %s.', - e.message, + error.message, retryable ); if (!retryable) { - return Promise.reject(e); + return Promise.reject(error); } } } diff --git a/packages/firestore/test/unit/local/simple_db.test.ts b/packages/firestore/test/unit/local/simple_db.test.ts index a2ef3c075cc..e54d80c0517 100644 --- a/packages/firestore/test/unit/local/simple_db.test.ts +++ b/packages/firestore/test/unit/local/simple_db.test.ts @@ -28,6 +28,7 @@ import { import { PersistencePromise } from '../../../src/local/persistence_promise'; import { fail } from '../../../src/util/assert'; +import { Code, FirestoreError } from '../../../src/util/error'; use(chaiAsPromised); @@ -551,8 +552,8 @@ describe('SimpleDb', () => { await expect( db.runTransaction('readwrite-idempotent', ['users'], txn => { ++attemptCount; - txn.abort(); - return PersistencePromise.reject(new Error('Aborted')); + txn.abort(new FirestoreError(Code.ABORTED, 'Aborted')); + return PersistencePromise.reject(new Error()); }) ).to.eventually.be.rejected; From 8d72617d5a1ec4ce4c5fc8b65d84cad1620307c3 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 21 Oct 2019 13:28:45 -0700 Subject: [PATCH 2/3] Update simple_db.ts --- packages/firestore/src/local/simple_db.ts | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/packages/firestore/src/local/simple_db.ts b/packages/firestore/src/local/simple_db.ts index c9e657fdb8b..fa80fd1250e 100644 --- a/packages/firestore/src/local/simple_db.ts +++ b/packages/firestore/src/local/simple_db.ts @@ -801,13 +801,3 @@ function checkForAndReportiOSError(error: DOMException): Error { } return error; } - -/** Checks whether an error is a DOMException (e.g. as thrown by IndexedDb). */ -function isDomException(error: Error): boolean { - // DOMException is not a global type in Node with persistence, and hence we - // check the constructor name if the type in unknown. - return ( - (typeof DOMException !== 'undefined' && error instanceof DOMException) || - error.constructor.name === 'DOMException' - ); -} From 0bfb4596ef3c360d6739116e6500c72601584d3b Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 21 Oct 2019 14:01:43 -0700 Subject: [PATCH 3/3] Update simple_db.ts --- packages/firestore/src/local/simple_db.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/firestore/src/local/simple_db.ts b/packages/firestore/src/local/simple_db.ts index fa80fd1250e..62d3bce1f82 100644 --- a/packages/firestore/src/local/simple_db.ts +++ b/packages/firestore/src/local/simple_db.ts @@ -298,6 +298,9 @@ export class SimpleDb { // TODO(schmidt-sebastian): We could probably be smarter about this and // not retry exceptions that are likely unrecoverable (such as quota // exceeded errors). + + // Note: We cannot use an instanceof check for FirestoreException, since the + // exception is wrapped in a generic error by our async/await handling. const retryable = idempotent && error.name !== 'FirebaseError' &&