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 4 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
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'
RetryTransaction = 'retry_transaction',

/**
* 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.resolve();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need a type of Promise<unknown>, as on tail, above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All retryable operations return void (to make sure users don't wait for results). Promise.resolve() returns Promise<void as a type implicitly, but I made it explicit by adding the type here.


// 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
71 changes: 71 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,71 @@
/**
* @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()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are fail and recover instructions issued from client 1? Does it matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Client 0 is the primary client and writes into WebStorage. Client1 is the secondary client, reads from WebStorage and fails to apply the changes. The code in this PR only deals with the behavior in secondary clients.

.client(0)
.writeAcks('collection/a', 1, { expectUserCallback: false })
.client(1)
.recoverDatabase()
.runTimer(TimerId.AsyncQueueRetry)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be useful to see that retry can happen multiple times (i.e. that running the timer before recovery does not raise an event but still does raise an event after recovery).

.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, {});
}
);
}
);
16 changes: 16 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,22 @@ export class SpecBuilder {
return this;
}

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;
}

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