From 17163ead3326a2eda3253c392474c777405a04c7 Mon Sep 17 00:00:00 2001 From: Archkon <180910180+Archkon@users.noreply.github.com> Date: Sun, 12 Jul 2026 19:16:52 +0800 Subject: [PATCH 1/6] http: do not emit socket errors after complete response Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com> PR-URL: https://github.com/nodejs/node/pull/64278 Fixes: https://github.com/nodejs/node/issues/64272 Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- lib/_http_client.js | 16 +++++- ...est-http-client-complete-response-reset.js | 55 +++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-http-client-complete-response-reset.js diff --git a/lib/_http_client.js b/lib/_http_client.js index 891da4e3f9a984..8fdcabaa2a8fd0 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -741,10 +741,24 @@ function socketErrorListener(err) { debug('SOCKET ERROR:', err.message, err.stack); if (req) { + const res = req.res; + const isUserDestroyError = err === req[kError] || err === res?.errored; + // For Safety. Some additional errors might fire later on // and we need to make sure we don't double-fire the error event. socket._hadError = true; - emitErrorEvent(req, err); + // Before a response exists, the request itself failed. Once a response + // exists, socket teardown belongs to the IncomingMessage and is finalized + // by socketCloseListener. Preserve errors explicitly used to destroy the + // request or response, which have historically been emitted on the request. + if (!res || isUserDestroyError) { + emitErrorEvent(req, err); + } + + if (res) { + socket.destroy(); + return; + } } const parser = socket.parser; diff --git a/test/parallel/test-http-client-complete-response-reset.js b/test/parallel/test-http-client-complete-response-reset.js new file mode 100644 index 00000000000000..aa9337dbcc5206 --- /dev/null +++ b/test/parallel/test-http-client-complete-response-reset.js @@ -0,0 +1,55 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const BODY = Buffer.alloc(2 * 1024 * 1024); + +const server = http.createServer(common.mustCall((request, response) => { + let received = 0; + + request.on('data', function onData(chunk) { + received += chunk.length; + request.off('data', onData); + request.destroy(); + }); + + response.writeHead(413, { 'content-length': 0 }); + response.end(); + + request.on('end', common.mustNotCall()); + request.on('close', common.mustCall(() => { + assert.strictEqual(request.complete, false); + assert.ok(received > 0); + })); +})); + +server.listen(0, common.mustCall(() => { + let response; + const req = http.request({ + method: 'POST', + port: server.address().port, + headers: { + 'content-type': 'application/json', + 'content-length': BODY.length, + }, + }, common.mustCall((res) => { + response = res; + assert.strictEqual(res.statusCode, 413); + // Deliberately do not consume the response body. A response that has + // already been received should not be followed by a late ClientRequest + // socket error. + req.write(BODY); + req.end(); + })); + + req.on('error', common.mustNotCall()); + req.on('close', common.mustCall(() => { + assert(response); + assert.strictEqual(response.complete, true); + server.close(); + })); + + req.flushHeaders(); +})); From 892976d17a4060cf647eb9613ce643e6b597daaa Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Sun, 12 Jul 2026 14:45:05 +0300 Subject: [PATCH 2/6] test_runner: add context.log() and test:log event Add a log(message[, data]) method to TestContext and SuiteContext that emits a new test:log event. Unlike test:diagnostic, which is buffered so it is emitted in the order tests are defined, test:log is emitted immediately, in the order tests execute, including under process isolation where it bypasses the per-file declaration order buffer. This gives reporters that render the test tree unbuffered a live, attributed logging channel that captured stdout cannot provide under concurrency. The event carries the message, an optional opaque structured payload that the runner passes through untouched, and the emitting test's name, testId, parentId, nesting, and location. Built-in reporters render it the same way they render test:diagnostic. Signed-off-by: Moses Atlow PR-URL: https://github.com/nodejs/node/pull/64389 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chemi Atlow --- doc/api/test.md | 81 +++++++++++++++++++ lib/internal/test_runner/reporter/junit.js | 3 +- lib/internal/test_runner/reporter/spec.js | 5 +- lib/internal/test_runner/reporter/tap.js | 1 + lib/internal/test_runner/reporter/utils.js | 4 + lib/internal/test_runner/runner.js | 2 +- lib/internal/test_runner/test.js | 15 ++++ lib/internal/test_runner/tests_stream.js | 13 +++ .../execution-ordered-bypass/fast-fail.mjs | 3 +- .../fixtures/test-runner/log-events/basic.mjs | 19 +++++ .../test-runner-execution-ordered-bypass.mjs | 12 +++ test/parallel/test-runner-log.mjs | 80 ++++++++++++++++++ 12 files changed, 233 insertions(+), 5 deletions(-) create mode 100644 test/fixtures/test-runner/log-events/basic.mjs create mode 100644 test/parallel/test-runner-log.mjs diff --git a/doc/api/test.md b/doc/api/test.md index 4a44fa91e8ea09..94f35abd03a04b 100644 --- a/doc/api/test.md +++ b/doc/api/test.md @@ -3478,6 +3478,10 @@ ordered events, emitted immediately as the tests execute. | [`'test:fail'`][] | [`'test:complete'`][] (`details.passed` is `false`) | | [`'test:plan'`][] | | | [`'test:diagnostic'`][] | | +| | [`'test:log'`][] | + +[`'test:log'`][] is deliberately execution ordered only: it is the live +counterpart of [`'test:diagnostic'`][]'s buffered reporting. File scoped and global events are always emitted immediately, in execution order. @@ -3747,6 +3751,38 @@ When using process isolation (the default), the test name will be the file path since the parent runner only knows about file-level tests. When using `--test-isolation=none`, the actual test name is shown. +### Event: `'test:log'` + + + +* `data` {Object} + * `column` {number|undefined} The column number where the test is defined, or + `undefined` if the test was run through the REPL. + * `data` {any} The structured payload passed to [`context.log`][], or + `undefined` if none was provided. The test runner does not interpret this + value. + * `entryFile` {string|undefined} The path of the test file that was + executed as the entry point of the child process that emitted this event. + Only present when tests run with process isolation. May differ from + `file` when the test is defined in a module imported by the entry file. + * `file` {string|undefined} The path of the test file, + `undefined` if test was run through the REPL. + * `line` {number|undefined} The line number where the test is defined, or + `undefined` if the test was run through the REPL. + * `message` {string} The log message. + * `name` {string} The test name. + * `nesting` {number} The nesting level of the test. + * `parentId` {number|undefined} The `testId` of the enclosing test, or + `undefined` for top-level tests. + * `testId` {number} A numeric identifier for the test instance that emitted + the log message. + +Emitted when [`context.log`][] is called. Unlike [`'test:diagnostic'`][], +this event is emitted immediately, in the order that the tests execute, +making it suitable for reporters that render test output unbuffered. + ### Event: `'test:pass'` * `data` {Object} @@ -4272,6 +4308,29 @@ test('top level test', (t) => { }); ``` +### `context.log(message[, data])` + + + +* `message` {string} Message to be reported. +* `data` {any} Optional structured payload attached to the message. The test + runner passes it through untouched. When tests run with process isolation, + this value must be compatible with the [HTML structured clone algorithm][]. + +This function is used to write a log message to the output. Unlike +[`context.diagnostic`][], the resulting [`'test:log'`][] event is emitted +immediately, in the order that the tests execute, rather than being buffered +until the test reports its results. This function does not return a value. + +```js +test('top level test', (t) => { + t.log('fetched user', { userId: 42 }); + t.log('retrying flaky endpoint', { attempt: 3 }); +}); +``` + ### `context.filePath` + +* `message` {string} Message to be reported. +* `data` {any} Optional structured payload attached to the message. The test + runner passes it through untouched. + +Write a log message to the output. The resulting [`'test:log'`][] event is +emitted immediately, in the order that the tests execute. + +```js +test.describe('my suite', (suite) => { + suite.log('Suite log message'); +}); +``` + +[HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm [TAP]: https://testanything.org/ [Test tags]: #test-tags [`'test:complete'`]: #event-testcomplete @@ -4770,6 +4849,7 @@ test.describe('my suite', (suite) => { [`'test:enqueue'`]: #event-testenqueue [`'test:fail'`]: #event-testfail [`'test:interrupted'`]: #event-testinterrupted +[`'test:log'`]: #event-testlog [`'test:pass'`]: #event-testpass [`'test:plan'`]: #event-testplan [`'test:start'`]: #event-teststart @@ -4805,6 +4885,7 @@ test.describe('my suite', (suite) => { [`TracingChannel`]: diagnostics_channel.md#class-tracingchannel [`assert.throws`]: assert.md#assertthrowsfn-error-message [`context.diagnostic`]: #contextdiagnosticmessage +[`context.log`]: #contextlogmessage-data [`context.skip`]: #contextskipmessage [`context.tags`]: #contexttags [`context.todo`]: #contexttodomessage diff --git a/lib/internal/test_runner/reporter/junit.js b/lib/internal/test_runner/reporter/junit.js index 130ff60af405ad..ed25a4bd5fbd7c 100644 --- a/lib/internal/test_runner/reporter/junit.js +++ b/lib/internal/test_runner/reporter/junit.js @@ -154,7 +154,8 @@ module.exports = async function* junitReporter(source) { } break; } - case 'test:diagnostic': { + case 'test:diagnostic': + case 'test:log': { const parent = currentSuite?.children ?? roots; ArrayPrototypePush(parent, { __proto__: null, nesting: event.data.nesting, comment: event.data.message, diff --git a/lib/internal/test_runner/reporter/spec.js b/lib/internal/test_runner/reporter/spec.js index fce0754e25061a..7f5920c7b38a30 100644 --- a/lib/internal/test_runner/reporter/spec.js +++ b/lib/internal/test_runner/reporter/spec.js @@ -91,8 +91,9 @@ class SpecReporter extends Transform { case 'test:stderr': case 'test:stdout': return data.message; - case 'test:diagnostic':{ - const diagnosticColor = reporterColorMap[data.level] || reporterColorMap['test:diagnostic']; + case 'test:diagnostic': + case 'test:log': { + const diagnosticColor = reporterColorMap[data.level] || reporterColorMap[type]; return `${diagnosticColor}${indent(data.nesting)}${reporterUnicodeSymbolMap[type]}${data.message}${colors.white}\n`; } case 'test:coverage': diff --git a/lib/internal/test_runner/reporter/tap.js b/lib/internal/test_runner/reporter/tap.js index c3bbd1d144eb35..2bacba167570a8 100644 --- a/lib/internal/test_runner/reporter/tap.js +++ b/lib/internal/test_runner/reporter/tap.js @@ -56,6 +56,7 @@ async function * tapReporter(source) { } break; } case 'test:diagnostic': + case 'test:log': yield `${indent(data.nesting)}# ${tapEscape(data.message)}\n`; break; case 'test:coverage': diff --git a/lib/internal/test_runner/reporter/utils.js b/lib/internal/test_runner/reporter/utils.js index 67030680019838..995155c533c51d 100644 --- a/lib/internal/test_runner/reporter/utils.js +++ b/lib/internal/test_runner/reporter/utils.js @@ -21,6 +21,7 @@ const reporterUnicodeSymbolMap = { 'test:fail': '\u2716 ', 'test:pass': '\u2714 ', 'test:diagnostic': '\u2139 ', + 'test:log': '\u2139 ', 'test:coverage': '\u2139 ', 'arrow:right': '\u25B6 ', 'hyphen:minus': '\uFE63 ', @@ -38,6 +39,9 @@ const reporterColorMap = { get 'test:diagnostic'() { return colors.blue; }, + get 'test:log'() { + return colors.blue; + }, get 'info'() { return colors.blue; }, diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index 7f50cc8521bcda..dc6529c220539a 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -130,7 +130,7 @@ const kCanceledTests = new SafeSet() // Execution-ordered events are forwarded immediately, bypassing the // per-file declaration-order buffer. const kExecutionOrderedEvents = new SafeSet() - .add('test:enqueue').add('test:dequeue').add('test:complete'); + .add('test:enqueue').add('test:dequeue').add('test:complete').add('test:log'); let kResistStopPropagation; diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index d19d6349f104bf..1b9faad58a68b5 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -80,6 +80,7 @@ const { validateNumber, validateObject, validateOneOf, + validateString, validateUint32, } = require('internal/validators'); const { @@ -315,6 +316,10 @@ class TestContext { this.#test.diagnostic(message); } + log(message, data) { + this.#test.log(message, data); + } + plan(count, options = kEmptyObject) { if (this.#test.plan !== null) { throw new ERR_TEST_FAILURE( @@ -523,6 +528,10 @@ class SuiteContext { diagnostic(message) { this.#suite.diagnostic(message); } + + log(message, data) { + this.#suite.log(message, data); + } } function parseExpectFailure(expectFailure) { @@ -1198,6 +1207,12 @@ class Test extends AsyncResource { ArrayPrototypePush(this.diagnostics, message); } + log(message, data) { + validateString(message, 'message'); + this.reporter.log(this.nesting, this.loc, message, data, + this.name, this.testId, this.parent?.testId); + } + start() { this.applyFilters(); diff --git a/lib/internal/test_runner/tests_stream.js b/lib/internal/test_runner/tests_stream.js index 8be33f90dfa0f2..7fb514fb99e2f5 100644 --- a/lib/internal/test_runner/tests_stream.js +++ b/lib/internal/test_runner/tests_stream.js @@ -139,6 +139,19 @@ class TestsStream extends Readable { }); } + log(nesting, loc, message, data, name, testId, parentId) { + this[kEmitMessage]('test:log', { + __proto__: null, + name, + nesting, + testId, + parentId, + message, + data, + ...loc, + }); + } + diagnostic(nesting, loc, message, level = 'info') { this[kEmitMessage]('test:diagnostic', { __proto__: null, diff --git a/test/fixtures/test-runner/execution-ordered-bypass/fast-fail.mjs b/test/fixtures/test-runner/execution-ordered-bypass/fast-fail.mjs index 74b77682b6821d..6c99569e11533c 100644 --- a/test/fixtures/test-runner/execution-ordered-bypass/fast-fail.mjs +++ b/test/fixtures/test-runner/execution-ordered-bypass/fast-fail.mjs @@ -1,6 +1,7 @@ import { test } from 'node:test'; import assert from 'node:assert'; -test('fast-fail', () => { +test('fast-fail', (t) => { + t.log('live'); assert.fail('fast'); }); diff --git a/test/fixtures/test-runner/log-events/basic.mjs b/test/fixtures/test-runner/log-events/basic.mjs new file mode 100644 index 00000000000000..f19a951b982980 --- /dev/null +++ b/test/fixtures/test-runner/log-events/basic.mjs @@ -0,0 +1,19 @@ +import { suite, test } from 'node:test'; +import { setTimeout } from 'node:timers/promises'; + +suite('my suite', (s) => { + s.log('suite message'); + test('in suite', () => {}); +}); + +test('parent', { concurrency: 2 }, async (t) => { + await Promise.all([ + t.test('slow', async () => { + await setTimeout(200); + }), + t.test('logger', (t) => { + t.log('hello', { foo: 1 }); + t.log('warned', { level: 'warn', attempt: 2 }); + }), + ]); +}); diff --git a/test/parallel/test-runner-execution-ordered-bypass.mjs b/test/parallel/test-runner-execution-ordered-bypass.mjs index 85b468cae2a7e1..ac1c97ee007d68 100644 --- a/test/parallel/test-runner-execution-ordered-bypass.mjs +++ b/test/parallel/test-runner-execution-ordered-bypass.mjs @@ -35,6 +35,10 @@ test('execution-ordered events bypass FileTest declaration-order buffer', async } }); + stream.on('test:log', (data) => { + events.push(`log:${data.message}`); + }); + // eslint-disable-next-line no-unused-vars for await (const _ of stream); @@ -56,4 +60,12 @@ test('execution-ordered events bypass FileTest declaration-order buffer', async failFast > completeSlow, `test:fail for fast-fail should arrive after test:complete for slow; events=${events.join(', ')}`, ); + + // test:log is execution-ordered, so it must bypass the buffer too. + const logFast = events.indexOf('log:live'); + assert.notStrictEqual(logFast, -1); + assert.ok( + logFast < completeSlow, + `test:log for fast-fail should arrive before slow's test:complete; events=${events.join(', ')}`, + ); }); diff --git a/test/parallel/test-runner-log.mjs b/test/parallel/test-runner-log.mjs new file mode 100644 index 00000000000000..8e767e0f903c8e --- /dev/null +++ b/test/parallel/test-runner-log.mjs @@ -0,0 +1,80 @@ +// Flags: --no-warnings +import '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import assert from 'node:assert'; +import { spawnSync } from 'node:child_process'; +import { test, run } from 'node:test'; + +const fixture = fixtures.path('test-runner', 'log-events', 'basic.mjs'); + +test('t.log() validates its arguments', (t) => { + assert.throws(() => t.log(123), { code: 'ERR_INVALID_ARG_TYPE' }); +}); + +test('built-in reporters render test:log like diagnostics', () => { + const tap = spawnSync(process.execPath, ['--test', '--test-reporter=tap', fixture]); + assert.match(tap.stdout.toString(), /# hello/); + assert.match(tap.stdout.toString(), /# suite message/); + + const spec = spawnSync(process.execPath, ['--test', '--test-reporter=spec', fixture]); + assert.match(spec.stdout.toString(), /ℹ hello/); + assert.match(spec.stdout.toString(), /ℹ warned/); + + const junit = spawnSync(process.execPath, ['--test', '--test-reporter=junit', fixture]); + assert.match(junit.stdout.toString(), //); +}); + +test('test:log is emitted immediately with the expected payload', async () => { + const stream = run({ + files: [fixture], + isolation: 'none', + }); + + const logs = []; + const events = []; + + stream.on('test:log', (data) => { + logs.push(data); + events.push(`log:${data.message}`); + }); + + stream.on('test:pass', (data) => { + events.push(`pass:${data.name}`); + }); + + // eslint-disable-next-line no-unused-vars + for await (const _ of stream); + + assert.strictEqual(logs.length, 3); + + const suiteLog = logs.find((log) => log.message === 'suite message'); + assert.strictEqual(suiteLog.name, 'my suite'); + assert.strictEqual(suiteLog.data, undefined); + + const hello = logs.find((log) => log.message === 'hello'); + assert.strictEqual(hello.name, 'logger'); + assert.ok(!('level' in hello)); + assert.deepStrictEqual(hello.data, { foo: 1 }); + assert.strictEqual(typeof hello.nesting, 'number'); + assert.strictEqual(typeof hello.testId, 'number'); + assert.strictEqual(typeof hello.parentId, 'number'); + assert.strictEqual(typeof hello.file, 'string'); + assert.strictEqual(typeof hello.line, 'number'); + assert.strictEqual(typeof hello.column, 'number'); + + // The runner does not interpret data; conventions like a level live there. + const warned = logs.find((log) => log.message === 'warned'); + assert.ok(!('level' in warned)); + assert.deepStrictEqual(warned.data, { level: 'warn', attempt: 2 }); + + // 'logger' logs immediately while its earlier-declared sibling 'slow' is + // still running, so the log must arrive before slow's buffered test:pass. + const logIndex = events.indexOf('log:hello'); + const slowPassIndex = events.indexOf('pass:slow'); + assert.notStrictEqual(logIndex, -1); + assert.notStrictEqual(slowPassIndex, -1); + assert.ok( + logIndex < slowPassIndex, + `test:log should arrive before slow's buffered test:pass; events=${events.join(', ')}`, + ); +}); From a4821c8a7586ab5de2e701bf407626fe43e7fa0d Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 12 Jul 2026 14:59:42 +0200 Subject: [PATCH 3/6] tools: add option for `benchmark.yml` to post comment on PR Signed-off-by: Antoine du Hamel PR-URL: https://github.com/nodejs/node/pull/64395 Reviewed-By: Yagiz Nizipli Reviewed-By: Colin Ihrig Reviewed-By: Matteo Collina --- .github/workflows/benchmark.yml | 66 ++++++++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index b989dd1faaef45..48b3f1d6125975 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -25,11 +25,43 @@ on: type: number default: 30 description: How many times to repeat each benchmark + post-comment: + type: boolean + description: Post a comment linking to the run, with the aggregated result once known. + token: + type: string + description: A GitHub token to post comments cross repository (not recommended) permissions: contents: read jobs: + post-comment: + if: inputs.post-comment + outputs: + body: ${{ steps.comment.outputs.COMMENT_BODY }} + url: ${{ steps.comment.outputs.COMMENT_URL }} + runs-on: ubuntu-slim + permissions: + pull-requests: write + steps: + - name: Mark token input as sensitive + if: inputs.token != '' + run: echo "::add-mask::${{ inputs.token }}" + - name: Add link to the current run + id: comment + run: | + FILTER_IF_SET= + [ -z "$FILTER" ] || FILTER_IF_SET=" / $FILTER" + COMMENT_BODY="Benchmark GHA (${CATEGORIES}${FILTER_IF_SET}): ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}" + echo "COMMENT_BODY=$COMMENT_BODY" >> "$GITHUB_OUTPUT" + echo "COMMENT_URL=$(gh pr comment -R "${REPO:-$GITHUB_REPOSITORY}" "$PR_ID" --body "$COMMENT_BODY")" >> "$GITHUB_OUTPUT" + env: + CATEGORIES: ${{ inputs.category }} + FILTER: ${{ inputs.filter }} + GH_TOKEN: ${{ inputs.token || github.token }} + REPO: ${{ inputs.repo }} + PR_ID: ${{ inputs.pr_id }} build: strategy: fail-fast: true @@ -155,9 +187,13 @@ jobs: path: ${{ matrix.system }}.csv aggregate-results: - needs: build + needs: [build, post-comment] + if: ${{ always() && needs.build.result == 'success' && needs.post-comment.result == (inputs.post-comment && 'success' || 'skipped') }} name: Aggregate benchmark results runs-on: ubuntu-slim + permissions: + contents: read + pull-requests: write steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: @@ -200,4 +236,30 @@ jobs: echo "> [!WARNING] " echo "> Do not take GHA benchmark results as face value, always confirm them" echo "> using a dedicated machine, e.g. Jenkins CI." - ' | tee /dev/stderr >> "$GITHUB_STEP_SUMMARY" + ' | tee /dev/stderr ${{ inputs.post-comment && 'body.txt' || '' }} >> "$GITHUB_STEP_SUMMARY" + - name: Mark token input as sensitive + if: inputs.token != '' + run: echo "::add-mask::${{ inputs.token }}" + - name: Edit comment + if: inputs.post-comment + run: | + { + echo "$COMMENT_BODY" + echo + echo '
Results' + echo + cat body.txt + echo + echo '
' + } | jq -Rrcs '{ body: . }' | gh api \ + --method PATCH \ + -H "Accept: application/vnd.github+json" \ + -H "X-GitHub-Api-Version: 2026-03-10" \ + "/repos/${REPO:-$GITHUB_REPOSITORY}/issues/comments/${COMMENT_URL##*-}" \ + --input - + env: + GH_TOKEN: ${{ inputs.token || github.token }} + REPO: ${{ inputs.repo }} + COMMENT_BODY: ${{ needs.post-comment.outputs.body }} + COMMENT_URL: ${{ needs.post-comment.outputs.url }} + PR_ID: ${{ inputs.pr_id }} From 2d7fee04f4c77c2638d7450aa0a8cfb8a28f6fce Mon Sep 17 00:00:00 2001 From: Stefano Baghino Date: Sun, 12 Jul 2026 16:48:42 +0200 Subject: [PATCH 4/6] http: fix perf_hooks detail.req.url port and proxied path The perf_hooks HTTP client entry built the reported URL from the bare hostname, dropping non-default ports and IPv6 brackets, and appended the request path even after it had been rewritten to absolute-form for proxying, duplicating the protocol and authority. Report the connection authority captured at request creation and, for proxied requests, use the rewritten absolute-form target as-is. Fixes: https://github.com/nodejs/node/issues/59625 Signed-off-by: Stefano Baghino PR-URL: https://github.com/nodejs/node/pull/64311 Reviewed-By: Matteo Collina Reviewed-By: Joyee Cheung Reviewed-By: James M Snell Reviewed-By: theanarkh --- lib/_http_client.js | 13 +++- .../test-http-proxy-perf-hooks.mjs | 63 +++++++++++++++++++ test/parallel/test-http-perf_hooks.js | 17 +++-- 3 files changed, 86 insertions(+), 7 deletions(-) create mode 100644 test/client-proxy/test-http-proxy-perf-hooks.mjs diff --git a/lib/_http_client.js b/lib/_http_client.js index 8fdcabaa2a8fd0..d202a28f03502f 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -121,6 +121,8 @@ let debug = require('internal/util/debuglog').debuglog('http', (fn) => { const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/; const kError = Symbol('kError'); const kPath = Symbol('kPath'); +const kAuthority = Symbol('kAuthority'); +const kProxyRewrittenToAbsolute = Symbol('kProxyRewrittenToAbsolute'); const HTTP_CLIENT_TRACE_EVENT_NAME = 'http.client.request'; @@ -318,6 +320,7 @@ function rewriteForProxiedHttp(req, reqOptions, proxyAuthority, userHostHeader, if (requestURL !== undefined) { // Convert the path to absolute-form. The authority is built from options. req.path = requestURL.href; + req[kProxyRewrittenToAbsolute] = true; } debug(`updated request for HTTP proxy ${href} with ${req.path} `, req[kOutHeaders]); return true; @@ -479,6 +482,7 @@ function ClientRequest(input, options, cb) { this.reusedSocket = false; this.host = host; this.protocol = protocol; + this[kProxyRewrittenToAbsolute] = false; if (this.agent) { // If there is an agent we should default to Connection:keep-alive, @@ -509,6 +513,9 @@ function ClientRequest(input, options, cb) { if (port && +port !== defaultPort) { hostHeaderFromOptions += ':' + port; } + // Preserve the request authority (with the port when non-default) so that + // the perf_hooks entry can report a faithful URL. + this[kAuthority] = hostHeaderFromOptions; const headersArray = ArrayIsArray(options.headers); if (!headersArray) { if (options.headers) { @@ -627,7 +634,11 @@ ClientRequest.prototype._finish = function _finish() { detail: { req: { method: this.method, - url: `${this.protocol}//${this.host}${this.path}`, + // If the path has been rewritten to absolute-form for proxying, + // it is already a full URL. + url: this[kProxyRewrittenToAbsolute] ? + this.path : + `${this.protocol}//${this[kAuthority]}${this.path}`, headers: typeof this.getHeaders === 'function' ? this.getHeaders() : {}, }, }, diff --git a/test/client-proxy/test-http-proxy-perf-hooks.mjs b/test/client-proxy/test-http-proxy-perf-hooks.mjs new file mode 100644 index 00000000000000..4469bbf24831ce --- /dev/null +++ b/test/client-proxy/test-http-proxy-perf-hooks.mjs @@ -0,0 +1,63 @@ +// This tests that when a request path is rewritten to absolute-form for +// proxying, the perf_hooks HTTP entries report it as the URL as-is, instead +// of appending it to the protocol and authority again. +// Refs: https://github.com/nodejs/node/issues/59625 +import * as common from '../common/index.mjs'; +import assert from 'node:assert'; +import { once } from 'events'; +import http from 'node:http'; +import { PerformanceObserver } from 'node:perf_hooks'; +import { createProxyServer } from '../common/proxy-server.js'; + +const entries = []; +const obs = new PerformanceObserver(common.mustCallAtLeast((items) => { + entries.push(...items.getEntries()); +})); +obs.observe({ type: 'http' }); + +// Start a server to process the final request. +const server = http.createServer(common.mustCall((req, res) => { + res.end('Hello world'); +})); +server.on('error', common.mustNotCall((err) => { console.error('Server error', err); })); +server.listen(0); +await once(server, 'listening'); + +// Start a minimal proxy server. +const { proxy, logs } = createProxyServer(); +proxy.listen(0); +await once(proxy, 'listening'); + +const requestUrl = `http://localhost:${server.address().port}/test`; +const agent = new http.Agent({ + proxyEnv: { + HTTP_PROXY: `http://localhost:${proxy.address().port}`, + }, +}); + +const res = await new Promise((resolve, reject) => { + http.request(requestUrl, { agent }, resolve).on('error', reject).end(); +}); +res.resume(); +await once(res, 'end'); + +// Verify that the request went through the proxy. +assert.strictEqual(logs.length, 1); +assert.strictEqual(logs[0].url, requestUrl); + +proxy.close(); +server.close(); + +process.on('exit', () => { + // Two HttpClient entries are expected: one for the proxied request, one for + // the request the proxy makes to forward it. Both should report the full + // URL, including the port. + const clientUrls = entries.filter((entry) => entry.name === 'HttpClient') + .map((entry) => entry.detail.req.url); + assert.deepStrictEqual(clientUrls, [requestUrl, requestUrl]); + // Two HttpRequest entries are expected: the proxy server receives the + // request target in absolute-form, the final server in origin-form. + const requestUrls = entries.filter((entry) => entry.name === 'HttpRequest') + .map((entry) => entry.detail.req.url).sort(); + assert.deepStrictEqual(requestUrls, ['/test', requestUrl].sort()); +}); diff --git a/test/parallel/test-http-perf_hooks.js b/test/parallel/test-http-perf_hooks.js index 9acf54c5617bd8..674bb591f0cbf2 100644 --- a/test/parallel/test-http-perf_hooks.js +++ b/test/parallel/test-http-perf_hooks.js @@ -36,16 +36,18 @@ const server = http.Server(common.mustCall((req, res) => { })); }, 2)); +let port; server.listen(0, common.mustCall(async () => { + port = server.address().port; await Promise.all([ makeRequest({ - port: server.address().port, + port, path: '/', method: 'POST', data: expected }), makeRequest({ - port: server.address().port, + port, path: '/', method: 'POST', data: expected @@ -63,14 +65,17 @@ process.on('exit', () => { assert.strictEqual(typeof entry.duration, 'number'); if (entry.name === 'HttpClient') { numberOfHttpClients++; + // The reported URL must include the port when it is non-default. + // Refs: https://github.com/nodejs/node/issues/59625 + assert.strictEqual(entry.detail.req.url, `http://localhost:${port}/`); } else if (entry.name === 'HttpRequest') { numberOfHttpRequests++; + assert.strictEqual(entry.detail.req.url, '/'); } - assert.strictEqual(typeof entry.detail.req.method, 'string'); - assert.strictEqual(typeof entry.detail.req.url, 'string'); + assert.strictEqual(entry.detail.req.method, 'POST'); assert.strictEqual(typeof entry.detail.req.headers, 'object'); - assert.strictEqual(typeof entry.detail.res.statusCode, 'number'); - assert.strictEqual(typeof entry.detail.res.statusMessage, 'string'); + assert.strictEqual(entry.detail.res.statusCode, 200); + assert.strictEqual(entry.detail.res.statusMessage, 'OK'); assert.strictEqual(typeof entry.detail.res.headers, 'object'); } assert.strictEqual(numberOfHttpClients, 2); From 47b6ef28e658cf7829f6ca0948fdfa627aee7d7b Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sun, 12 Jul 2026 17:11:28 +0200 Subject: [PATCH 5/6] net: make TCP Server and Socket transferable across worker threads Allow a listening net.Server or an accepted net.Socket to be moved to another thread by listing it in the transferList of a worker_threads postMessage() call. Unix only; Windows throws. Signed-off-by: Matteo Collina PR-URL: https://github.com/nodejs/node/pull/64225 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Paolo Insogna --- doc/api/errors.md | 16 +++ doc/api/net.md | 42 ++++++ doc/api/worker_threads.md | 21 ++- lib/internal/errors.js | 6 + lib/net.js | 122 ++++++++++++++++++ src/tcp_wrap.cc | 82 ++++++++++++ src/tcp_wrap.h | 28 ++++ .../test-net-server-transfer-worker.js | 60 +++++++++ .../test-net-socket-transfer-worker-http.js | 80 ++++++++++++ .../test-net-socket-transfer-worker.js | 65 ++++++++++ test/parallel/test-net-transfer-guards.js | 52 ++++++++ ...-transfer-fake-js-transferable-internal.js | 9 +- 12 files changed, 575 insertions(+), 8 deletions(-) create mode 100644 test/parallel/test-net-server-transfer-worker.js create mode 100644 test/parallel/test-net-socket-transfer-worker-http.js create mode 100644 test/parallel/test-net-socket-transfer-worker.js create mode 100644 test/parallel/test-net-transfer-guards.js diff --git a/doc/api/errors.md b/doc/api/errors.md index db3eb81eff6b90..29cd9e8a4937d1 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -3582,6 +3582,22 @@ added: v18.1.0 The `Response` that has been passed to `WebAssembly.compileStreaming` or to `WebAssembly.instantiateStreaming` is not a valid WebAssembly response. + + +### `ERR_WORKER_HANDLE_NOT_TRANSFERABLE` + +An attempt was made to transfer a `net.Socket` or `net.Server` to another thread +via a `worker_threads` `postMessage()` call while it was not in a transferable +state, for example because it had already started reading or had buffered data. + + + +### `ERR_WORKER_HANDLE_TRANSFER_UNSUPPORTED` + +An attempt was made to transfer a `net.Socket` or `net.Server` to another thread +on a platform where moving the underlying handle between event loops is not +supported (currently Windows). + ### `ERR_WORKER_INIT_FAILED` diff --git a/doc/api/net.md b/doc/api/net.md index e2f06776409898..81863d9b0a13c5 100644 --- a/doc/api/net.md +++ b/doc/api/net.md @@ -305,6 +305,11 @@ added: v0.1.90 This class is used to create a TCP or [IPC][] server. +A listening TCP `net.Server` can be transferred to a worker thread by listing it +in the `transferList` of a [`worker_threads`][] `postMessage()` call. This moves +the underlying listening socket to the receiving thread, where it resumes +accepting connections. See [Transferring TCP handles to other threads][]. + ### `new net.Server([options][, connectionListener])` * `options` {Object} See @@ -751,6 +756,41 @@ is received. For example, it is passed to the listeners of a [`'connection'`][] event emitted on a [`net.Server`][], so the user can use it to interact with the client. +### Transferring TCP handles to other threads + +A connected TCP `net.Socket` can be moved to another thread by listing it in the +`transferList` of a [`worker_threads`][] `postMessage()` call. After the +transfer, the source socket is destroyed on the sending thread (further use +fails with `ERR_STREAM_DESTROYED` rather than silently dropping data), and the +socket continues to work on the receiving thread. This makes it possible to +accept connections on one thread and distribute them across a pool of worker +threads, for example to build a `node:cluster`-like model on top of worker +threads. + +The socket must be a freshly accepted or created TCP connection: it must still +be attached to a live handle, must not be connecting or destroyed, and must not +have started reading or have buffered data. Otherwise `postMessage()` throws +`ERR_WORKER_HANDLE_NOT_TRANSFERABLE`. Only TCP sockets are supported, and only +on Unix-like platforms; on Windows `postMessage()` throws +`ERR_WORKER_HANDLE_TRANSFER_UNSUPPORTED`. + +```cjs +const net = require('node:net'); +const { Worker } = require('node:worker_threads'); + +// worker.js receives `{ socket }` messages and handles each connection. +const worker = new Worker('./worker.js'); + +const server = net.createServer((socket) => { + // Hand the freshly accepted connection off to the worker thread. + worker.postMessage({ socket }, [socket]); +}); +server.listen(8000); +``` + +A listening [`net.Server`][] can be transferred the same way, which moves the +listening socket itself (and its pending accept queue) to the receiving thread. + ### `new net.Socket([options])`