Skip to content

Add test for getHighestListenSequenceNumber() recovery #3083

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 21, 2020
Merged

Add test for getHighestListenSequenceNumber() recovery #3083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12ff025
Add test for getHighestListenSequenceNumber() recovery
schmidt-sebastian May 19, 2020
2f701bd
Simplify
schmidt-sebastian May 20, 2020
1a0638e
Merge
schmidt-sebastian May 21, 2020
15ec123
Add debugAssert
schmidt-sebastian May 21, 2020
a26b21f
Merge
schmidt-sebastian May 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions packages/firestore/src/local/indexeddb_persistence.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,11 @@ import {
DbPrimaryClient,
DbPrimaryClientKey,
DbTargetDocument,
DbTargetGlobal,
SCHEMA_VERSION,
SchemaConverter
} from './indexeddb_schema';
import {
documentTargetStore,
getHighestListenSequenceNumber,
IndexedDbTargetCache
} from './indexeddb_target_cache';
import { LocalSerializer } from './local_serializer';
Expand Down Expand Up @@ -306,10 +304,10 @@ export class IndexedDbPersistence implements Persistence {

this.scheduleClientMetadataAndPrimaryLeaseRefreshes();

return this.simpleDb.runTransaction(
return this.runTransaction(
'getHighestListenSequenceNumber',
'readonly',
[DbTargetGlobal.store],
txn => getHighestListenSequenceNumber(txn)
txn => this.targetCache.getHighestSequenceNumber(txn)
);
})
.then(highestListenSequenceNumber => {
Expand Down
12 changes: 9 additions & 3 deletions packages/firestore/src/local/indexeddb_schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import {
EncodedResourcePath
} from './encoded_resource_path';
import { removeMutationBatch } from './indexeddb_mutation_queue';
import { getHighestListenSequenceNumber } from './indexeddb_target_cache';
import { dbDocumentSize } from './indexeddb_remote_document_cache';
import { LocalSerializer } from './local_serializer';
import { MemoryCollectionParentIndex } from './memory_index_manager';
Expand Down Expand Up @@ -231,16 +230,23 @@ export class SchemaConverter implements SimpleDbSchemaConverter {
const documentsStore = txn.store<DbRemoteDocumentKey, DbRemoteDocument>(
DbRemoteDocument.store
);
const globalTargetStore = txn.store<DbTargetGlobalKey, DbTargetGlobal>(
DbTargetGlobal.store
);

return getHighestListenSequenceNumber(txn).next(currentSequenceNumber => {
return globalTargetStore.get(DbTargetGlobal.key).next(metadata => {
debugAssert(
!!metadata,
'Metadata should have been written during the version 3 migration'
);
const writeSentinelKey = (
path: ResourcePath
): PersistencePromise<void> => {
return documentTargetStore.put(
new DbTargetDocument(
0,
encodeResourcePath(path),
currentSequenceNumber
metadata!.highestListenSequenceNumber!
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We set the metadata earlier in the migration. It should exist at this point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wary of this style of commenting (review-only). The reviewer of this PR isn't the only one who's ever going to read this code.

Don't get me wrong--there's definitely a place for this kind of thing, but this comment in particular seems reasonable to just make in the code itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a debug assert that contains this message.

)
);
};
Expand Down
39 changes: 10 additions & 29 deletions packages/firestore/src/local/indexeddb_target_cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ import {
} from './encoded_resource_path';
import {
IndexedDbLruDelegate,
IndexedDbPersistence,
IndexedDbTransaction
IndexedDbPersistence
} from './indexeddb_persistence';
import {
DbTarget,
Expand All @@ -46,7 +45,7 @@ import { PersistenceTransaction } from './persistence';
import { PersistencePromise } from './persistence_promise';
import { TargetCache } from './target_cache';
import { TargetData } from './target_data';
import { SimpleDb, SimpleDbStore, SimpleDbTransaction } from './simple_db';
import { SimpleDbStore } from './simple_db';
import { Target } from '../core/target';

export class IndexedDbTargetCache implements TargetCache {
Expand Down Expand Up @@ -90,8 +89,8 @@ export class IndexedDbTargetCache implements TargetCache {
getHighestSequenceNumber(
transaction: PersistenceTransaction
): PersistencePromise<ListenSequenceNumber> {
return getHighestListenSequenceNumber(
(transaction as IndexedDbTransaction).simpleDbTransaction
return this.retrieveMetadata(transaction).next(
targetGlobal => targetGlobal.highestListenSequenceNumber
);
}

Expand Down Expand Up @@ -192,9 +191,12 @@ export class IndexedDbTargetCache implements TargetCache {
private retrieveMetadata(
transaction: PersistenceTransaction
): PersistencePromise<DbTargetGlobal> {
return retrieveMetadata(
(transaction as IndexedDbTransaction).simpleDbTransaction
);
return globalTargetStore(transaction)
.get(DbTargetGlobal.key)
.next(metadata => {
hardAssert(metadata !== null, 'Missing metadata row.');
return metadata;
});
}

private saveMetadata(
Expand Down Expand Up @@ -422,27 +424,6 @@ function globalTargetStore(
);
}

function retrieveMetadata(
txn: SimpleDbTransaction
): PersistencePromise<DbTargetGlobal> {
const globalStore = SimpleDb.getStore<DbTargetGlobalKey, DbTargetGlobal>(
txn,
DbTargetGlobal.store
);
return globalStore.get(DbTargetGlobal.key).next(metadata => {
hardAssert(metadata !== null, 'Missing metadata row.');
return metadata;
});
}

export function getHighestListenSequenceNumber(
txn: SimpleDbTransaction
): PersistencePromise<ListenSequenceNumber> {
return retrieveMetadata(txn).next(
targetGlobal => targetGlobal.highestListenSequenceNumber
);
}

/**
* Helper to get a typed SimpleDbStore for the document target object store.
*/
Expand Down
19 changes: 18 additions & 1 deletion packages/firestore/test/unit/local/indexeddb_persistence.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
* limitations under the License.
*/

import { expect } from 'chai';
import * as chaiAsPromised from 'chai-as-promised';

import { expect, use } from 'chai';
import { Query } from '../../../src/core/query';
import { SnapshotVersion } from '../../../src/core/snapshot_version';
import {
Expand Down Expand Up @@ -74,6 +76,8 @@ import {
TEST_SERIALIZER
} from './persistence_test_helpers';

use(chaiAsPromised);

/* eslint-disable no-restricted-globals */

function withDb(
Expand Down Expand Up @@ -1165,6 +1169,19 @@ describe('IndexedDb: allowTabSynchronization', () => {
);
});

it('blocks start() when getHighestListenSequenceNumber() fails', async () => {
await withUnstartedCustomPersistence(
'clientA',
/* multiClient= */ false,
async db1 => {
db1.injectFailures = ['getHighestListenSequenceNumber'];
await expect(db1.start()).to.eventually.be.rejectedWith(
'IndexedDB transaction failed'
);
}
);
});

it('ignores intermittent IndexedDbTransactionError during lease refresh', async () => {
await withPersistence('clientA', async (db, _, queue) => {
db.injectFailures = ['updateClientMetadataAndTryBecomePrimary'];
Expand Down
4 changes: 2 additions & 2 deletions packages/firestore/test/unit/specs/recovery_spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,12 +464,12 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
fromCache: true,
hasPendingWrites: true
})
.failDatabaseTransactions({ 'Handle user change': true })
.failDatabaseTransactions('Handle user change')
.changeUser('user2')
.recoverDatabase()
.runTimer(TimerId.AsyncQueueRetry)
.expectEvents(query, { removed: [doc1], fromCache: true })
.failDatabaseTransactions({ 'Handle user change': true })
.failDatabaseTransactions('Handle user change')
.changeUser('user1')
.recoverDatabase()
.runTimer(TimerId.AsyncQueueRetry)
Expand Down
3 changes: 2 additions & 1 deletion packages/firestore/test/unit/specs/spec_test_runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1225,7 +1225,8 @@ export type PersistenceAction =
| 'Get target data'
| 'Get new document changes'
| 'Synchronize last document change read time'
| 'updateClientMetadataAndTryBecomePrimary';
| 'updateClientMetadataAndTryBecomePrimary'
| 'getHighestListenSequenceNumber';

/**
* Union type for each step. The step consists of exactly one `field`
Expand Down