diff --git a/packages/firestore/CHANGELOG.md b/packages/firestore/CHANGELOG.md index dce46a2ee05..69b5ed8abc2 100644 --- a/packages/firestore/CHANGELOG.md +++ b/packages/firestore/CHANGELOG.md @@ -1,4 +1,6 @@ # Unreleased +- [fixed] Firestore now rejects write operations if they cannot be persisted + in IndexedDB. Previously, these errors crashed the client. - [fixed] Fixed a source of IndexedDB-related crashes for tabs that receive multi-tab notifications while the file system is locked. diff --git a/packages/firestore/src/core/sync_engine.ts b/packages/firestore/src/core/sync_engine.ts index ea5cb515c07..727c89eee5a 100644 --- a/packages/firestore/src/core/sync_engine.ts +++ b/packages/firestore/src/core/sync_engine.ts @@ -16,7 +16,11 @@ */ import { User } from '../auth/user'; -import { ignoreIfPrimaryLeaseLoss, LocalStore } from '../local/local_store'; +import { + ignoreIfPrimaryLeaseLoss, + LocalStore, + LocalWriteResult +} from '../local/local_store'; import { LocalViewChanges } from '../local/local_view_changes'; import { ReferenceSet } from '../local/reference_set'; import { TargetData, TargetPurpose } from '../local/target_data'; @@ -34,7 +38,7 @@ import { RemoteStore } from '../remote/remote_store'; import { RemoteSyncer } from '../remote/remote_syncer'; import { debugAssert, fail, hardAssert } from '../util/assert'; import { Code, FirestoreError } from '../util/error'; -import { logDebug } from '../util/log'; +import { logDebug, logError } from '../util/log'; import { primitiveComparator } from '../util/misc'; import { ObjectMap } from '../util/obj_map'; import { Deferred } from '../util/promise'; @@ -371,7 +375,24 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer { */ async write(batch: Mutation[], userCallback: Deferred): Promise { this.assertSubscribed('write()'); - const result = await this.localStore.localWrite(batch); + + let result: LocalWriteResult; + try { + result = await this.localStore.localWrite(batch); + } catch (e) { + if (e.name === 'IndexedDbTransactionError') { + // If we can't persist the mutation, we reject the user callback and + // don't send the mutation. The user can then retry the write. + logError(LOG_TAG, 'Dropping write that cannot be persisted: ' + e); + userCallback.reject( + new FirestoreError(Code.UNAVAILABLE, 'Failed to persist write: ' + e) + ); + return; + } else { + throw e; + } + } + this.sharedClientState.addPendingMutation(result.batchId); this.addMutationCallback(result.batchId, userCallback); await this.emitNewSnapsAndNotifyLocalStore(result.changes); diff --git a/packages/firestore/test/integration/bootstrap.ts b/packages/firestore/test/integration/bootstrap.ts index 8da78025fc0..a38829ab6b9 100644 --- a/packages/firestore/test/integration/bootstrap.ts +++ b/packages/firestore/test/integration/bootstrap.ts @@ -1,6 +1,6 @@ /** * @license - * Copyright 2017 Google Inc. + * Copyright 2017 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -26,7 +26,11 @@ import '../../index'; // 'context()' definition requires additional dependency on webpack-env package. // eslint-disable-next-line @typescript-eslint/no-explicit-any -const testsContext = (require as any).context('.', true, /^((?!node).)*\.test$/); +const testsContext = (require as any).context( + '.', + true, + /^((?!node).)*\.test$/ +); const browserTests = testsContext .keys() .filter((file: string) => !file.match(/([\/.])node([\/.])/)); diff --git a/packages/firestore/test/unit/bootstrap.ts b/packages/firestore/test/unit/bootstrap.ts index 0282fe73856..1013e3e1f96 100644 --- a/packages/firestore/test/unit/bootstrap.ts +++ b/packages/firestore/test/unit/bootstrap.ts @@ -26,7 +26,11 @@ import '../../src/platform_browser/browser_init'; // 'context()' definition requires additional dependency on webpack-env package. // eslint-disable-next-line @typescript-eslint/no-explicit-any -const testsContext = (require as any).context('.', true, /^((?!node).)*\.test$/); +const testsContext = (require as any).context( + '.', + true, + /^((?!node).)*\.test$/ +); const browserTests = testsContext .keys() .filter((file: string) => !file.match(/([\/.])node([\/.])/)); diff --git a/packages/firestore/test/unit/remote/serializer.helper.ts b/packages/firestore/test/unit/remote/serializer.helper.ts index e475f8a7431..eb34dfd931e 100644 --- a/packages/firestore/test/unit/remote/serializer.helper.ts +++ b/packages/firestore/test/unit/remote/serializer.helper.ts @@ -96,7 +96,7 @@ const protoJsReader = testUserDataReader(/* useProto3Json= */ false); */ export function serializerTest( protobufJsVerifier: (jsonValue: api.Value) => void = () => {} -) : void { +): void { describe('Serializer', () => { const partition = new DatabaseId('p', 'd'); const s = new JsonProtoSerializer(partition, { useProto3Json: false }); diff --git a/packages/firestore/test/unit/specs/recovery_spec.test.ts b/packages/firestore/test/unit/specs/recovery_spec.test.ts index 5aadd6976bf..4b771bae0c7 100644 --- a/packages/firestore/test/unit/specs/recovery_spec.test.ts +++ b/packages/firestore/test/unit/specs/recovery_spec.test.ts @@ -16,98 +16,150 @@ */ import { describeSpec, specTest } from './describe_spec'; -import { client } from './spec_builder'; +import { client, spec } from './spec_builder'; import { TimerId } from '../../../src/util/async_queue'; import { Query } from '../../../src/core/query'; -import { path } from '../../util/helpers'; +import { doc, path } from '../../util/helpers'; -describeSpec( - 'Persistence Recovery', - ['durable-persistence', 'no-ios', 'no-android'], - () => { - specTest( - 'Write is acknowledged by primary client (with recovery)', - ['multi-client'], - () => { - return ( - client(0) - .expectPrimaryState(true) - .client(1) - .expectPrimaryState(false) - .userSets('collection/a', { v: 1 }) - .failDatabase() - .client(0) - .writeAcks('collection/a', 1, { expectUserCallback: false }) - .client(1) - // Client 1 has received the WebStorage notification that the write - // has been acknowledged, but failed to process the change. Hence, - // we did not get a user callback. We schedule the first retry and - // make sure that it also does not get processed until - // `recoverDatabase` is called. - .runTimer(TimerId.AsyncQueueRetry) - .recoverDatabase() - .runTimer(TimerId.AsyncQueueRetry) - .expectUserCallbacks({ - acknowledged: ['collection/a'] - }) - ); - } - ); +describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => { + specTest( + 'Write is acknowledged by primary client (with recovery)', + ['multi-client'], + () => { + return ( + client(0) + .expectPrimaryState(true) + .client(1) + .expectPrimaryState(false) + .userSets('collection/a', { v: 1 }) + .failDatabase() + .client(0) + .writeAcks('collection/a', 1, { expectUserCallback: false }) + .client(1) + // Client 1 has received the WebStorage notification that the write + // has been acknowledged, but failed to process the change. Hence, + // we did not get a user callback. We schedule the first retry and + // make sure that it also does not get processed until + // `recoverDatabase` is called. + .runTimer(TimerId.AsyncQueueRetry) + .recoverDatabase() + .runTimer(TimerId.AsyncQueueRetry) + .expectUserCallbacks({ + acknowledged: ['collection/a'] + }) + ); + } + ); + + specTest( + 'Query raises events in secondary client (with recovery)', + ['multi-client'], + () => { + const query = Query.atPath(path('collection')); - specTest( - 'Query raises events in secondary client (with recovery)', - ['multi-client'], - () => { - const query = Query.atPath(path('collection')); + return client(0) + .expectPrimaryState(true) + .client(1) + .expectPrimaryState(false) + .userListens(query) + .failDatabase() + .client(0) + .expectListen(query) + .watchAcksFull(query, 1000) + .client(1) + .recoverDatabase() + .runTimer(TimerId.AsyncQueueRetry) + .expectEvents(query, {}); + } + ); - return client(0) + specTest( + 'Query is listened to by primary (with recovery)', + ['multi-client'], + () => { + const query = Query.atPath(path('collection')); + + return ( + client(0) .expectPrimaryState(true) + .failDatabase() .client(1) - .expectPrimaryState(false) .userListens(query) - .failDatabase() .client(0) + // The primary client 0 receives a WebStorage notification about the + // new query, but it cannot load the target from IndexedDB. The + // query will only be listened to once we recover the database. + .recoverDatabase() + .runTimer(TimerId.AsyncQueueRetry) .expectListen(query) - .watchAcksFull(query, 1000) + .failDatabase() .client(1) + .userUnlistens(query) + .client(0) + // The primary client 0 receives a notification that the query can + // be released, but it can only process the change after we recover + // the database. + .expectActiveTargets({ query }) .recoverDatabase() .runTimer(TimerId.AsyncQueueRetry) - .expectEvents(query, {}); - } - ); + .expectActiveTargets() + ); + } + ); - specTest( - 'Query is listened to by primary (with recovery)', - ['multi-client'], - () => { - const query = Query.atPath(path('collection')); + specTest('Recovers when write cannot be persisted', [], () => { + return spec() + .userSets('collection/key1', { foo: 'a' }) + .expectNumOutstandingWrites(1) + .failDatabase() + .userSets('collection/key2', { bar: 'b' }) + .expectUserCallbacks({ rejected: ['collection/key2'] }) + .recoverDatabase() + .expectNumOutstandingWrites(1) + .userSets('collection/key3', { baz: 'c' }) + .expectNumOutstandingWrites(2) + .writeAcks('collection/key1', 1) + .writeAcks('collection/key3', 2) + .expectNumOutstandingWrites(0); + }); - return ( - client(0) - .expectPrimaryState(true) - .failDatabase() - .client(1) - .userListens(query) - .client(0) - // The primary client 0 receives a WebStorage notification about the - // new query, but it cannot load the target from IndexedDB. The - // query will only be listened to once we recover the database. - .recoverDatabase() - .runTimer(TimerId.AsyncQueueRetry) - .expectListen(query) - .failDatabase() - .client(1) - .userUnlistens(query) - .client(0) - // The primary client 0 receives a notification that the query can - // be released, but it can only process the change after we recover - // the database. - .expectActiveTargets({ query }) - .recoverDatabase() - .runTimer(TimerId.AsyncQueueRetry) - .expectActiveTargets() - ); - } + specTest('Does not surface non-persisted writes', [], () => { + const query = Query.atPath(path('collection')); + const doc1Local = doc( + 'collection/key1', + 0, + { foo: 'a' }, + { hasLocalMutations: true } + ); + const doc1 = doc('collection/key1', 1, { foo: 'a' }); + const doc3Local = doc( + 'collection/key3', + 0, + { foo: 'c' }, + { hasLocalMutations: true } ); - } -); + const doc3 = doc('collection/key3', 2, { foo: 'c' }); + return spec() + .userListens(query) + .userSets('collection/key1', { foo: 'a' }) + .expectEvents(query, { + added: [doc1Local], + fromCache: true, + hasPendingWrites: true + }) + .failDatabase() + .userSets('collection/key2', { foo: 'b' }) + .expectUserCallbacks({ rejected: ['collection/key2'] }) + .recoverDatabase() + .userSets('collection/key3', { foo: 'c' }) + .expectEvents(query, { + added: [doc3Local], + fromCache: true, + hasPendingWrites: true + }) + .writeAcks('collection/key1', 1) + .writeAcks('collection/key3', 2) + .watchAcksFull(query, 2, doc1, doc3) + .expectEvents(query, { metadata: [doc1, doc3] }); + }); +}); diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 8e4438508ee..4a530aed0a0 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -687,7 +687,9 @@ abstract class TestRunner { () => this.rejectedDocs.push(...documentKeys) ); - this.sharedWrites.push(mutations); + if (!this.persistence.injectFailures) { + this.sharedWrites.push(mutations); + } return this.queue.enqueue(() => { return this.syncEngine.write(mutations, syncEngineCallback);