-
Notifications
You must be signed in to change notification settings - Fork 935
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
Changes from 4 commits
d3c8e55
4d7eb94
b79b9ca
d2fa82a
7b538dc
00af1f3
aba251d
0f07ebe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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' | ||
} | ||
|
||
/** | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need a type of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All retryable operations return |
||
|
||
// Is this AsyncQueue being shut down? Once it is set to true, it will not | ||
// be changed again. | ||
private _isShuttingDown: boolean = false; | ||
|
@@ -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 { | ||
|
@@ -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); | ||
} | ||
} | ||
|
@@ -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; | ||
|
@@ -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); | ||
|
||
|
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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, {}); | ||
} | ||
); | ||
} | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -418,6 +418,22 @@ export class SpecBuilder { | |
return this; | ||
} | ||
|
||
failDatabase(): this { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This ends up calling |
||
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!; | ||
|
There was a problem hiding this comment.
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 renameRetryTransaction
toTransactionRetry
to match?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed
RetryTransaction
.