Skip to content

Retry WebStorage operations #2879

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.

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 8 commits into from
Apr 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions packages/firestore/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
# Unreleased
- [fixed] Fixed a source of IndexedDB-related crashes for tabs that receive
multi-tab notifications while the file system is locked.

# 1.10.2
- [fixed] Temporarily reverted the use of window.crypto to generate document
IDs to address compatibility issues with IE 11, WebWorkers, and React Native.
- [changed] Firestore now limits the number of concurrent document lookups it
Expand Down
18 changes: 0 additions & 18 deletions packages/firestore/src/core/component_provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ import { IndexFreeQueryEngine } from '../local/index_free_query_engine';
import { IndexedDbPersistence } from '../local/indexeddb_persistence';
import {
MemoryEagerDelegate,
MemoryLruDelegate,
MemoryPersistence
} from '../local/memory_persistence';

Expand Down Expand Up @@ -189,23 +188,6 @@ export class MemoryComponentProvider {
}
}

/**
* Provides all components needed for Firestore with in-memory persistence.
* Uses LRU garbage collection.
*/
export class MemoryLruComponentProvider extends MemoryComponentProvider {
createPersistence(cfg: ComponentConfiguration): Persistence {
debugAssert(
!cfg.persistenceSettings.durable,
'Can only start memory persistence'
);
return new MemoryPersistence(
cfg.clientId,
p => new MemoryLruDelegate(p, LruParams.DEFAULT)
);
}
}

