Skip to content

Only retry DOMException/DOMError #2919

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 7 commits into from
Apr 20, 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: 2 additions & 2 deletions packages/firestore/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
"test:all": "run-p test:browser test:travis test:minified",
"test:browser": "karma start --single-run",
"test:browser:debug": "karma start --browsers=Chrome --auto-watch",
"test:node": "FIRESTORE_EMULATOR_PORT=8080 FIRESTORE_EMULATOR_PROJECT_ID=test-emulator TS_NODE_CACHE=NO TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' nyc --reporter lcovonly -- mocha 'test/{,!(browser)/**/}*.test.ts' --file index.node.ts --opts ../../config/mocha.node.opts",
"test:node:prod": "TS_NODE_CACHE=NO TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' nyc --reporter lcovonly -- mocha 'test/{,!(browser)/**/}*.test.ts' --file index.node.ts --opts ../../config/mocha.node.opts",
"test:node": "FIRESTORE_EMULATOR_PORT=8080 FIRESTORE_EMULATOR_PROJECT_ID=test-emulator TS_NODE_CACHE=NO TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' nyc --reporter lcovonly -- mocha 'test/{,!(browser)/**/}*.test.ts' --file index.node.ts --file test/util/node_persistence.ts --opts ../../config/mocha.node.opts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While this seems wrong, this is needed to pull in DOMException. Since USE_MOCK_PERSISTENCE is not set, there are no other side effects. I chose not to rename this file since it is used in our test launchers.

