From e410afc4c6cdbf7f259d09bd01998dc8bf193a86 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 3 May 2024 19:22:22 +0200 Subject: [PATCH 01/25] use FinalizationRegistry to cancel the body if response is collected Signed-off-by: Matteo Collina --- lib/web/fetch/index.js | 18 +++++++++---- lib/web/fetch/response.js | 14 ++++++++++ test/fetch/fire-and-forget.js | 49 +++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 5 deletions(-) create mode 100644 test/fetch/fire-and-forget.js diff --git a/lib/web/fetch/index.js b/lib/web/fetch/index.js index e7e7acfb0ea..d007130f7a8 100644 --- a/lib/web/fetch/index.js +++ b/lib/web/fetch/index.js @@ -120,6 +120,10 @@ class Fetch extends EE { } } +function handleFetchDone (response) { + finalizeAndReportTiming(response, 'fetch') +} + // https://fetch.spec.whatwg.org/#fetch-method function fetch (input, init = undefined) { webidl.argumentLengthCheck(arguments, 1, 'globalThis.fetch') @@ -185,16 +189,17 @@ function fetch (input, init = undefined) { // 3. Abort controller with requestObject’s signal’s abort reason. controller.abort(requestObject.signal.reason) + const realResponse = responseObject?.deref() + // 4. Abort the fetch() call with p, request, responseObject, // and requestObject’s signal’s abort reason. - abortFetch(p, request, responseObject, requestObject.signal.reason) + abortFetch(p, request, realResponse, requestObject.signal.reason) } ) // 12. Let handleFetchDone given response response be to finalize and // report timing with response, globalObject, and "fetch". - const handleFetchDone = (response) => - finalizeAndReportTiming(response, 'fetch') + // see function handleFetchDone // 13. Set controller to the result of calling fetch given request, // with processResponseEndOfBody set to handleFetchDone, and processResponse @@ -228,7 +233,7 @@ function fetch (input, init = undefined) { // 4. Set responseObject to the result of creating a Response object, // given response, "immutable", and relevantRealm. - responseObject = fromInnerResponse(response, 'immutable') + responseObject = new WeakRef(fromInnerResponse(response, 'immutable')) // 5. Resolve p with responseObject. p.resolve(responseObject) @@ -1066,7 +1071,10 @@ function fetchFinale (fetchParams, response) { // 4. If fetchParams’s process response is non-null, then queue a fetch task to run fetchParams’s // process response given response, with fetchParams’s task destination. if (fetchParams.processResponse != null) { - queueMicrotask(() => fetchParams.processResponse(response)) + queueMicrotask(() => { + fetchParams.processResponse(response) + fetchParams.processResponse = null + }) } // 5. Let internalResponse be response, if response is a network error; otherwise response’s internal response. diff --git a/lib/web/fetch/response.js b/lib/web/fetch/response.js index 222a9a5b2f7..62c0bb03c20 100644 --- a/lib/web/fetch/response.js +++ b/lib/web/fetch/response.js @@ -29,6 +29,15 @@ const { types } = require('node:util') const textEncoder = new TextEncoder('utf-8') +const hasFinalizationRegistry = globalThis.FinalizationRegistry && process.version.indexOf('v18') !== 0 +let registry + +if (hasFinalizationRegistry) { + registry = new FinalizationRegistry((stream) => { + stream.cancel('Response object has been garbage collected') + }) +} + // https://fetch.spec.whatwg.org/#response-class class Response { // Creates network error Response. @@ -510,6 +519,11 @@ function fromInnerResponse (innerResponse, guard) { response[kHeaders] = new Headers(kConstruct) response[kHeaders][kHeadersList] = innerResponse.headersList response[kHeaders][kGuard] = guard + + if (hasFinalizationRegistry && innerResponse.body?.stream) { + registry.register(response, innerResponse.body.stream) + } + return response } diff --git a/test/fetch/fire-and-forget.js b/test/fetch/fire-and-forget.js new file mode 100644 index 00000000000..deefe288b7a --- /dev/null +++ b/test/fetch/fire-and-forget.js @@ -0,0 +1,49 @@ +'use strict' + +const { randomFillSync } = require('node:crypto') +const { setTimeout: sleep } = require('timers/promises') +const { test } = require('node:test') +const { fetch, Agent, setGlobalDispatcher } = require('../..') +const { createServer } = require('node:http') +const { closeServerAsPromise } = require('../utils/node-http') + +const blob = randomFillSync(new Uint8Array(1024 * 512)) +const fmt = new Intl.NumberFormat() + +test('does not need the body to be consumed to continue', async (t) => { + const agent = new Agent({ + keepAliveMaxTimeout: 10, + keepAliveTimeoutThreshold: 10 + }) + setGlobalDispatcher(agent) + const server = createServer((req, res) => { + res.writeHead(200) + res.end(blob) + }) + t.after(closeServerAsPromise(server)) + + await new Promise((resolve) => { + server.listen(0, resolve) + }) + + const url = new URL(`http://127.0.0.1:${server.address().port}`) + + const batch = 50 + const delay = 0 + let total = 0 + while (total < 10000) { + const array = new Array(batch) + for (let i = 0; i < batch; i++) { + array[i] = fetch(url).catch(() => {}) + } + await Promise.all(array) + await sleep(delay) + + console.log( + 'RSS', + (process.memoryUsage.rss() / 1024 / 1024) | 0, + 'MB after', + fmt.format((total += batch)) + ' fetch() requests' + ) + } +}) From f0f8b04c0cf383e75a96f3402ec91289e7746c5d Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 3 May 2024 19:31:56 +0200 Subject: [PATCH 02/25] fixup Signed-off-by: Matteo Collina --- lib/web/fetch/index.js | 10 +++++++--- test/mock-agent.js | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/web/fetch/index.js b/lib/web/fetch/index.js index d007130f7a8..f475606b352 100644 --- a/lib/web/fetch/index.js +++ b/lib/web/fetch/index.js @@ -129,7 +129,7 @@ function fetch (input, init = undefined) { webidl.argumentLengthCheck(arguments, 1, 'globalThis.fetch') // 1. Let p be a new promise. - const p = createDeferredPromise() + let p = createDeferredPromise() // 2. Let requestObject be the result of invoking the initial value of // Request as constructor with input and init as arguments. If this throws @@ -236,7 +236,8 @@ function fetch (input, init = undefined) { responseObject = new WeakRef(fromInnerResponse(response, 'immutable')) // 5. Resolve p with responseObject. - p.resolve(responseObject) + p.resolve(responseObject.deref()) + p = null } controller = fetching({ @@ -319,7 +320,10 @@ const markResourceTiming = performance.markResourceTiming // https://fetch.spec.whatwg.org/#abort-fetch function abortFetch (p, request, responseObject, error) { // 1. Reject promise with error. - p.reject(error) + if (p) { + // We might have already resolved the promise at this stage + p.reject(error) + } // 2. If request’s body is not null and is readable, then cancel request’s // body with error. diff --git a/test/mock-agent.js b/test/mock-agent.js index eb58afc6ad1..7f79c0f426e 100644 --- a/test/mock-agent.js +++ b/test/mock-agent.js @@ -2495,7 +2495,7 @@ test('MockAgent - headers in mock dispatcher intercept should be case-insensitiv }) // https://github.com/nodejs/undici/issues/1757 -test('MockAgent - reply callback can be asynchronous', async (t) => { +test('MockAgent - reply callback can be asynchronous', { only: true }, async (t) => { t = tspl(t, { plan: 2 }) class MiniflareDispatcher extends Dispatcher { From 7cc4904a819554d25ba811f741f9f148a194fd35 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 3 May 2024 19:39:21 +0200 Subject: [PATCH 03/25] fixup Signed-off-by: Matteo Collina --- lib/web/fetch/response.js | 4 +++- test/mock-agent.js | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/web/fetch/response.js b/lib/web/fetch/response.js index 62c0bb03c20..ee2885359fa 100644 --- a/lib/web/fetch/response.js +++ b/lib/web/fetch/response.js @@ -34,7 +34,9 @@ let registry if (hasFinalizationRegistry) { registry = new FinalizationRegistry((stream) => { - stream.cancel('Response object has been garbage collected') + if (!stream.locked && stream.state === 'readable') { + stream.cancel('Response object has been garbage collected') + } }) } diff --git a/test/mock-agent.js b/test/mock-agent.js index 7f79c0f426e..eb58afc6ad1 100644 --- a/test/mock-agent.js +++ b/test/mock-agent.js @@ -2495,7 +2495,7 @@ test('MockAgent - headers in mock dispatcher intercept should be case-insensitiv }) // https://github.com/nodejs/undici/issues/1757 -test('MockAgent - reply callback can be asynchronous', { only: true }, async (t) => { +test('MockAgent - reply callback can be asynchronous', async (t) => { t = tspl(t, { plan: 2 }) class MiniflareDispatcher extends Dispatcher { From 02bc59e1a0e6748ff1838d9afeb070533cbc8336 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 3 May 2024 19:46:11 +0200 Subject: [PATCH 04/25] fixup Signed-off-by: Matteo Collina --- lib/web/fetch/response.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/web/fetch/response.js b/lib/web/fetch/response.js index ee2885359fa..5d945d99d86 100644 --- a/lib/web/fetch/response.js +++ b/lib/web/fetch/response.js @@ -26,6 +26,7 @@ const { URLSerializer } = require('./data-url') const { kHeadersList, kConstruct } = require('../../core/symbols') const assert = require('node:assert') const { types } = require('node:util') +const { isDisturbed } = require('node:stream') const textEncoder = new TextEncoder('utf-8') @@ -34,7 +35,7 @@ let registry if (hasFinalizationRegistry) { registry = new FinalizationRegistry((stream) => { - if (!stream.locked && stream.state === 'readable') { + if (!stream.locked && !isDisturbed(stream)) { stream.cancel('Response object has been garbage collected') } }) From 758689b830e9fc4f43ee0b4eb6d86eada52a61b0 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sat, 4 May 2024 00:27:45 +0200 Subject: [PATCH 05/25] fixup Signed-off-by: Matteo Collina --- lib/web/fetch/response.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/web/fetch/response.js b/lib/web/fetch/response.js index 5d945d99d86..224f25b8df8 100644 --- a/lib/web/fetch/response.js +++ b/lib/web/fetch/response.js @@ -26,7 +26,7 @@ const { URLSerializer } = require('./data-url') const { kHeadersList, kConstruct } = require('../../core/symbols') const assert = require('node:assert') const { types } = require('node:util') -const { isDisturbed } = require('node:stream') +const { isDisturbed, isErrored } = require('node:stream') const textEncoder = new TextEncoder('utf-8') @@ -35,7 +35,7 @@ let registry if (hasFinalizationRegistry) { registry = new FinalizationRegistry((stream) => { - if (!stream.locked && !isDisturbed(stream)) { + if (!stream.locked && !isDisturbed(stream) && !isErrored(stream)) { stream.cancel('Response object has been garbage collected') } }) From 1b40253c01f31e2c9d1fce202d98a8db1c33b896 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sat, 4 May 2024 09:50:04 +0200 Subject: [PATCH 06/25] fixup Signed-off-by: Matteo Collina --- test/fetch/fire-and-forget.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fetch/fire-and-forget.js b/test/fetch/fire-and-forget.js index deefe288b7a..4cf86b31064 100644 --- a/test/fetch/fire-and-forget.js +++ b/test/fetch/fire-and-forget.js @@ -10,7 +10,7 @@ const { closeServerAsPromise } = require('../utils/node-http') const blob = randomFillSync(new Uint8Array(1024 * 512)) const fmt = new Intl.NumberFormat() -test('does not need the body to be consumed to continue', async (t) => { +test('does not need the body to be consumed to continue', { timeout: 120_1000 }, async (t) => { const agent = new Agent({ keepAliveMaxTimeout: 10, keepAliveTimeoutThreshold: 10 From 32ed11b82f34a5d9cb679782e731140b528d06da Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sat, 4 May 2024 09:51:39 +0200 Subject: [PATCH 07/25] fixup Signed-off-by: Matteo Collina --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 0b8f491a59b..ff61d61cdc5 100644 --- a/package.json +++ b/package.json @@ -77,7 +77,7 @@ "test:eventsource:nobuild": "borp --expose-gc -p \"test/eventsource/*.js\"", "test:fuzzing": "node test/fuzzing/fuzzing.test.js", "test:fetch": "npm run build:node && npm run test:fetch:nobuild", - "test:fetch:nobuild": "borp --expose-gc -p \"test/fetch/*.js\" && npm run test:webidl && npm run test:busboy", + "test:fetch:nobuild": "borp --timeout 120000 --expose-gc -p \"test/fetch/*.js\" && npm run test:webidl && npm run test:busboy", "test:interceptors": "borp -p \"test/interceptors/*.js\"", "test:jest": "cross-env NODE_V8_COVERAGE= jest", "test:unit": "borp --expose-gc -p \"test/*.js\"", From fae667bd498a202580d4da9ae6cf5f3a05c71988 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sat, 4 May 2024 09:58:11 +0200 Subject: [PATCH 08/25] fixup Signed-off-by: Matteo Collina --- .github/workflows/nodejs.yml | 9 +++++---- test/fetch/fire-and-forget.js | 5 ++++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index a8fb052176c..58b4cd170b4 100644 --- a/.github/workflows/nodejs.yml +++ b/.github/workflows/nodejs.yml @@ -37,13 +37,13 @@ jobs: - name: Checkout uses: actions/checkout@9bb56186c3b09b4f86b1c65136769dd318469633 # v4.1.2 with: - persist-credentials: false + persist-credentials: false - name: Setup Node.js uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2 with: node-version: lts/* - + - name: Install dependencies run: npm install @@ -59,6 +59,7 @@ jobs: - 18 - 20 - 21 + - 22 runs-on: - ubuntu-latest - windows-latest @@ -168,13 +169,13 @@ jobs: - name: Checkout uses: actions/checkout@9bb56186c3b09b4f86b1c65136769dd318469633 # v4.1.2 with: - persist-credentials: false + persist-credentials: false - name: Setup Node.js uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2 with: node-version: lts/* - + - name: Install dependencies run: npm install diff --git a/test/fetch/fire-and-forget.js b/test/fetch/fire-and-forget.js index 4cf86b31064..241fdc381fd 100644 --- a/test/fetch/fire-and-forget.js +++ b/test/fetch/fire-and-forget.js @@ -10,7 +10,10 @@ const { closeServerAsPromise } = require('../utils/node-http') const blob = randomFillSync(new Uint8Array(1024 * 512)) const fmt = new Intl.NumberFormat() -test('does not need the body to be consumed to continue', { timeout: 120_1000 }, async (t) => { +// Enable when/if FinalizationRegistry in Node.js 18 becomes stable again +const isNode18 = process.version.startsWith('v18') + +test('does not need the body to be consumed to continue', { timeout: 120_000, skip: isNode18 }, async (t) => { const agent = new Agent({ keepAliveMaxTimeout: 10, keepAliveTimeoutThreshold: 10 From 41688473970982e06d0e94e96ac010024045c9a7 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sat, 4 May 2024 10:00:46 +0200 Subject: [PATCH 09/25] fixup Signed-off-by: Matteo Collina --- test/fetch/fire-and-forget.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/fetch/fire-and-forget.js b/test/fetch/fire-and-forget.js index 241fdc381fd..f6f02c1ad67 100644 --- a/test/fetch/fire-and-forget.js +++ b/test/fetch/fire-and-forget.js @@ -35,6 +35,8 @@ test('does not need the body to be consumed to continue', { timeout: 120_000, sk const delay = 0 let total = 0 while (total < 10000) { + // eslint-disable-next-line no-undef + gc(true) const array = new Array(batch) for (let i = 0; i < batch; i++) { array[i] = fetch(url).catch(() => {}) From bcba422fd39bd014bbf6b4318d19c5c85fd4e39f Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sat, 4 May 2024 10:02:22 +0200 Subject: [PATCH 10/25] fixup Signed-off-by: Matteo Collina --- test/fetch/fire-and-forget.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/fetch/fire-and-forget.js b/test/fetch/fire-and-forget.js index f6f02c1ad67..3bcf93c7145 100644 --- a/test/fetch/fire-and-forget.js +++ b/test/fetch/fire-and-forget.js @@ -8,7 +8,6 @@ const { createServer } = require('node:http') const { closeServerAsPromise } = require('../utils/node-http') const blob = randomFillSync(new Uint8Array(1024 * 512)) -const fmt = new Intl.NumberFormat() // Enable when/if FinalizationRegistry in Node.js 18 becomes stable again const isNode18 = process.version.startsWith('v18') @@ -48,7 +47,7 @@ test('does not need the body to be consumed to continue', { timeout: 120_000, sk 'RSS', (process.memoryUsage.rss() / 1024 / 1024) | 0, 'MB after', - fmt.format((total += batch)) + ' fetch() requests' + (total += batch) + ' fetch() requests' ) } }) From e233b8f2a3302ceb019c10087a758e5bdb45877b Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sat, 4 May 2024 22:07:48 +0200 Subject: [PATCH 11/25] fixup Signed-off-by: Matteo Collina --- package.json | 2 +- test/fetch/fire-and-forget.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index ff61d61cdc5..65d976b2195 100644 --- a/package.json +++ b/package.json @@ -77,7 +77,7 @@ "test:eventsource:nobuild": "borp --expose-gc -p \"test/eventsource/*.js\"", "test:fuzzing": "node test/fuzzing/fuzzing.test.js", "test:fetch": "npm run build:node && npm run test:fetch:nobuild", - "test:fetch:nobuild": "borp --timeout 120000 --expose-gc -p \"test/fetch/*.js\" && npm run test:webidl && npm run test:busboy", + "test:fetch:nobuild": "borp --timeout 180000 --expose-gc -p \"test/fetch/*.js\" && npm run test:webidl && npm run test:busboy", "test:interceptors": "borp -p \"test/interceptors/*.js\"", "test:jest": "cross-env NODE_V8_COVERAGE= jest", "test:unit": "borp --expose-gc -p \"test/*.js\"", diff --git a/test/fetch/fire-and-forget.js b/test/fetch/fire-and-forget.js index 3bcf93c7145..b72365c4adb 100644 --- a/test/fetch/fire-and-forget.js +++ b/test/fetch/fire-and-forget.js @@ -12,7 +12,7 @@ const blob = randomFillSync(new Uint8Array(1024 * 512)) // Enable when/if FinalizationRegistry in Node.js 18 becomes stable again const isNode18 = process.version.startsWith('v18') -test('does not need the body to be consumed to continue', { timeout: 120_000, skip: isNode18 }, async (t) => { +test('does not need the body to be consumed to continue', { timeout: 180_000, skip: isNode18 }, async (t) => { const agent = new Agent({ keepAliveMaxTimeout: 10, keepAliveTimeoutThreshold: 10 From 66375852eb34c6ba276d4cdac118fea893d841ee Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sat, 4 May 2024 22:12:01 +0200 Subject: [PATCH 12/25] fixup Signed-off-by: Matteo Collina --- lib/web/fetch/index.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/web/fetch/index.js b/lib/web/fetch/index.js index f475606b352..5fbef495ad6 100644 --- a/lib/web/fetch/index.js +++ b/lib/web/fetch/index.js @@ -1896,7 +1896,11 @@ async function httpNetworkFetch ( // 12. Let cancelAlgorithm be an algorithm that aborts fetchParams’s // controller with reason, given reason. const cancelAlgorithm = (reason) => { - fetchParams.controller.abort(reason) + // If the aborted fetch was already terminated, then we do not + // need to do anything. + if (!isCancelled(fetchParams)) { + fetchParams.controller.abort(reason) + } } // 13. Let highWaterMark be a non-negative, non-NaN number, chosen by From ce448e514e06a5bb1ed786c6fec96af4a3f95c52 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sun, 5 May 2024 00:53:31 +0200 Subject: [PATCH 13/25] fixup Signed-off-by: Matteo Collina --- lib/web/fetch/response.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/web/fetch/response.js b/lib/web/fetch/response.js index 224f25b8df8..81c32fe3e51 100644 --- a/lib/web/fetch/response.js +++ b/lib/web/fetch/response.js @@ -36,11 +36,13 @@ let registry if (hasFinalizationRegistry) { registry = new FinalizationRegistry((stream) => { if (!stream.locked && !isDisturbed(stream) && !isErrored(stream)) { - stream.cancel('Response object has been garbage collected') + stream.cancel('Response object has been garbage collected').catch(noop) } }) } +function noop () {} + // https://fetch.spec.whatwg.org/#response-class class Response { // Creates network error Response. From f2a9b84ddda748b983e88a45804bd11f2574f5a1 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sun, 5 May 2024 01:03:39 +0200 Subject: [PATCH 14/25] fixup Signed-off-by: Matteo Collina --- test/fetch/cookies.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/fetch/cookies.js b/test/fetch/cookies.js index 3bc69c6837b..048cfbc9bef 100644 --- a/test/fetch/cookies.js +++ b/test/fetch/cookies.js @@ -2,15 +2,22 @@ const { once } = require('node:events') const { createServer } = require('node:http') -const { test } = require('node:test') +const { test, beforeEach } = require('node:test') const assert = require('node:assert') const { tspl } = require('@matteo.collina/tspl') -const { Client, fetch, Headers } = require('../..') +const { Client, fetch, Headers, Agent, setGlobalDispatcher } = require('../..') const { closeServerAsPromise } = require('../utils/node-http') const pem = require('https-pem') const { createSecureServer } = require('node:http2') const { closeClientAndServerAsPromise } = require('../utils/node-http') +beforeEach(() => { + setGlobalDispatcher(new Agent({ + keepAliveMaxTimeout: 10, + keepAliveTimeoutThreshold: 10 + })) +}) + test('Can receive set-cookie headers from a server using fetch - issue #1262', async (t) => { const server = createServer((req, res) => { res.setHeader('set-cookie', 'name=value; Domain=example.com') From a042e86ac51f2f1d53bf565490a7fd4b92c780db Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 6 May 2024 18:38:06 +0200 Subject: [PATCH 15/25] fixup Signed-off-by: Matteo Collina --- test/fetch/cookies.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/fetch/cookies.js b/test/fetch/cookies.js index 048cfbc9bef..fe7f87ffbc0 100644 --- a/test/fetch/cookies.js +++ b/test/fetch/cookies.js @@ -11,11 +11,16 @@ const pem = require('https-pem') const { createSecureServer } = require('node:http2') const { closeClientAndServerAsPromise } = require('../utils/node-http') +let agent beforeEach(() => { - setGlobalDispatcher(new Agent({ + if (agent) { + agent.destroy() + } + agent = new Agent({ keepAliveMaxTimeout: 10, keepAliveTimeoutThreshold: 10 - })) + }) + setGlobalDispatcher(agent) }) test('Can receive set-cookie headers from a server using fetch - issue #1262', async (t) => { From e47d71fc5345ef4ad1d28b2ed02a0a1b3b9038d4 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 6 May 2024 19:23:45 +0200 Subject: [PATCH 16/25] fixup Signed-off-by: Matteo Collina --- test/fetch/cookies.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/fetch/cookies.js b/test/fetch/cookies.js index fe7f87ffbc0..81edaa1abc1 100644 --- a/test/fetch/cookies.js +++ b/test/fetch/cookies.js @@ -59,6 +59,7 @@ test('Can send cookies to a server with fetch - issue #1463', async (t) => { ] for (const headers of headersInit) { + console.log(headers) await fetch(`http://localhost:${server.address().port}`, { headers }) } }) From ce8b2601789b5ad4a0c9225fd6c5f95921fd03ca Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 6 May 2024 19:29:04 +0200 Subject: [PATCH 17/25] fixup Signed-off-by: Matteo Collina --- lib/web/fetch/index.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/web/fetch/index.js b/lib/web/fetch/index.js index 5fbef495ad6..58ff147ee6c 100644 --- a/lib/web/fetch/index.js +++ b/lib/web/fetch/index.js @@ -2090,8 +2090,16 @@ async function httpNetworkFetch ( if (connection.destroyed) { abort(new DOMException('The operation was aborted.', 'AbortError')) } else { - fetchParams.controller.on('terminated', abort) - this.abort = connection.abort = abort + function abort2 (err) { + process._rawDebug('>>> abort2 <<<') + // If we have already completed the fetch, we don't need to abort + // + // if (!fetchParams.controller.ended) { + abort(err) + // } + } + fetchParams.controller.on('terminated', abort2) + this.abort = connection.abort = abort2 } // Set timingInfo’s final network-request start time to the coarsened shared current time given From 115944f42957618e59c1c4c8fbe4e2bed1a13136 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 6 May 2024 19:47:42 +0200 Subject: [PATCH 18/25] fixup Signed-off-by: Matteo Collina --- lib/web/fetch/index.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/web/fetch/index.js b/lib/web/fetch/index.js index 58ff147ee6c..56722fd73aa 100644 --- a/lib/web/fetch/index.js +++ b/lib/web/fetch/index.js @@ -2076,6 +2076,7 @@ async function httpNetworkFetch ( { body: null, abort: null, + completed: false, onConnect (abort) { // TODO (fix): Do we need connection here? @@ -2090,13 +2091,10 @@ async function httpNetworkFetch ( if (connection.destroyed) { abort(new DOMException('The operation was aborted.', 'AbortError')) } else { - function abort2 (err) { - process._rawDebug('>>> abort2 <<<') - // If we have already completed the fetch, we don't need to abort - // - // if (!fetchParams.controller.ended) { - abort(err) - // } + const abort2 = (err) => { + if (!this.completed) { + abort(err) + } } fetchParams.controller.on('terminated', abort2) this.abort = connection.abort = abort2 @@ -2213,7 +2211,9 @@ async function httpNetworkFetch ( fetchParams.controller.off('terminated', fetchParams.controller.onAborted) } + // We need to use two variables because somebody else is also writing to fetchParas.controller.ended fetchParams.controller.ended = true + this.completed = true this.body.push(null) }, From 5ec60996d8b66ec5f2fcb308914beedfda9cff84 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 6 May 2024 22:18:39 +0200 Subject: [PATCH 19/25] fixup Signed-off-by: Matteo Collina --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 65d976b2195..29f3d88ec95 100644 --- a/package.json +++ b/package.json @@ -77,7 +77,7 @@ "test:eventsource:nobuild": "borp --expose-gc -p \"test/eventsource/*.js\"", "test:fuzzing": "node test/fuzzing/fuzzing.test.js", "test:fetch": "npm run build:node && npm run test:fetch:nobuild", - "test:fetch:nobuild": "borp --timeout 180000 --expose-gc -p \"test/fetch/*.js\" && npm run test:webidl && npm run test:busboy", + "test:fetch:nobuild": "borp --timeout 180000 --expose-gc --concurrency 1 -p \"test/fetch/*.js\" && npm run test:webidl && npm run test:busboy", "test:interceptors": "borp -p \"test/interceptors/*.js\"", "test:jest": "cross-env NODE_V8_COVERAGE= jest", "test:unit": "borp --expose-gc -p \"test/*.js\"", From 83daf214ab5534a3d1130068cd1ebb9d06ceaeda Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 7 May 2024 09:36:09 +0200 Subject: [PATCH 20/25] fixup Signed-off-by: Matteo Collina --- test/fetch/cookies.js | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/test/fetch/cookies.js b/test/fetch/cookies.js index 81edaa1abc1..3bc69c6837b 100644 --- a/test/fetch/cookies.js +++ b/test/fetch/cookies.js @@ -2,27 +2,15 @@ const { once } = require('node:events') const { createServer } = require('node:http') -const { test, beforeEach } = require('node:test') +const { test } = require('node:test') const assert = require('node:assert') const { tspl } = require('@matteo.collina/tspl') -const { Client, fetch, Headers, Agent, setGlobalDispatcher } = require('../..') +const { Client, fetch, Headers } = require('../..') const { closeServerAsPromise } = require('../utils/node-http') const pem = require('https-pem') const { createSecureServer } = require('node:http2') const { closeClientAndServerAsPromise } = require('../utils/node-http') -let agent -beforeEach(() => { - if (agent) { - agent.destroy() - } - agent = new Agent({ - keepAliveMaxTimeout: 10, - keepAliveTimeoutThreshold: 10 - }) - setGlobalDispatcher(agent) -}) - test('Can receive set-cookie headers from a server using fetch - issue #1262', async (t) => { const server = createServer((req, res) => { res.setHeader('set-cookie', 'name=value; Domain=example.com') @@ -59,7 +47,6 @@ test('Can send cookies to a server with fetch - issue #1463', async (t) => { ] for (const headers of headersInit) { - console.log(headers) await fetch(`http://localhost:${server.address().port}`, { headers }) } }) From 5ead0e6c9e28953b3917bb9b4850f6abdcbaafcc Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 7 May 2024 09:46:21 +0200 Subject: [PATCH 21/25] fixup Signed-off-by: Matteo Collina --- .github/workflows/nodejs.yml | 9 ++++----- lib/web/fetch/index.js | 12 ++---------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index 58b4cd170b4..a8fb052176c 100644 --- a/.github/workflows/nodejs.yml +++ b/.github/workflows/nodejs.yml @@ -37,13 +37,13 @@ jobs: - name: Checkout uses: actions/checkout@9bb56186c3b09b4f86b1c65136769dd318469633 # v4.1.2 with: - persist-credentials: false + persist-credentials: false - name: Setup Node.js uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2 with: node-version: lts/* - + - name: Install dependencies run: npm install @@ -59,7 +59,6 @@ jobs: - 18 - 20 - 21 - - 22 runs-on: - ubuntu-latest - windows-latest @@ -169,13 +168,13 @@ jobs: - name: Checkout uses: actions/checkout@9bb56186c3b09b4f86b1c65136769dd318469633 # v4.1.2 with: - persist-credentials: false + persist-credentials: false - name: Setup Node.js uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2 with: node-version: lts/* - + - name: Install dependencies run: npm install diff --git a/lib/web/fetch/index.js b/lib/web/fetch/index.js index 56722fd73aa..5fbef495ad6 100644 --- a/lib/web/fetch/index.js +++ b/lib/web/fetch/index.js @@ -2076,7 +2076,6 @@ async function httpNetworkFetch ( { body: null, abort: null, - completed: false, onConnect (abort) { // TODO (fix): Do we need connection here? @@ -2091,13 +2090,8 @@ async function httpNetworkFetch ( if (connection.destroyed) { abort(new DOMException('The operation was aborted.', 'AbortError')) } else { - const abort2 = (err) => { - if (!this.completed) { - abort(err) - } - } - fetchParams.controller.on('terminated', abort2) - this.abort = connection.abort = abort2 + fetchParams.controller.on('terminated', abort) + this.abort = connection.abort = abort } // Set timingInfo’s final network-request start time to the coarsened shared current time given @@ -2211,9 +2205,7 @@ async function httpNetworkFetch ( fetchParams.controller.off('terminated', fetchParams.controller.onAborted) } - // We need to use two variables because somebody else is also writing to fetchParas.controller.ended fetchParams.controller.ended = true - this.completed = true this.body.push(null) }, From 6c310d5df86c8ad8949613ab0ded51dc30f17b5f Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 7 May 2024 12:24:34 +0200 Subject: [PATCH 22/25] fixup Signed-off-by: Matteo Collina --- lib/dispatcher/client-h2.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/dispatcher/client-h2.js b/lib/dispatcher/client-h2.js index cd63bcda2f2..21eaa93042f 100644 --- a/lib/dispatcher/client-h2.js +++ b/lib/dispatcher/client-h2.js @@ -583,7 +583,9 @@ function writeStream ({ abort, socket, expectsPayload, h2stream, body, client, r (err) => { if (err) { util.destroy(pipe, err) - abort(err) + if (typeof abort === 'function') { + abort(err) + } } else { util.removeAllListeners(pipe) request.onRequestSent() @@ -628,7 +630,9 @@ async function writeBlob ({ abort, h2stream, body, client, request, socket, cont client[kResume]() } catch (err) { - abort(err) + if (typeof abort === 'function') { + abort(err) + } } } @@ -682,7 +686,9 @@ async function writeIterable ({ abort, h2stream, body, client, request, socket, client[kResume]() } catch (err) { - abort(err) + if (typeof abort === 'function') { + abort(err) + } } finally { h2stream .off('close', onDrain) From 5715494f3f89a1d8b4d5223295d0d8d579f32749 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 7 May 2024 12:29:56 +0200 Subject: [PATCH 23/25] fixup Signed-off-by: Matteo Collina --- lib/dispatcher/client-h2.js | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/lib/dispatcher/client-h2.js b/lib/dispatcher/client-h2.js index e99c5aa114e..cd63bcda2f2 100644 --- a/lib/dispatcher/client-h2.js +++ b/lib/dispatcher/client-h2.js @@ -524,7 +524,6 @@ function writeH2 (client, request) { } } else if (util.isStream(body)) { writeStream({ - abort, body, client, request, @@ -536,7 +535,6 @@ function writeH2 (client, request) { }) } else if (util.isIterable(body)) { writeIterable({ - abort, body, client, request, @@ -585,9 +583,7 @@ function writeStream ({ abort, socket, expectsPayload, h2stream, body, client, r (err) => { if (err) { util.destroy(pipe, err) - if (typeof abort === 'function') { - abort(err) - } + abort(err) } else { util.removeAllListeners(pipe) request.onRequestSent() @@ -632,9 +628,7 @@ async function writeBlob ({ abort, h2stream, body, client, request, socket, cont client[kResume]() } catch (err) { - if (typeof abort === 'function') { - abort(err) - } + abort(err) } } @@ -688,9 +682,7 @@ async function writeIterable ({ abort, h2stream, body, client, request, socket, client[kResume]() } catch (err) { - if (typeof abort === 'function') { - abort(err) - } + abort(err) } finally { h2stream .off('close', onDrain) From 2ef6d6ca46932b6727bec775b58934863ed468af Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 7 May 2024 12:34:20 +0200 Subject: [PATCH 24/25] fixup Signed-off-by: Matteo Collina --- lib/dispatcher/client-h2.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/dispatcher/client-h2.js b/lib/dispatcher/client-h2.js index cd63bcda2f2..a1137724a69 100644 --- a/lib/dispatcher/client-h2.js +++ b/lib/dispatcher/client-h2.js @@ -403,7 +403,7 @@ function writeH2 (client, request) { // where the stream has been assigned, but the request has been aborted // the request remains in-flight and headers hasn't been received yet // for those scenarios, best effort is to destroy the stream immediately - // as there's no value to keep it open. + // is there's no value to keep it open. if (request.aborted) { const err = new RequestAbortedError() util.errorRequest(client, request, err) @@ -524,6 +524,7 @@ function writeH2 (client, request) { } } else if (util.isStream(body)) { writeStream({ + abort, body, client, request, @@ -535,6 +536,7 @@ function writeH2 (client, request) { }) } else if (util.isIterable(body)) { writeIterable({ + abort, body, client, request, From 728412f4b56ea018abd24b2ef69cc0aa4876a9dd Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 7 May 2024 12:57:23 +0200 Subject: [PATCH 25/25] Update lib/dispatcher/client-h2.js Co-authored-by: Aras Abbasi --- lib/dispatcher/client-h2.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/dispatcher/client-h2.js b/lib/dispatcher/client-h2.js index a1137724a69..c78fe602c48 100644 --- a/lib/dispatcher/client-h2.js +++ b/lib/dispatcher/client-h2.js @@ -403,7 +403,7 @@ function writeH2 (client, request) { // where the stream has been assigned, but the request has been aborted // the request remains in-flight and headers hasn't been received yet // for those scenarios, best effort is to destroy the stream immediately - // is there's no value to keep it open. + // as there's no value to keep it open. if (request.aborted) { const err = new RequestAbortedError() util.errorRequest(client, request, err)