/**
* Provides all components needed for Firestore with IndexedDB persistence.
*/
Expand Down
4 changes: 2 additions & 2 deletions packages/firestore/src/core/transaction_runner.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2019 Google Inc.
* Copyright 2019 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -42,7 +42,7 @@ export class TransactionRunner<T> {
) {
this.backoff = new ExponentialBackoff(
this.asyncQueue,
TimerId.RetryTransaction
TimerId.TransactionRetry
);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/local/shared_client_state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,7 @@ export class WebStorageSharedClientState implements SharedClientState {
return;
}

this.queue.enqueueAndForget(async () => {
this.queue.enqueueRetryable(async () => {
if (!this.started) {
this.earlyEvents.push(event);
return;
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/remote/backoff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export class ExponentialBackoff {
desiredDelayWithJitterMs - delaySoFarMs
);

if (this.currentBaseMs > 0) {
if (remainingDelayMs > 0) {
logDebug(
LOG_TAG,
`Backing off for ${remainingDelayMs} ms ` +
Expand Down
77 changes: 69 additions & 8 deletions packages/firestore/src/util/async_queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@

import { debugAssert, fail } from './assert';
import { Code, FirestoreError } from './error';
import { logError } from './log';
import { logDebug, logError } from './log';
import { CancelablePromise, Deferred } from './promise';
import { ExponentialBackoff } from '../remote/backoff';
import { PlatformSupport } from '../platform/platform';

const LOG_TAG = 'AsyncQueue';

// Accept any return type from setTimeout().
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Expand Down Expand Up @@ -66,7 +70,13 @@ export const enum TimerId {
* A timer used to retry transactions. Since there can be multiple concurrent
* transactions, multiple of these may be in the queue at a given time.
*/
RetryTransaction = 'retry_transaction'
TransactionRetry = 'transaction_retry',

/**
* A timer used to retry operations scheduled via retryable AsyncQueue
* operations.
*/
AsyncQueueRetry = 'async_queue_retry'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the inconsistency between "retry" at the beginning vs end of the enumeration entry is a little glaring. RetryAsyncQueue sounds a little weird. Maybe rename RetryTransaction to TransactionRetry to match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed RetryTransaction.

}

/**
Expand Down Expand Up @@ -195,6 +205,10 @@ export class AsyncQueue {
// The last promise in the queue.
private tail: Promise<unknown> = Promise.resolve();

// The last retryable operation. Retryable operation are run in order and
// retried with backoff.
private retryableTail: Promise<void> = Promise.resolve();

// Is this AsyncQueue being shut down? Once it is set to true, it will not
// be changed again.
private _isShuttingDown: boolean = false;
Expand All @@ -213,6 +227,24 @@ export class AsyncQueue {
// List of TimerIds to fast-forward delays for.
private timerIdsToSkip: TimerId[] = [];

// Backoff timer used to schedule retries for retryable operations
private backoff = new ExponentialBackoff(this, TimerId.AsyncQueueRetry);

// Visibility handler that triggers an immediate retry of all retryable
// operations. Meant to speed up recovery when we regain file system access
// after page comes into foreground.
private visibilityHandler = (): void => {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this.runDelayedOperationsEarly(TimerId.AsyncQueueRetry);
};

constructor() {
const window = PlatformSupport.getPlatform().window;
if (window) {
window.addEventListener('visibilitychange', this.visibilityHandler);
}
}

// Is this AsyncQueue being shut down? If true, this instance will not enqueue
// any new operations, Promises from enqueue requests will not resolve.
get isShuttingDown(): boolean {
Expand Down Expand Up @@ -262,6 +294,10 @@ export class AsyncQueue {
this.verifyNotFailed();
if (!this._isShuttingDown) {
this._isShuttingDown = true;
const window = PlatformSupport.getPlatform().window;
if (window) {
window.removeEventListener('visibilitychange', this.visibilityHandler);
}
await this.enqueueEvenAfterShutdown(op);
}
}
Expand All @@ -279,6 +315,37 @@ export class AsyncQueue {
return this.enqueueInternal(op);
}

/**
* Enqueue a retryable operation.
*
* A retryable operation is rescheduled with backoff if it fails with any
* exception. All retryable operations are executed in order and only run
* if all prior operations were retried successfully.
*/
enqueueRetryable(op: () => Promise<void>): void {
this.verifyNotFailed();

if (this._isShuttingDown) {
return;
}

this.retryableTail = this.retryableTail.then(() => {
const deferred = new Deferred<void>();
const retryingOp = async (): Promise<void> => {
try {
await op();
deferred.resolve();
this.backoff.reset();
} catch (e) {
logDebug(LOG_TAG, 'Retryable operation failed: ' + e.message);
this.backoff.backoffAndRun(retryingOp);
}
};
this.enqueueAndForget(retryingOp);
return deferred.promise;
});
}

private enqueueInternal<T extends unknown>(op: () => Promise<T>): Promise<T> {
const newTail = this.tail.then(() => {
this.operationInProgress = true;
Expand Down Expand Up @@ -399,12 +466,6 @@ export class AsyncQueue {
runDelayedOperationsEarly(lastTimerId: TimerId): Promise<void> {
// Note that draining may generate more delayed ops, so we do that first.
return this.drain().then(() => {
debugAssert(
lastTimerId === TimerId.All ||
this.containsDelayedOperation(lastTimerId),
`Attempted to drain to missing operation ${lastTimerId}`
);

// Run ops in the same order they'd run if they ran naturally.
this.delayedOperations.sort((a, b) => a.targetTimeMs - b.targetTimeMs);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2019 Google Inc.
* Copyright 2019 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -36,7 +36,7 @@ apiDescribe(
let started = 0;

return integrationHelpers.withTestDb(persistence, db => {
asyncQueue(db).skipDelaysForTimerId(TimerId.RetryTransaction);
asyncQueue(db).skipDelaysForTimerId(TimerId.TransactionRetry);
const doc = db.collection('counters').doc();
return doc
.set({
Expand Down Expand Up @@ -93,7 +93,7 @@ apiDescribe(
let counter = 0;

return integrationHelpers.withTestDb(persistence, db => {
asyncQueue(db).skipDelaysForTimerId(TimerId.RetryTransaction);
asyncQueue(db).skipDelaysForTimerId(TimerId.TransactionRetry);
const doc = db.collection('counters').doc();
return doc
.set({
Expand Down Expand Up @@ -148,7 +148,7 @@ apiDescribe(

it('handle reading a doc twice with different versions', () => {
return integrationHelpers.withTestDb(persistence, db => {
asyncQueue(db).skipDelaysForTimerId(TimerId.RetryTransaction);
asyncQueue(db).skipDelaysForTimerId(TimerId.TransactionRetry);
const doc = db.collection('counters').doc();
let counter = 0;
return doc
Expand Down
79 changes: 79 additions & 0 deletions packages/firestore/test/unit/specs/recovery_spec.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/**
* @license
* Copyright 2020 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { describeSpec, specTest } from './describe_spec';
import { client } from './spec_builder';
import { TimerId } from '../../../src/util/async_queue';
import { Query } from '../../../src/core/query';
import { 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']
})
);
}
);

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, {});
}
);
}
);
18 changes: 18 additions & 0 deletions packages/firestore/test/unit/specs/spec_builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,24 @@ export class SpecBuilder {
return this;
}

/** Fails all database operations until `recoverDatabase()` is called. */
failDatabase(): this {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add comments here about how this works? I suspect this works by modifying all steps going forward until recoverDatabase is called, but the code here suggests this merely modifies the current step only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ends up calling doFailDatabase() which flips injectFailures. injectFailures is long-lived and tied to the lifetime of the persistence implementation. I added a short comment.

this.nextStep();
this.currentStep = {
failDatabase: true
};
return this;
}

/** Stops failing database operations. */
recoverDatabase(): this {
this.nextStep();
this.currentStep = {
failDatabase: false
};
return this;
}

expectIsShutdown(): this {
this.assertStep('Active target expectation requires previous step');
const currentStep = this.currentStep!;
Expand Down
Loading