From 5f09defc75d8e1527de25ceb11f3c93c9f9687d5 Mon Sep 17 00:00:00 2001 From: Archkon <180910180+Archkon@users.noreply.github.com> Date: Thu, 9 Jul 2026 22:22:32 +0800 Subject: [PATCH] http: avoid aborting IncomingMessage signal on normal close IncomingMessage 'close' is emitted when request handling completes, not only when the underlying connection is closed. Using that event to abort IncomingMessage.signal makes the signal abort after a request body is read normally. Track the underlying socket close instead, and detach the listener when the request or response completes normally. This keeps the signal from being aborted by normal stream completion or later keep-alive socket closure, while still aborting it when the connection closes during the active request lifecycle. Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com> --- lib/_http_client.js | 2 + lib/_http_incoming.js | 55 +++++++++++++-- lib/_http_server.js | 1 + test/parallel/test-http-request-signal.js | 86 +++++++++++++++++++++-- 4 files changed, 135 insertions(+), 9 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 891da4e3f9a984..dd6cc46895d8f1 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -999,6 +999,8 @@ function responseOnEnd() { const req = this.req; const socket = req.socket; + this._detachAbortSignal(); + if (socket) { if (req.timeoutCb) socket.removeListener('timeout', emitRequestTimeout); socket.removeListener('timeout', responseOnTimeout); diff --git a/lib/_http_incoming.js b/lib/_http_incoming.js index 6c082454b08a2b..dabe891d9dd326 100644 --- a/lib/_http_incoming.js +++ b/lib/_http_incoming.js @@ -38,6 +38,9 @@ const kTrailers = Symbol('kTrailers'); const kTrailersDistinct = Symbol('kTrailersDistinct'); const kTrailersCount = Symbol('kTrailersCount'); const kAbortController = Symbol('kAbortController'); +const kAbortSignalSocket = Symbol('kAbortSignalSocket'); +const kAbortSignalListener = Symbol('kAbortSignalListener'); +const kAbortSignalDetached = Symbol('kAbortSignalDetached'); function readStart(socket) { if (socket && !socket._paused && socket.readable) @@ -94,6 +97,9 @@ function IncomingMessage(socket) { // read by the user, so there's no point continuing to handle it. this._dumped = false; this[kAbortController] = null; + this[kAbortSignalSocket] = null; + this[kAbortSignalListener] = null; + this[kAbortSignalDetached] = false; } ObjectSetPrototypeOf(IncomingMessage.prototype, Readable.prototype); ObjectSetPrototypeOf(IncomingMessage, Readable); @@ -195,18 +201,51 @@ ObjectDefineProperty(IncomingMessage.prototype, 'signal', { if (this[kAbortController] === null) { const ac = new AbortController(); this[kAbortController] = ac; - if (this.destroyed) { + if (this.destroyed && (!this.readableEnded || !this.complete)) { ac.abort(); } else { - this.once('close', function() { - ac.abort(); - }); + this._attachAbortSignal(); } } return this[kAbortController].signal; }, }); +IncomingMessage.prototype._attachAbortSignal = function _attachAbortSignal() { + if (this[kAbortController].signal.aborted || + this[kAbortSignalDetached] || + this[kAbortSignalListener] !== null) { + return; + } + + const socket = this.socket; + if (!socket) { + return; + } + + if (socket.destroyed) { + abortSignal(this); + return; + } + + this[kAbortSignalSocket] = socket; + this[kAbortSignalListener] = () => { + abortSignal(this); + }; + socket.once('close', this[kAbortSignalListener]); +}; + +IncomingMessage.prototype._detachAbortSignal = function _detachAbortSignal() { + const socket = this[kAbortSignalSocket]; + const listener = this[kAbortSignalListener]; + this[kAbortSignalDetached] = true; + this[kAbortSignalSocket] = null; + this[kAbortSignalListener] = null; + if (socket !== null && listener !== null) { + socket.removeListener('close', listener); + } +}; + IncomingMessage.prototype.setTimeout = function setTimeout(msecs, callback) { if (callback) this.on('timeout', callback); @@ -242,6 +281,7 @@ IncomingMessage.prototype._destroy = function _destroy(err, cb) { if (!this.readableEnded || !this.complete) { this.aborted = true; this.emit('aborted'); + abortSignal(this); } // If aborted and the underlying socket is not already destroyed, @@ -263,6 +303,13 @@ IncomingMessage.prototype._destroy = function _destroy(err, cb) { } }; +function abortSignal(self) { + self._detachAbortSignal(); + if (self[kAbortController] !== null) { + self[kAbortController].abort(); + } +} + IncomingMessage.prototype._addHeaderLines = _addHeaderLines; function _addHeaderLines(headers, n) { if (headers?.length) { diff --git a/lib/_http_server.js b/lib/_http_server.js index 0f5865126689d3..86b8df98e70ee2 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -1198,6 +1198,7 @@ function resOnFinish(req, res, socket, state, server) { // array will be empty. assert(state.incoming.length === 0 || state.incoming[0] === req); + req._detachAbortSignal(); state.incoming.shift(); // If the user never called req.read(), and didn't pipe() or diff --git a/test/parallel/test-http-request-signal.js b/test/parallel/test-http-request-signal.js index b7c9fdc79e8365..5b9fd17c433e10 100644 --- a/test/parallel/test-http-request-signal.js +++ b/test/parallel/test-http-request-signal.js @@ -4,7 +4,7 @@ const common = require('../common'); const assert = require('assert'); const http = require('http'); -// Test 1: req.signal is an AbortSignal and aborts on 'close' +// Test 1: req.signal is an AbortSignal and aborts on socket close { const server = http.createServer(common.mustCall((req, res) => { assert.ok(req.signal instanceof AbortSignal); @@ -21,21 +21,68 @@ const http = require('http'); })); } -// Test 2: req.signal is aborted if accessed after destroy +// Test 2: req.signal is not aborted when a request body completes normally. +{ + const body = JSON.stringify({ hello: 'world' }); + const server = http.createServer(common.mustCall((req, res) => { + assert.ok(req.signal instanceof AbortSignal); + assert.strictEqual(req.signal.aborted, false); + req.signal.onabort = common.mustNotCall(); + + req.on('close', common.mustCall(() => { + assert.strictEqual(req.aborted, false); + assert.strictEqual(req.complete, true); + assert.strictEqual(req.signal.aborted, false); + })); + + req.on('end', common.mustCall(() => { + setTimeout(common.mustCall(() => { + assert.strictEqual(req.aborted, false); + assert.strictEqual(req.complete, true); + assert.strictEqual(req.signal.aborted, false); + res.end('ok'); + }), 10); + })); + req.resume(); + })); + + server.listen(0, common.mustCall(() => { + const clientReq = http.request( + { + port: server.address().port, + method: 'PATCH', + path: '/tables/1', + headers: { + 'content-type': 'application/json', + 'content-length': Buffer.byteLength(body), + }, + }, + common.mustCall((res) => { + res.resume(); + res.on('end', common.mustCall(() => { + server.close(); + })); + }), + ); + clientReq.end(body); + })); +} + +// Test 3: req.signal is aborted if accessed after destroy { const req = new http.IncomingMessage(null); req.destroy(); assert.strictEqual(req.signal.aborted, true); } -// Test 3: Multiple accesses return the same signal +// Test 4: Multiple accesses return the same signal { const req = new http.IncomingMessage(null); assert.strictEqual(req.signal, req.signal); } -// Test 4: res.signal on a client-side http.request() response (IncomingMessage). +// Test 5: res.signal on a client-side http.request() response (IncomingMessage). { const server = http.createServer(common.mustCall((req, res) => { res.writeHead(200); @@ -61,7 +108,36 @@ const http = require('http'); })); } -// Test 5: Client cancels a pending request. +// Test 6: res.signal is not aborted when a response body completes normally. +{ + const server = http.createServer(common.mustCall((req, res) => { + res.end('ok'); + })); + + server.listen(0, common.mustCall(() => { + const clientReq = http.request( + { port: server.address().port }, + common.mustCall((res) => { + assert.ok(res.signal instanceof AbortSignal); + assert.strictEqual(res.signal.aborted, false); + res.signal.onabort = common.mustNotCall(); + + res.resume(); + res.on('end', common.mustCall(() => { + assert.strictEqual(res.complete, true); + assert.strictEqual(res.signal.aborted, false); + })); + res.on('close', common.mustCall(() => { + assert.strictEqual(res.signal.aborted, false); + server.close(); + })); + }), + ); + clientReq.end(); + })); +} + +// Test 7: Client cancels a pending request. { const server = http.createServer(common.mustCall((req, res) => { req.signal.onabort = common.mustCall(() => {