diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index 8e88caf6849..26b831e82a3 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -27,7 +27,7 @@ import { } from '../model/mutation_batch'; import { emptyByteString } from '../platform/platform'; import { assert } from '../util/assert'; -import { Code, FirestoreError } from '../util/error'; +import { FirestoreError } from '../util/error'; import * as log from '../util/log'; import * as objUtils from '../util/obj'; @@ -41,7 +41,7 @@ import { PersistentWriteStream } from './persistent_stream'; import { RemoteSyncer } from './remote_syncer'; -import { isPermanentError } from './rpc_error'; +import { isPermanentError, isPermanentWriteError } from './rpc_error'; import { DocumentWatchChange, ExistenceFilterChange, @@ -644,9 +644,10 @@ export class RemoteStore implements TargetMetadataProvider { } private async handleHandshakeError(error: FirestoreError): Promise { - // Reset the token if it's a permanent error or the error code is - // ABORTED, signaling the write stream is no longer valid. - if (isPermanentError(error.code) || error.code === Code.ABORTED) { + // Reset the token if it's a permanent error, signaling the write stream is + // no longer valid. Note that the handshake does not count as a write: see + // comments on isPermanentWriteError for details. + if (isPermanentError(error.code)) { log.debug( LOG_TAG, 'RemoteStore error before completed handshake; resetting stream token: ', @@ -664,7 +665,9 @@ export class RemoteStore implements TargetMetadataProvider { } private async handleWriteError(error: FirestoreError): Promise { - if (isPermanentError(error.code)) { + // Only handle permanent errors here. If it's transient, just let the retry + // logic kick in. + if (isPermanentWriteError(error.code)) { // This was a permanent error, the request itself was the problem // so it's not going to succeed if we resend it. const batch = this.writePipeline.shift()!; diff --git a/packages/firestore/src/remote/rpc_error.ts b/packages/firestore/src/remote/rpc_error.ts index 1e554dce5df..81d55f9b344 100644 --- a/packages/firestore/src/remote/rpc_error.ts +++ b/packages/firestore/src/remote/rpc_error.ts @@ -48,6 +48,12 @@ enum RpcCode { DATA_LOSS = 15 } +/** + * Determines whether an error code represents a permanent error when received + * in response to a non-write operation. + * + * See isPermanentWriteError for classifying write errors. + */ export function isPermanentError(code: Code): boolean { switch (code) { case Code.OK: @@ -80,6 +86,22 @@ export function isPermanentError(code: Code): boolean { } } +/** + * Determines whether an error code represents a permanent error when received + * in response to a write operation. + * + * Write operations must be handled specially because as of b/119437764, ABORTED + * errors on the write stream should be retried too (even though ABORTED errors + * are not generally retryable). + * + * Note that during the initial handshake on the write stream an ABORTED error + * signals that we should discard our stream token (i.e. it is permanent). This + * means a handshake error should be classified with isPermanentError, above. + */ +export function isPermanentWriteError(code: Code): boolean { + return isPermanentError(code) && code !== Code.ABORTED; +} + /** * Maps an error Code from a GRPC status identifier like 'NOT_FOUND'. * diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index 6c4836e54f3..cad45f02915 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -25,7 +25,7 @@ import { import { DocumentKey } from '../../../src/model/document_key'; import { JsonObject } from '../../../src/model/field_value'; import { - isPermanentError, + isPermanentWriteError, mapCodeFromRpcCode, mapRpcCodeFromCode } from '../../../src/remote/rpc_error'; @@ -504,7 +504,8 @@ export class SpecBuilder { // If this is a permanent error, the write is not expected to be sent // again. - const isPermanentFailure = isPermanentError(mapCodeFromRpcCode(error.code)); + const code = mapCodeFromRpcCode(error.code); + const isPermanentFailure = isPermanentWriteError(code); const keepInQueue = options.keepInQueue !== undefined ? options.keepInQueue diff --git a/packages/firestore/test/unit/specs/write_spec.test.ts b/packages/firestore/test/unit/specs/write_spec.test.ts index 737019cbb80..8159f7f286e 100644 --- a/packages/firestore/test/unit/specs/write_spec.test.ts +++ b/packages/firestore/test/unit/specs/write_spec.test.ts @@ -633,7 +633,6 @@ describeSpec('Writes:', [], () => { Code.ALREADY_EXISTS, Code.PERMISSION_DENIED, Code.FAILED_PRECONDITION, - Code.ABORTED, Code.OUT_OF_RANGE, Code.UNIMPLEMENTED, Code.DATA_LOSS @@ -697,6 +696,7 @@ describeSpec('Writes:', [], () => { ); for (const code of [ + Code.ABORTED, Code.CANCELLED, Code.UNKNOWN, Code.DEADLINE_EXCEEDED,