Only retry DOMException/DOMError #2919
Conversation
Binary Size ReportAffected SDKs
Test Logs |
schmidt-sebastian
force-pushed
the
mrschmidt/limitretries
branch
from
bd7f13c to
1119bc3
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/limitretries
branch
from
a551569 to
7e3b47e
Compare
packages/firestore/package.json
Outdated
| @@ -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", | |||
There was a problem hiding this comment.
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.
| // 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' && |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done. I am now wrapping exceptions thrown by abort and error in their own types.
schmidt-sebastian
force-pushed
the
mrschmidt/limitretries
branch
from
dc130f6 to
32bb104
Compare
schmidt-sebastian
force-pushed
the
mrschmidt/limitretries
branch
from
374948d to
d2ac3b0
Compare
| constructor(cause: Error) { | ||
| super( | ||
| Code.UNKNOWN, | ||
| 'IndexedDB transaction failed with environment error: ' + cause |
There was a problem hiding this comment.
Drop "with environment error". "IndexedDB transaction failed" is enough to indicate what's failing and the cause will carry the details.
|
|
||
| constructor(cause: Error) { | ||
| super( | ||
| Code.UNKNOWN, |
There was a problem hiding this comment.
UNAVAILABLE could be a better code for this given what we know about when this happens.
| * 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 | ||
| * SimpleDbTransactionError (the error type used by SimpleDb). All |
There was a problem hiding this comment.
The error is actually IndexedDbTransactionError.
Limit the number of errors that can be retried on the AsyncQueue to those thrown by browsers.
Addresses #2755