"test:node:prod": "TS_NODE_CACHE=NO TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' nyc --reporter lcovonly -- mocha 'test/{,!(browser)/**/}*.test.ts' --file index.node.ts --file test/util/node_persistence.ts --opts ../../config/mocha.node.opts",
"test:node:persistence": "FIRESTORE_EMULATOR_PORT=8080 FIRESTORE_EMULATOR_PROJECT_ID=test-emulator USE_MOCK_PERSISTENCE=YES TS_NODE_CACHE=NO TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' nyc --reporter lcovonly -- mocha 'test/{,!(browser)/**/}*.test.ts' --require ts-node/register --require index.node.ts --require test/util/node_persistence.ts --opts ../../config/mocha.node.opts",
"test:node:persistence:prod": "USE_MOCK_PERSISTENCE=YES TS_NODE_CACHE=NO TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' nyc --reporter lcovonly -- mocha 'test/{,!(browser)/**/}*.test.ts' --require ts-node/register --require index.node.ts --require test/util/node_persistence.ts --opts ../../config/mocha.node.opts",
"test:travis": "ts-node --compiler-options='{\"module\":\"commonjs\"}' ../../scripts/emulator-testing/firestore-test-runner.ts",
Expand Down
22 changes: 17 additions & 5 deletions packages/firestore/src/util/async_queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,9 +318,10 @@ export class AsyncQueue {
/**
* 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.
* A retryable operation is rescheduled with backoff if it fails with a
* DOMException or a DOMError (the error types used by IndexedDB). All
* retryable operations are executed in order and only run if all prior
* operations were retried successfully.
*/
enqueueRetryable(op: () => Promise<void>): void {
this.verifyNotFailed();
Expand All @@ -337,8 +338,19 @@ export class AsyncQueue {
deferred.resolve();
this.backoff.reset();
} catch (e) {
logDebug(LOG_TAG, 'Retryable operation failed: ' + e.message);
this.backoff.backoffAndRun(retryingOp);
// We only retry errors that match the ones thrown by IndexedDB (see
// https://developer.mozilla.org/en-US/docs/Web/API/IDBTransaction/error)
if (
(typeof DOMException !== 'undefined' &&
Copy link
Contributor

Choose a reason for hiding this comment

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

DOMException can mean just about any failure in the browser. A better way to handle this would be to take the transaction.error property and wrap it in our own PersistenceError error type (or similar). That way it's unambiguous that it's an error that came from IndexedDB. This also sidesteps the issue of the availability of the DOMException type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I am now wrapping exceptions thrown by abort and error in their own types.

e instanceof DOMException) ||
(typeof DOMError !== 'undefined' && e instanceof DOMError)
) {
logDebug(LOG_TAG, 'Operation failed with retryable error: ' + e);
this.backoff.backoffAndRun(retryingOp);
} else {
deferred.resolve();
throw e; // Failure will be handled by AsyncQueue
}
}
};
this.enqueueAndForget(retryingOp);
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/unit/specs/recovery_spec.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describeSpec(
// 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
// make sure that it also does not get processed until
// `recoverDatabase` is called.
.runTimer(TimerId.AsyncQueueRetry)
.recoverDatabase()
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/unit/specs/spec_test_components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ export class MockPersistence implements Persistence {
) => PersistencePromise<T>
): Promise<T> {
if (this.injectFailures) {
return Promise.reject(new Error('Injected Failure'));
return Promise.reject(new DOMException('Simulated retryable error'));
} else {
return this.delegate.runTransaction(action, mode, transactionOperation);
}
Expand Down
29 changes: 25 additions & 4 deletions packages/firestore/test/unit/util/async_queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,16 @@
* limitations under the License.
*/

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

import { expect, use } from 'chai';
import { AsyncQueue, TimerId } from '../../../src/util/async_queue';
import { Code } from '../../../src/util/error';
import { getLogLevel, setLogLevel, LogLevel } from '../../../src/util/log';
import { Deferred, Rejecter, Resolver } from '../../../src/util/promise';
import { fail } from '../../../src/util/assert';

use(chaiAsPromised);

describe('AsyncQueue', () => {
// We reuse these TimerIds for generic testing.
Expand Down Expand Up @@ -212,13 +217,29 @@ describe('AsyncQueue', () => {
queue.enqueueRetryable(async () => {
doStep(1);
if (completedSteps.length === 1) {
throw new Error('Simulated retryable error');
throw new DOMException('Simulated retryable error');
}
});
await queue.runDelayedOperationsEarly(TimerId.AsyncQueueRetry);
expect(completedSteps).to.deep.equal([1, 1]);
});

it("Doesn't retry internal exceptions", async () => {
const queue = new AsyncQueue();
// We use a deferred Promise as retryable operations are scheduled only
// when Promise chains are resolved, which can happen after the
// `queue.enqueue()` call below.
const deferred = new Deferred<void>();
queue.enqueueRetryable(async () => {
deferred.resolve();
throw fail('Simulated test failure');
});
await deferred.promise;
await expect(
queue.enqueue(() => Promise.resolve())
).to.eventually.be.rejectedWith('Simulated test failure');
});

it('Schedules first retryable attempt with no delay', async () => {
const queue = new AsyncQueue();
const completedSteps: number[] = [];
Expand All @@ -244,7 +265,7 @@ describe('AsyncQueue', () => {
queue.enqueueRetryable(async () => {
doStep(1);
if (completedSteps.length === 1) {
throw new Error('Simulated retryable error');
throw new DOMException('Simulated retryable error');
}
});

Expand All @@ -270,7 +291,7 @@ describe('AsyncQueue', () => {
doStep(1);
if (completedSteps.length > 1) {
} else {
throw new Error('Simulated retryable error');
throw new DOMException('Simulated retryable error');
}
});
queue.enqueueRetryable(async () => {
Expand Down
10 changes: 9 additions & 1 deletion packages/firestore/test/util/node_persistence.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2018 Google Inc.
* Copyright 2018 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 @@ -58,3 +58,11 @@ if (process.env.USE_MOCK_PERSISTENCE === 'YES') {

globalAny.Event = Event;
}

if (typeof DOMException === 'undefined') {
// Define DOMException as it is used in tests
class DOMException {
constructor(readonly message: string) {}
}
globalAny.DOMException = DOMException;
}