diff --git a/core/packages/gax/src/normalCalls/retries.ts b/core/packages/gax/src/normalCalls/retries.ts index 0ba7b31017cd..115653afef2c 100644 --- a/core/packages/gax/src/normalCalls/retries.ts +++ b/core/packages/gax/src/normalCalls/retries.ts @@ -68,6 +68,10 @@ export function retryable( return (argument: RequestType, callback: APICallback) => { let canceller: GRPCCallResult | null; let timeoutId: ReturnType | null; + // Set by `cancel()` below. The retry decision is made purely on the error + // code, which cannot distinguish a transport failure from the abort this + // call issued itself, so the caller's intent has to be recorded. + let cancelled = false; let now = new Date(); let deadline: number; if (retry.backoffSettings.totalTimeoutMillis) { @@ -152,6 +156,32 @@ export function retryable( return; } canceller = null; + // The caller cancelled, so this error is the consequence of the abort + // issued just above, whatever the transport chose to report it as. + // + // Without this, the decision below is made on the error code alone. A + // transport that reports an aborted request as a retryable code — the + // REST fallback reports transport failures as UNAVAILABLE — schedules + // another attempt, which discards the cancellation. Worse, `cancel()` + // has already run: `canceller` is null and the timer it would have + // cleared has not been created yet, so nothing remains that can stop + // the new attempt. It runs unbounded and the caller's callback is + // never invoked. + if (cancelled) { + // A transport that already reported the abort as CANCELLED has said + // everything worth saying; re-wrapping would only bury it. + if (err.code === Status.CANCELLED) { + callback(err); + return; + } + const error = new GoogleError( + 'cancelled' + errorDetailsSuffix(errorsEncountered), + {cause: err}, + ); + error.code = Status.CANCELLED; + callback(error); + return; + } if ( retry.retryCodes.length > 0 && retry.retryCodes.indexOf(err!.code!) < 0 @@ -194,6 +224,10 @@ export function retryable( return { cancel() { + // Recorded before aborting: `canceller.cancel()` can drive the + // transport's callback synchronously, and the guard there has to see + // this already set. + cancelled = true; if (timeoutId) { clearTimeout(timeoutId); } diff --git a/core/packages/gax/test/unit/apiCallable.ts b/core/packages/gax/test/unit/apiCallable.ts index f7cfaed41485..0cef34818607 100644 --- a/core/packages/gax/test/unit/apiCallable.ts +++ b/core/packages/gax/test/unit/apiCallable.ts @@ -457,6 +457,101 @@ describe('Promise', () => { }, 15); }); + it('does not retry a cancelled call whose error is retryable', done => { + // The REST fallback reports any transport-level failure as UNAVAILABLE, + // including the abort that `cancel()` itself triggers, so a cancelled call + // arrives here carrying a retryable code. Retrying it would discard the + // cancellation, and because `cancel()` has already run nothing remains that + // could stop the new attempt: it would run unbounded and this promise would + // never settle. Before the fix this test times out rather than failing. + const retryOptions = gax.createRetryOptions( + [status.UNAVAILABLE], + gax.createBackoffSettings(1, 1, 1, 1000, 1, 1000, 5000), + ); + + let callCount = 0; + function func( + argument: {}, + metadata: {}, + options: {}, + callback: Function, + ) { + callCount++; + // Completes only when cancelled, like a request to an endpoint that + // accepted the connection and then went quiet. + return function cancelFunc() { + const err = new GoogleError('The operation was aborted.'); + err.code = status.UNAVAILABLE; + callback(err); + }; + } + + const apiCall = createApiCall(func, { + settings: {retry: retryOptions}, + returnCancelFunc: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const promise = (apiCall as any)(null); + promise + .then(() => done(new Error('should not reach'))) + .catch((err: GoogleError) => { + try { + assert.strictEqual(err.code, status.CANCELLED); + assert.strictEqual( + callCount, + 1, + `expected no retry after cancel, saw ${callCount} invocations`, + ); + done(); + } catch (e) { + done(e); + } + }); + setTimeout(() => promise.cancel(), 10); + }); + + it('keeps a CANCELLED reported by the transport intact', done => { + // A transport that already resolved the abort to CANCELLED has said + // everything worth saying, so it should not be re-wrapped. + const retryOptions = gax.createRetryOptions( + [status.UNAVAILABLE], + gax.createBackoffSettings(1, 1, 1, 1000, 1, 1000, 5000), + ); + + const transportError = new GoogleError('cancelled by the transport'); + transportError.code = status.CANCELLED; + + function func( + argument: {}, + metadata: {}, + options: {}, + callback: Function, + ) { + return function cancelFunc() { + callback(transportError); + }; + } + + const apiCall = createApiCall(func, { + settings: {retry: retryOptions}, + returnCancelFunc: true, + }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const promise = (apiCall as any)(null); + promise + .then(() => done(new Error('should not reach'))) + .catch((err: GoogleError) => { + try { + assert.strictEqual(err, transportError); + assert.strictEqual(err.message, 'cancelled by the transport'); + done(); + } catch (e) { + done(e); + } + }); + setTimeout(() => promise.cancel(), 10); + }); + it('does not return promise when callback is supplied', done => { function func(argument: {}, metadata: {}, options: {}, callback: Function) { callback(null, 42);