From 8f16b859066b2542eaef4162ca1e83808796a960 Mon Sep 17 00:00:00 2001 From: Sean Kelly Date: Thu, 19 Jan 2023 16:03:43 -0800 Subject: [PATCH 1/5] Fix default fetch parameters --- lib/fetch/index.js | 2 -- test/fetch/fetch-long.js | 51 ++++++++++++++++++++++++++++++++++++++ test/fetch/request-long.js | 48 +++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 test/fetch/fetch-long.js create mode 100644 test/fetch/request-long.js diff --git a/lib/fetch/index.js b/lib/fetch/index.js index ddcf3c18f91..133aa999d74 100644 --- a/lib/fetch/index.js +++ b/lib/fetch/index.js @@ -1951,8 +1951,6 @@ async function httpNetworkFetch ( body: fetchParams.controller.dispatcher.isMockActive ? request.body && request.body.source : body, headers: request.headersList[kHeadersCaseInsensitive], maxRedirections: 0, - bodyTimeout: 300_000, - headersTimeout: 300_000, upgrade: request.mode === 'websocket' ? 'websocket' : undefined }, { diff --git a/test/fetch/fetch-long.js b/test/fetch/fetch-long.js new file mode 100644 index 00000000000..eb57abb6ebd --- /dev/null +++ b/test/fetch/fetch-long.js @@ -0,0 +1,51 @@ +'use strict' + +const { test } = require('tap') + +const { fetch, Agent } = require('../../') +const { createServer } = require('http') +const FakeTimers = require('@sinonjs/fake-timers') + +const minutes = 6 +const msToDelay = 1000 * 60 * minutes + +const agent = new Agent({ + headersTimeout: 0, + connectTimeout: 0, + bodyTimeout: 0 +}) + +test('Long time for a single fetch', (t) => { + t.setTimeout(undefined) + t.plan(1) + + const clock = FakeTimers.install() + t.teardown(clock.uninstall.bind(clock)) + + const server = createServer((req, res) => { + setTimeout(() => { + res.end('hello') + }, msToDelay) + clock.tick(msToDelay + 1) + }) + t.teardown(server.close.bind(server)) + + server.listen(0, () => { + fetch(`http://localhost:${server.address().port}`, { + path: '/', + method: 'GET', + dispatcher: agent + }) + .then((response) => response.text()) + .then((response) => { + t.equal('hello', response) + t.end() + }) + .catch((err) => { + console.error(err) + t.error(err) + }) + + clock.tick(msToDelay - 1) + }) +}) diff --git a/test/fetch/request-long.js b/test/fetch/request-long.js new file mode 100644 index 00000000000..7dbe4b6115f --- /dev/null +++ b/test/fetch/request-long.js @@ -0,0 +1,48 @@ +'use strict' + +const { test } = require('tap') + +const { Client } = require('../..') +const { createServer } = require('http') +const FakeTimers = require('@sinonjs/fake-timers') + +const minutes = 6 +const msToDelay = 1000 * 60 * minutes + +test('Long time for a single request', (t) => { + t.setTimeout(undefined) + + t.plan(2) + + const clock = FakeTimers.install() + t.teardown(clock.uninstall.bind(clock)) + + const server = createServer((req, res) => { + setTimeout(() => { + res.end('hello') + }, msToDelay) + clock.tick(msToDelay + 1) + }) + t.teardown(server.close.bind(server)) + + server.listen(0, () => { + const client = new Client(`http://localhost:${server.address().port}`, { + headersTimeout: 0, + connectTimeout: 0 + }) + t.teardown(client.destroy.bind(client)) + + client.request({ path: '/', method: 'GET' }, (err, response) => { + t.error(err) + const bufs = [] + response.body.on('data', (buf) => { + bufs.push(buf) + }) + response.body.on('end', () => { + t.equal('hello', Buffer.concat(bufs).toString('utf8')) + }) + }) + + clock.tick(msToDelay - 1) + }) +}) From 0a666a28d59a4f05d1fa1dc2d5e78f5b6de80270 Mon Sep 17 00:00:00 2001 From: Sean Kelly Date: Thu, 19 Jan 2023 16:49:24 -0800 Subject: [PATCH 2/5] Preserve existing behavior with 300 second timeout if not defined --- lib/agent.js | 4 ++++ lib/fetch/index.js | 2 ++ 2 files changed, 6 insertions(+) diff --git a/lib/agent.js b/lib/agent.js index 0b18f2a91bd..6dfbb68461d 100644 --- a/lib/agent.js +++ b/lib/agent.js @@ -81,6 +81,10 @@ class Agent extends DispatcherBase { } } + options () { + return this[kOptions] + } + get [kRunning] () { let ret = 0 for (const ref of this[kClients].values()) { diff --git a/lib/fetch/index.js b/lib/fetch/index.js index 133aa999d74..5d93b27dbbf 100644 --- a/lib/fetch/index.js +++ b/lib/fetch/index.js @@ -1951,6 +1951,8 @@ async function httpNetworkFetch ( body: fetchParams.controller.dispatcher.isMockActive ? request.body && request.body.source : body, headers: request.headersList[kHeadersCaseInsensitive], maxRedirections: 0, + bodyTimeout: agent.options?.().bodyTimeout ?? 300_000, + headersTimeout: agent.options?.().headersTimeout ?? 300_000, upgrade: request.mode === 'websocket' ? 'websocket' : undefined }, { From db6722950fe5641f411fced4da5e32a63d17fb24 Mon Sep 17 00:00:00 2001 From: Sean Kelly Date: Thu, 19 Jan 2023 17:03:06 -0800 Subject: [PATCH 3/5] Add test for 300 second timeout as default --- test/fetch/fetch-timeouts.js | 81 ++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 test/fetch/fetch-timeouts.js diff --git a/test/fetch/fetch-timeouts.js b/test/fetch/fetch-timeouts.js new file mode 100644 index 00000000000..f072fb14a35 --- /dev/null +++ b/test/fetch/fetch-timeouts.js @@ -0,0 +1,81 @@ +'use strict' + +const { test } = require('tap') + +const { fetch, errors, Agent } = require('../..') +const { createServer } = require('http') +const FakeTimers = require('@sinonjs/fake-timers') + +const agent = new Agent({ + connectTimeout: 0 +}) + +test('Fetch should have a default timeout of 300 seconds triggered', (t) => { + const msToDelay = 300_000 + t.setTimeout(undefined) + t.plan(1) + + const clock = FakeTimers.install() + t.teardown(clock.uninstall.bind(clock)) + + const server = createServer((req, res) => { + setTimeout(() => { + res.end('hello') + }, msToDelay) + clock.tick(msToDelay + 1) + }) + t.teardown(server.close.bind(server)) + + server.listen(0, () => { + fetch(`http://localhost:${server.address().port}`, { + path: '/', + method: 'GET', + dispatcher: agent + }) + .then(() => { + // This should not happen, a timeout error should occur + t.error(true) + }) + .catch((err) => { + t.type(err.cause, errors.HeadersTimeoutError) + }) + + clock.tick(msToDelay - 1) + }) +}) + +test('Fetch should have a default timeout of 300 seconds not triggered', (t) => { + const msToDelay = 299_000 + t.setTimeout(undefined) + t.plan(1) + + const clock = FakeTimers.install() + t.teardown(clock.uninstall.bind(clock)) + + const server = createServer((req, res) => { + setTimeout(() => { + res.end('hello') + }, msToDelay) + clock.tick(msToDelay + 1) + }) + t.teardown(server.close.bind(server)) + + server.listen(0, () => { + fetch(`http://localhost:${server.address().port}`, { + path: '/', + method: 'GET', + dispatcher: agent + }) + .then((response) => response.text()) + .then((response) => { + t.equal('hello', response) + t.end() + }) + .catch((err) => { + // This should not happen, a timeout error should not occur + t.error(err) + }) + + clock.tick(msToDelay - 1) + }) +}) From 2c0caef7ae191877ab79bee102d1eea6d79b5608 Mon Sep 17 00:00:00 2001 From: Sean Kelly Date: Thu, 19 Jan 2023 20:23:00 -0800 Subject: [PATCH 4/5] Cleanup old unused tests --- test/fetch/fetch-long.js | 51 ------------------------------- test/fetch/fetch-timeouts.js | 54 +++++++++++++++++++++++++++++---- test/node-fetch/main.js | 16 ---------- test/node-fetch/utils/server.js | 16 ---------- 4 files changed, 48 insertions(+), 89 deletions(-) delete mode 100644 test/fetch/fetch-long.js diff --git a/test/fetch/fetch-long.js b/test/fetch/fetch-long.js deleted file mode 100644 index eb57abb6ebd..00000000000 --- a/test/fetch/fetch-long.js +++ /dev/null @@ -1,51 +0,0 @@ -'use strict' - -const { test } = require('tap') - -const { fetch, Agent } = require('../../') -const { createServer } = require('http') -const FakeTimers = require('@sinonjs/fake-timers') - -const minutes = 6 -const msToDelay = 1000 * 60 * minutes - -const agent = new Agent({ - headersTimeout: 0, - connectTimeout: 0, - bodyTimeout: 0 -}) - -test('Long time for a single fetch', (t) => { - t.setTimeout(undefined) - t.plan(1) - - const clock = FakeTimers.install() - t.teardown(clock.uninstall.bind(clock)) - - const server = createServer((req, res) => { - setTimeout(() => { - res.end('hello') - }, msToDelay) - clock.tick(msToDelay + 1) - }) - t.teardown(server.close.bind(server)) - - server.listen(0, () => { - fetch(`http://localhost:${server.address().port}`, { - path: '/', - method: 'GET', - dispatcher: agent - }) - .then((response) => response.text()) - .then((response) => { - t.equal('hello', response) - t.end() - }) - .catch((err) => { - console.error(err) - t.error(err) - }) - - clock.tick(msToDelay - 1) - }) -}) diff --git a/test/fetch/fetch-timeouts.js b/test/fetch/fetch-timeouts.js index f072fb14a35..f53a9434d90 100644 --- a/test/fetch/fetch-timeouts.js +++ b/test/fetch/fetch-timeouts.js @@ -6,10 +6,6 @@ const { fetch, errors, Agent } = require('../..') const { createServer } = require('http') const FakeTimers = require('@sinonjs/fake-timers') -const agent = new Agent({ - connectTimeout: 0 -}) - test('Fetch should have a default timeout of 300 seconds triggered', (t) => { const msToDelay = 300_000 t.setTimeout(undefined) @@ -30,7 +26,9 @@ test('Fetch should have a default timeout of 300 seconds triggered', (t) => { fetch(`http://localhost:${server.address().port}`, { path: '/', method: 'GET', - dispatcher: agent + dispatcher: new Agent({ + connectTimeout: 0 + }) }) .then(() => { // This should not happen, a timeout error should occur @@ -64,7 +62,51 @@ test('Fetch should have a default timeout of 300 seconds not triggered', (t) => fetch(`http://localhost:${server.address().port}`, { path: '/', method: 'GET', - dispatcher: agent + dispatcher: new Agent({ + connectTimeout: 0 + }) + }) + .then((response) => response.text()) + .then((response) => { + t.equal('hello', response) + t.end() + }) + .catch((err) => { + // This should not happen, a timeout error should not occur + t.error(err) + }) + + clock.tick(msToDelay - 1) + }) +}) + +test('Fetch very long request, timeout overridden so no error', (t) => { + const minutes = 6 + const msToDelay = 1000 * 60 * minutes + + t.setTimeout(undefined) + t.plan(1) + + const clock = FakeTimers.install() + t.teardown(clock.uninstall.bind(clock)) + + const server = createServer((req, res) => { + setTimeout(() => { + res.end('hello') + }, msToDelay) + clock.tick(msToDelay + 1) + }) + t.teardown(server.close.bind(server)) + + server.listen(0, () => { + fetch(`http://localhost:${server.address().port}`, { + path: '/', + method: 'GET', + dispatcher: new Agent({ + headersTimeout: 0, + connectTimeout: 0, + bodyTimeout: 0 + }) }) .then((response) => response.text()) .then((response) => { diff --git a/test/node-fetch/main.js b/test/node-fetch/main.js index acefe825ea4..ab3ee9f70ae 100644 --- a/test/node-fetch/main.js +++ b/test/node-fetch/main.js @@ -1660,20 +1660,4 @@ describe('node-fetch', () => { expect(res.ok).to.be.false }) }) - - // it('should not time out waiting for a response 60 seconds', function () { - // this.timeout(65_000) - // return fetch(`${base}timeout60s`).then(res => { - // expect(res.status).to.equal(200) - // expect(res.ok).to.be.true - // return res.text().then(result => { - // expect(result).to.equal('text') - // }) - // }) - // }) - - // it('should time out waiting for more than 300 seconds', function () { - // this.timeout(305_000) - // return expect(fetch(`${base}timeout300s`)).to.eventually.be.rejectedWith(TypeError) - // }) }) diff --git a/test/node-fetch/utils/server.js b/test/node-fetch/utils/server.js index e846465dbf7..46103055305 100644 --- a/test/node-fetch/utils/server.js +++ b/test/node-fetch/utils/server.js @@ -227,22 +227,6 @@ module.exports = class TestServer { }, 1000) } - if (p === '/timeout60s') { - setTimeout(() => { - res.statusCode = 200 - res.setHeader('Content-Type', 'text/plain') - res.end('text') - }, 60_000) - } - - if (p === '/timeout300s') { - setTimeout(() => { - res.statusCode = 200 - res.setHeader('Content-Type', 'text/plain') - res.end('text') - }, 300_000) - } - if (p === '/slow') { res.statusCode = 200 res.setHeader('Content-Type', 'text/plain') From 1e3d1293fe5b9ad3275efa1f6fe880745e079319 Mon Sep 17 00:00:00 2001 From: Sean Kelly Date: Fri, 20 Jan 2023 08:25:51 -0800 Subject: [PATCH 5/5] Simplify how fetch utilizes timeouts from agent --- lib/agent.js | 4 -- lib/fetch/index.js | 2 - test/fetch/fetch-timeouts.js | 76 +----------------------------------- test/fetch/request-long.js | 48 ----------------------- 4 files changed, 1 insertion(+), 129 deletions(-) delete mode 100644 test/fetch/request-long.js diff --git a/lib/agent.js b/lib/agent.js index 6dfbb68461d..0b18f2a91bd 100644 --- a/lib/agent.js +++ b/lib/agent.js @@ -81,10 +81,6 @@ class Agent extends DispatcherBase { } } - options () { - return this[kOptions] - } - get [kRunning] () { let ret = 0 for (const ref of this[kClients].values()) { diff --git a/lib/fetch/index.js b/lib/fetch/index.js index 5d93b27dbbf..133aa999d74 100644 --- a/lib/fetch/index.js +++ b/lib/fetch/index.js @@ -1951,8 +1951,6 @@ async function httpNetworkFetch ( body: fetchParams.controller.dispatcher.isMockActive ? request.body && request.body.source : body, headers: request.headersList[kHeadersCaseInsensitive], maxRedirections: 0, - bodyTimeout: agent.options?.().bodyTimeout ?? 300_000, - headersTimeout: agent.options?.().headersTimeout ?? 300_000, upgrade: request.mode === 'websocket' ? 'websocket' : undefined }, { diff --git a/test/fetch/fetch-timeouts.js b/test/fetch/fetch-timeouts.js index f53a9434d90..adbf888ebba 100644 --- a/test/fetch/fetch-timeouts.js +++ b/test/fetch/fetch-timeouts.js @@ -2,84 +2,10 @@ const { test } = require('tap') -const { fetch, errors, Agent } = require('../..') +const { fetch, Agent } = require('../..') const { createServer } = require('http') const FakeTimers = require('@sinonjs/fake-timers') -test('Fetch should have a default timeout of 300 seconds triggered', (t) => { - const msToDelay = 300_000 - t.setTimeout(undefined) - t.plan(1) - - const clock = FakeTimers.install() - t.teardown(clock.uninstall.bind(clock)) - - const server = createServer((req, res) => { - setTimeout(() => { - res.end('hello') - }, msToDelay) - clock.tick(msToDelay + 1) - }) - t.teardown(server.close.bind(server)) - - server.listen(0, () => { - fetch(`http://localhost:${server.address().port}`, { - path: '/', - method: 'GET', - dispatcher: new Agent({ - connectTimeout: 0 - }) - }) - .then(() => { - // This should not happen, a timeout error should occur - t.error(true) - }) - .catch((err) => { - t.type(err.cause, errors.HeadersTimeoutError) - }) - - clock.tick(msToDelay - 1) - }) -}) - -test('Fetch should have a default timeout of 300 seconds not triggered', (t) => { - const msToDelay = 299_000 - t.setTimeout(undefined) - t.plan(1) - - const clock = FakeTimers.install() - t.teardown(clock.uninstall.bind(clock)) - - const server = createServer((req, res) => { - setTimeout(() => { - res.end('hello') - }, msToDelay) - clock.tick(msToDelay + 1) - }) - t.teardown(server.close.bind(server)) - - server.listen(0, () => { - fetch(`http://localhost:${server.address().port}`, { - path: '/', - method: 'GET', - dispatcher: new Agent({ - connectTimeout: 0 - }) - }) - .then((response) => response.text()) - .then((response) => { - t.equal('hello', response) - t.end() - }) - .catch((err) => { - // This should not happen, a timeout error should not occur - t.error(err) - }) - - clock.tick(msToDelay - 1) - }) -}) - test('Fetch very long request, timeout overridden so no error', (t) => { const minutes = 6 const msToDelay = 1000 * 60 * minutes diff --git a/test/fetch/request-long.js b/test/fetch/request-long.js deleted file mode 100644 index 7dbe4b6115f..00000000000 --- a/test/fetch/request-long.js +++ /dev/null @@ -1,48 +0,0 @@ -'use strict' - -const { test } = require('tap') - -const { Client } = require('../..') -const { createServer } = require('http') -const FakeTimers = require('@sinonjs/fake-timers') - -const minutes = 6 -const msToDelay = 1000 * 60 * minutes - -test('Long time for a single request', (t) => { - t.setTimeout(undefined) - - t.plan(2) - - const clock = FakeTimers.install() - t.teardown(clock.uninstall.bind(clock)) - - const server = createServer((req, res) => { - setTimeout(() => { - res.end('hello') - }, msToDelay) - clock.tick(msToDelay + 1) - }) - t.teardown(server.close.bind(server)) - - server.listen(0, () => { - const client = new Client(`http://localhost:${server.address().port}`, { - headersTimeout: 0, - connectTimeout: 0 - }) - t.teardown(client.destroy.bind(client)) - - client.request({ path: '/', method: 'GET' }, (err, response) => { - t.error(err) - const bufs = [] - response.body.on('data', (buf) => { - bufs.push(buf) - }) - response.body.on('end', () => { - t.equal('hello', Buffer.concat(bufs).toString('utf8')) - }) - }) - - clock.tick(msToDelay - 1) - }) -})