diff --git a/packages/firestore/src/local/indexeddb_mutation_queue.ts b/packages/firestore/src/local/indexeddb_mutation_queue.ts index cd19c0078bc..d1bbfd52678 100644 --- a/packages/firestore/src/local/indexeddb_mutation_queue.ts +++ b/packages/firestore/src/local/indexeddb_mutation_queue.ts @@ -473,46 +473,44 @@ export class IndexedDbMutationQueue implements MutationQueue { return PersistencePromise.waitFor(promises).next(() => results); } - removeMutationBatches( + removeMutationBatch( transaction: PersistenceTransaction, - batches: MutationBatch[] + batch: MutationBatch ): PersistencePromise { const mutationStore = mutationsStore(transaction); const indexTxn = documentMutationsStore(transaction); const promises: Array> = []; - for (const batch of batches) { - const range = IDBKeyRange.only(batch.batchId); - let numDeleted = 0; - const removePromise = mutationStore.iterate( - { range }, - (key, value, control) => { - numDeleted++; - return control.delete(); - } - ); - promises.push( - removePromise.next(() => { - assert( - numDeleted === 1, - 'Dangling document-mutation reference found: Missing batch ' + - batch.batchId - ); - }) - ); - for (const mutation of batch.mutations) { - const indexKey = DbDocumentMutation.key( - this.userId, - mutation.key.path, - batch.batchId + const range = IDBKeyRange.only(batch.batchId); + let numDeleted = 0; + const removePromise = mutationStore.iterate( + { range }, + (key, value, control) => { + numDeleted++; + return control.delete(); + } + ); + promises.push( + removePromise.next(() => { + assert( + numDeleted === 1, + 'Dangling document-mutation reference found: Missing batch ' + + batch.batchId ); - this.removeCachedMutationKeys(batch.batchId); - promises.push(indexTxn.delete(indexKey)); - if (this.garbageCollector !== null) { - this.garbageCollector.addPotentialGarbageKey(mutation.key); - } + }) + ); + for (const mutation of batch.mutations) { + const indexKey = DbDocumentMutation.key( + this.userId, + mutation.key.path, + batch.batchId + ); + promises.push(indexTxn.delete(indexKey)); + if (this.garbageCollector !== null) { + this.garbageCollector.addPotentialGarbageKey(mutation.key); } } + this.removeCachedMutationKeys(batch.batchId); return PersistencePromise.waitFor(promises); } diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts index de8059a04ba..67637e768de 100644 --- a/packages/firestore/src/local/local_store.ts +++ b/packages/firestore/src/local/local_store.ts @@ -302,11 +302,11 @@ export class LocalStore { } }) .next(ackedBatches => { - if (ackedBatches.length > 0) { - return this.mutationQueue.removeMutationBatches(txn, ackedBatches); - } else { - return PersistencePromise.resolve(); - } + let p = PersistencePromise.resolve(); + ackedBatches.forEach(batch => { + p = p.next(() => this.mutationQueue.removeMutationBatch(txn, batch)); + }); + return p; }); } @@ -979,16 +979,17 @@ export class LocalStore { batches: MutationBatch[] ): PersistencePromise { let affectedDocs = documentKeySet(); + + let p = PersistencePromise.resolve(); for (const batch of batches) { for (const mutation of batch.mutations) { const key = mutation.key; affectedDocs = affectedDocs.add(key); } + p = p.next(() => this.mutationQueue.removeMutationBatch(txn, batch)); } - return this.mutationQueue - .removeMutationBatches(txn, batches) - .next(() => affectedDocs); + return p.next(() => affectedDocs); } private applyWriteToRemoteDocuments( diff --git a/packages/firestore/src/local/memory_mutation_queue.ts b/packages/firestore/src/local/memory_mutation_queue.ts index ba5af955576..4607214735d 100644 --- a/packages/firestore/src/local/memory_mutation_queue.ts +++ b/packages/firestore/src/local/memory_mutation_queue.ts @@ -328,73 +328,45 @@ export class MemoryMutationQueue implements MutationQueue { return result; } - removeMutationBatches( + removeMutationBatch( transaction: PersistenceTransaction, - batches: MutationBatch[] + batch: MutationBatch ): PersistencePromise { - const batchCount = batches.length; - assert(batchCount > 0, 'Should not remove mutations when none exist.'); - - const firstBatchId = batches[0].batchId; - const queueCount = this.mutationQueue.length; - // Find the position of the first batch for removal. This need not be the // first entry in the queue. - const startIndex = this.indexOfExistingBatchId(firstBatchId, 'removed'); + const batchIndex = this.indexOfExistingBatchId(batch.batchId, 'removed'); assert( - this.mutationQueue[startIndex].batchId === firstBatchId, + this.mutationQueue[batchIndex].batchId === batch.batchId, 'Removed batches must exist in the queue' ); - // Check that removed batches are contiguous (while excluding tombstones). - let batchIndex = 1; - let queueIndex = startIndex + 1; - while (batchIndex < batchCount && queueIndex < queueCount) { - const batch = this.mutationQueue[queueIndex]; - if (batch.isTombstone()) { - queueIndex++; - continue; - } - - assert( - batch.batchId === batches[batchIndex].batchId, - 'Removed batches must be contiguous in the queue' - ); - batchIndex++; - queueIndex++; - } - // Only actually remove batches if removing at the front of the queue. // Previously rejected batches may have left tombstones in the queue, so // expand the removal range to include any tombstones. - if (startIndex === 0) { - for (; queueIndex < queueCount; queueIndex++) { - const batch = this.mutationQueue[queueIndex]; + if (batchIndex === 0) { + let endIndex = 1; + for (; endIndex < this.mutationQueue.length; endIndex++) { + const batch = this.mutationQueue[endIndex]; if (!batch.isTombstone()) { break; } } - const length = queueIndex - startIndex; - this.mutationQueue.splice(startIndex, length); + this.mutationQueue.splice(0, endIndex); } else { - // Mark the tombstones - for (let i = startIndex; i < queueIndex; i++) { - this.mutationQueue[i] = this.mutationQueue[i].toTombstone(); - } + this.mutationQueue[batchIndex] = this.mutationQueue[ + batchIndex + ].toTombstone(); } let references = this.batchesByDocumentKey; - for (const batch of batches) { - const batchId = batch.batchId; - for (const mutation of batch.mutations) { - const key = mutation.key; - if (this.garbageCollector !== null) { - this.garbageCollector.addPotentialGarbageKey(key); - } - - const ref = new DocReference(key, batchId); - references = references.delete(ref); + for (const mutation of batch.mutations) { + const key = mutation.key; + if (this.garbageCollector !== null) { + this.garbageCollector.addPotentialGarbageKey(key); } + + const ref = new DocReference(key, batch.batchId); + references = references.delete(ref); } this.batchesByDocumentKey = references; return PersistencePromise.resolve(); diff --git a/packages/firestore/src/local/mutation_queue.ts b/packages/firestore/src/local/mutation_queue.ts index 84946fb02d0..0da93d274db 100644 --- a/packages/firestore/src/local/mutation_queue.ts +++ b/packages/firestore/src/local/mutation_queue.ts @@ -197,21 +197,17 @@ export interface MutationQueue extends GarbageSource { ): PersistencePromise; /** - * Removes the given mutation batches from the queue. This is useful in two + * Removes the given mutation batch from the queue. This is useful in two * circumstances: * - * + Removing applied mutations from the head of the queue - * + Removing rejected mutations from anywhere in the queue - * - * In both cases, the array of mutations to remove must be a contiguous range - * of batchIds. This is most easily accomplished by loading mutations with - * getAllMutationBatchesThroughBatchId() + * + Removing an applied mutation from the head of the queue + * + Removing a rejected mutation from anywhere in the queue * * Multi-Tab Note: This operation should only be called by the primary client. */ - removeMutationBatches( + removeMutationBatch( transaction: PersistenceTransaction, - batches: MutationBatch[] + batch: MutationBatch ): PersistencePromise; /** diff --git a/packages/firestore/test/unit/local/mutation_queue.test.ts b/packages/firestore/test/unit/local/mutation_queue.test.ts index ec6d91f27a0..f985c3bae38 100644 --- a/packages/firestore/test/unit/local/mutation_queue.test.ts +++ b/packages/firestore/test/unit/local/mutation_queue.test.ts @@ -127,7 +127,7 @@ function genericMutationQueueTests(): void { for (let i = 0; i < holes.length; i++) { const index = holes[i] - i; const batch = batches[index]; - await mutationQueue.removeMutationBatches([batch]); + await mutationQueue.removeMutationBatch(batch); batches.splice(index, 1); removed.push(batch); @@ -146,10 +146,10 @@ function genericMutationQueueTests(): void { const batch2 = await addMutationBatch(); expect(await mutationQueue.countBatches()).to.equal(2); - await mutationQueue.removeMutationBatches([batch2]); + await mutationQueue.removeMutationBatch(batch2); expect(await mutationQueue.countBatches()).to.equal(1); - await mutationQueue.removeMutationBatches([batch1]); + await mutationQueue.removeMutationBatch(batch1); expect(await mutationQueue.countBatches()).to.equal(0); }); @@ -181,18 +181,18 @@ function genericMutationQueueTests(): void { batch2.batchId ); - await mutationQueue.removeMutationBatches([batch1]); + await mutationQueue.removeMutationBatch(batch1); expect(await mutationQueue.getHighestAcknowledgedBatchId()).to.equal( batch2.batchId ); - await mutationQueue.removeMutationBatches([batch2]); + await mutationQueue.removeMutationBatch(batch2); expect(await mutationQueue.getHighestAcknowledgedBatchId()).to.equal( batch2.batchId ); // Batch 3 never acknowledged. - await mutationQueue.removeMutationBatches([batch3]); + await mutationQueue.removeMutationBatch(batch3); expect(await mutationQueue.getHighestAcknowledgedBatchId()).to.equal( batch2.batchId ); @@ -206,7 +206,7 @@ function genericMutationQueueTests(): void { ); await mutationQueue.acknowledgeBatch(batch1, emptyByteString()); - await mutationQueue.removeMutationBatches([batch1]); + await mutationQueue.removeMutationBatch(batch1); expect(await mutationQueue.countBatches()).to.equal(0); expect(await mutationQueue.getHighestAcknowledgedBatchId()).to.equal( @@ -226,7 +226,8 @@ function genericMutationQueueTests(): void { batch2.batchId ); - await mutationQueue.removeMutationBatches([batch1, batch2]); + await mutationQueue.removeMutationBatch(batch1); + await mutationQueue.removeMutationBatch(batch2); expect(await mutationQueue.getHighestAcknowledgedBatchId()).to.equal( batch2.batchId ); @@ -461,16 +462,17 @@ function genericMutationQueueTests(): void { await addMutationBatch('foo/baz') ]; - await mutationQueue.removeMutationBatches([batches[0]]); + await mutationQueue.removeMutationBatch(batches[0]); expectSetToEqual(await mutationQueue.collectGarbage(gc), []); - await mutationQueue.removeMutationBatches([batches[1]]); + await mutationQueue.removeMutationBatch(batches[1]); expectSetToEqual(await mutationQueue.collectGarbage(gc), [key('foo/ba')]); - await mutationQueue.removeMutationBatches([batches[5]]); + await mutationQueue.removeMutationBatch(batches[5]); expectSetToEqual(await mutationQueue.collectGarbage(gc), [key('foo/baz')]); - await mutationQueue.removeMutationBatches([batches[2], batches[3]]); + await mutationQueue.removeMutationBatch(batches[2]); + await mutationQueue.removeMutationBatch(batches[3]); expectSetToEqual(await mutationQueue.collectGarbage(gc), [ key('foo/bar'), key('foo/bar2') @@ -479,7 +481,8 @@ function genericMutationQueueTests(): void { batches.push(await addMutationBatch('foo/bar/suffix/baz')); expectSetToEqual(await mutationQueue.collectGarbage(gc), []); - await mutationQueue.removeMutationBatches([batches[4], batches[6]]); + await mutationQueue.removeMutationBatch(batches[4]); + await mutationQueue.removeMutationBatch(batches[6]); expectSetToEqual(await mutationQueue.collectGarbage(gc), [ key('foo/bar/suffix/baz') ]); @@ -509,11 +512,11 @@ function genericMutationQueueTests(): void { ); }); - it('can removeMutationBatches()', async () => { + it('can removeMutationBatch()', async () => { const batches = await createBatches(10); const last = batches[batches.length - 1]; - await mutationQueue.removeMutationBatches([batches[0]]); + await mutationQueue.removeMutationBatch(batches[0]); batches.splice(0, 1); expect(await mutationQueue.countBatches()).to.equal(9); @@ -525,11 +528,9 @@ function genericMutationQueueTests(): void { expectEqualArrays(found, batches); expect(found.length).to.equal(9); - await mutationQueue.removeMutationBatches([ - batches[0], - batches[1], - batches[2] - ]); + await mutationQueue.removeMutationBatch(batches[0]); + await mutationQueue.removeMutationBatch(batches[1]); + await mutationQueue.removeMutationBatch(batches[2]); batches.splice(0, 3); expect(await mutationQueue.countBatches()).to.equal(6); @@ -539,7 +540,7 @@ function genericMutationQueueTests(): void { expectEqualArrays(found, batches); expect(found.length).to.equal(6); - await mutationQueue.removeMutationBatches([batches[batches.length - 1]]); + await mutationQueue.removeMutationBatch(batches[batches.length - 1]); batches.splice(batches.length - 1, 1); expect(await mutationQueue.countBatches()).to.equal(5); @@ -549,11 +550,11 @@ function genericMutationQueueTests(): void { expectEqualArrays(found, batches); expect(found.length).to.equal(5); - await mutationQueue.removeMutationBatches([batches[3]]); + await mutationQueue.removeMutationBatch(batches[3]); batches.splice(3, 1); expect(await mutationQueue.countBatches()).to.equal(4); - await mutationQueue.removeMutationBatches([batches[1]]); + await mutationQueue.removeMutationBatch(batches[1]); batches.splice(1, 1); expect(await mutationQueue.countBatches()).to.equal(3); @@ -564,10 +565,10 @@ function genericMutationQueueTests(): void { expect(found.length).to.equal(3); expect(await mutationQueue.checkEmpty()).to.equal(false); - await mutationQueue.removeMutationBatches(batches); - found = await mutationQueue.getAllMutationBatchesThroughBatchId( - last.batchId - ); + for (const batch of batches) { + await mutationQueue.removeMutationBatch(batch); + } + found = await mutationQueue.getAllMutationBatches(); expectEqualArrays(found, []); expect(found.length).to.equal(0); expect(await mutationQueue.checkEmpty()).to.equal(true); diff --git a/packages/firestore/test/unit/local/test_mutation_queue.ts b/packages/firestore/test/unit/local/test_mutation_queue.ts index 265e287a730..5d1937e7bfd 100644 --- a/packages/firestore/test/unit/local/test_mutation_queue.ts +++ b/packages/firestore/test/unit/local/test_mutation_queue.ts @@ -178,14 +178,10 @@ export class TestMutationQueue { ); } - removeMutationBatches(batches: MutationBatch[]): Promise { - return this.persistence.runTransaction( - 'removeMutationBatches', - true, - txn => { - return this.queue.removeMutationBatches(txn, batches); - } - ); + removeMutationBatch(batch: MutationBatch): Promise { + return this.persistence.runTransaction('removeMutationBatch', true, txn => { + return this.queue.removeMutationBatch(txn, batch); + }); } collectGarbage(gc: GarbageCollector): Promise {