From f9a2d69343a821397cfe3d55ea190c7ff80bbafc Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Fri, 25 Oct 2024 13:22:14 -0700 Subject: [PATCH 1/6] feat: Support `timeout` for `fetch` and `node-fetch` v3 --- README.md | 4 ++-- src/common.ts | 3 +++ src/gaxios.ts | 10 ++++++++++ test/test.getch.ts | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 0114e2f8..2cbe6a26 100644 --- a/README.md +++ b/README.md @@ -86,8 +86,8 @@ interface GaxiosOptions = { querystring: 'parameters' }, - // The timeout for the HTTP request in milliseconds. Defaults to 0. - timeout: 1000, + // The timeout for the HTTP request in milliseconds. No timeout by default. + timeout: 60000, // Optional method to override making the actual HTTP request. Useful // for writing tests and instrumentation diff --git a/src/common.ts b/src/common.ts index 911a0505..47665355 100644 --- a/src/common.ts +++ b/src/common.ts @@ -234,6 +234,9 @@ export interface GaxiosOptions extends RequestInit { * @deprecated Use {@link URLSearchParams} instead and pass this directly to {@link GaxiosOptions.data `data`}. */ paramsSerializer?: (params: {[index: string]: string | number}) => string; + /** + * A timeout for the request, in milliseconds. No timeout by default. + */ timeout?: number; /** * @deprecated ignored diff --git a/src/gaxios.ts b/src/gaxios.ts index 8e2a2d32..3dbfcd70 100644 --- a/src/gaxios.ts +++ b/src/gaxios.ts @@ -468,6 +468,16 @@ export class Gaxios { (opts as {duplex: string}).duplex = 'half'; } + if (opts.timeout) { + const timeoutSignal = AbortSignal.timeout(opts.timeout); + + if (opts.signal) { + opts.signal = AbortSignal.any([opts.signal, timeoutSignal]); + } else { + opts.signal = timeoutSignal; + } + } + return Object.assign(opts, { headers: preparedHeaders, url: opts.url instanceof URL ? opts.url : new URL(opts.url), diff --git a/test/test.getch.ts b/test/test.getch.ts index 93426f29..03da7e8b 100644 --- a/test/test.getch.ts +++ b/test/test.getch.ts @@ -690,6 +690,44 @@ describe('🥁 configuration options', () => { assert.equal(instance.defaults.errorRedactor, errorRedactor); }); + + describe('timeout', () => { + it('should accept and use a `timeout`', async () => { + nock(url).get('/').delay(2000).reply(204); + const gaxios = new Gaxios(); + const timeout = 10; + + await assert.rejects(() => gaxios.request({url, timeout}), /abort/); + }); + + it('should a `timeout`, an existing `signal`, and be triggered by timeout', async () => { + nock(url).get('/').delay(2000).reply(204); + const gaxios = new Gaxios(); + const signal = new AbortController().signal; + const timeout = 10; + + await assert.rejects( + () => gaxios.request({url, timeout, signal}), + /abort/ + ); + }); + + it('should use a `timeout`, a `signal`, and be triggered by signal', async () => { + nock(url).get('/').delay(2000).reply(204); + const gaxios = new Gaxios(); + const ac = new AbortController(); + const signal = ac.signal; + const timeout = Number.MAX_SAFE_INTEGER; + const message = 'Changed my mind - no request please'; + + setTimeout(() => ac.abort(message), 10); + + await assert.rejects( + () => gaxios.request({url, timeout, signal}), + message + ); + }); + }); }); describe('🎏 data handling', () => { From 59a465563b9de51d29e228d8ac24e3238db733fd Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Fri, 25 Oct 2024 14:37:33 -0700 Subject: [PATCH 2/6] test: fix --- test/test.getch.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test.getch.ts b/test/test.getch.ts index 03da7e8b..8578e392 100644 --- a/test/test.getch.ts +++ b/test/test.getch.ts @@ -720,11 +720,11 @@ describe('🥁 configuration options', () => { const timeout = Number.MAX_SAFE_INTEGER; const message = 'Changed my mind - no request please'; - setTimeout(() => ac.abort(message), 10); + setTimeout(() => ac.abort(new Error(message)), 10); await assert.rejects( () => gaxios.request({url, timeout, signal}), - message + new RegExp(message) ); }); }); From d5bda53233fe26565c9d476f8333cf397481385b Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Fri, 25 Oct 2024 14:41:19 -0700 Subject: [PATCH 3/6] test: fix --- test/test.getch.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test.getch.ts b/test/test.getch.ts index 8578e392..b097777a 100644 --- a/test/test.getch.ts +++ b/test/test.getch.ts @@ -717,7 +717,7 @@ describe('🥁 configuration options', () => { const gaxios = new Gaxios(); const ac = new AbortController(); const signal = ac.signal; - const timeout = Number.MAX_SAFE_INTEGER; + const timeout = 4000; // after network delay, so this shouldn't trigger const message = 'Changed my mind - no request please'; setTimeout(() => ac.abort(new Error(message)), 10); From 0190a1dada809ce7b2bc1022d6765b6f5725bb82 Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Fri, 25 Oct 2024 16:10:47 -0700 Subject: [PATCH 4/6] feat: `timeout`s should be retryable --- src/retry.ts | 2 +- test/test.getch.ts | 9 +++++++-- test/test.retry.ts | 27 +++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/retry.ts b/src/retry.ts index 6f696676..3e76bf14 100644 --- a/src/retry.ts +++ b/src/retry.ts @@ -103,7 +103,7 @@ function shouldRetryRequest(err: GaxiosError) { const config = getConfig(err); if ( - err.config.signal?.aborted || + (err.config.signal?.aborted && err.error?.name !== 'TimeoutError') || err.name === 'AbortError' || err.error?.name === 'AbortError' ) { diff --git a/test/test.getch.ts b/test/test.getch.ts index b097777a..5f1a6dec 100644 --- a/test/test.getch.ts +++ b/test/test.getch.ts @@ -720,11 +720,16 @@ describe('🥁 configuration options', () => { const timeout = 4000; // after network delay, so this shouldn't trigger const message = 'Changed my mind - no request please'; - setTimeout(() => ac.abort(new Error(message)), 10); + setTimeout(() => ac.abort(message), 10); + + // await gaxios.request({url, timeout, signal}); await assert.rejects( () => gaxios.request({url, timeout, signal}), - new RegExp(message) + // `node-fetch` always rejects with the generic 'abort' error: + /abort/ + // native `fetch` matches the error properly: + // new RegExp(message) ); }); }); diff --git a/test/test.retry.ts b/test/test.retry.ts index 00ecbe16..559c4312 100644 --- a/test/test.retry.ts +++ b/test/test.retry.ts @@ -367,4 +367,31 @@ describe('🛸 retry & exponential backoff', () => { assert.ok(delay > 4000 && delay < 4999); scope.done(); }); + + it('should retry on `timeout`', async () => { + let scope = nock(url).get('/').delay(2000).reply(400); + + const gaxios = new Gaxios(); + const timeout = 10; + + function onRetryAttempt() { + // prepare nock for next request + scope = nock(url).get('/').reply(204); + } + + const res = await gaxios.request({ + url, + timeout, + // NOTE: `node-fetch` does not yet support `TimeoutError` - testing with native `fetch` for now. + fetchImplementation: fetch, + retryConfig: { + onRetryAttempt, + }, + }); + + assert.equal(res.status, 204); + assert.equal(res.config?.retryConfig?.currentRetryAttempt, 1); + + scope.done(); + }); }); From 85a81c6830009557b3376e3b573145c425b66dd3 Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Fri, 25 Oct 2024 16:37:42 -0700 Subject: [PATCH 5/6] chore: lint fixes --- test/test.getch.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test.getch.ts b/test/test.getch.ts index 284df58a..9a6c4e30 100644 --- a/test/test.getch.ts +++ b/test/test.getch.ts @@ -708,7 +708,7 @@ describe('🥁 configuration options', () => { await assert.rejects( () => gaxios.request({url, timeout, signal}), - /abort/ + /abort/, ); }); @@ -727,7 +727,7 @@ describe('🥁 configuration options', () => { await assert.rejects( () => gaxios.request({url, timeout, signal}), // `node-fetch` always rejects with the generic 'abort' error: - /abort/ + /abort/, // native `fetch` matches the error properly: // new RegExp(message) ); From 0f3ab9b5a89b9cebb7821adfd930ac86850d3e69 Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Mon, 28 Oct 2024 11:16:48 -0700 Subject: [PATCH 6/6] chore: test clean-up --- test/test.getch.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/test.getch.ts b/test/test.getch.ts index 9a6c4e30..c07b522f 100644 --- a/test/test.getch.ts +++ b/test/test.getch.ts @@ -722,8 +722,6 @@ describe('🥁 configuration options', () => { setTimeout(() => ac.abort(message), 10); - // await gaxios.request({url, timeout, signal}); - await assert.rejects( () => gaxios.request({url, timeout, signal}), // `node-fetch` always rejects with the generic 'abort' error: