diff --git a/doc/api/http.md b/doc/api/http.md index 3cdcab71361260..92dcee74a6f08a 100644 --- a/doc/api/http.md +++ b/doc/api/http.md @@ -3045,13 +3045,20 @@ Calls `message.socket.setTimeout(msecs, callback)`. added: - v26.1.0 - v24.16.0 +changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/64392 + description: The signal is no longer aborted after the message + completes normally. --> * Type: {AbortSignal} -An {AbortSignal} that is aborted when the underlying socket closes or the -request is destroyed. The signal is created lazily on first access — no -{AbortController} is allocated for requests that never use this property. +An {AbortSignal} that is aborted when the message is destroyed before +completion or when its underlying socket closes before request handling or +response reading completes. +The signal is created lazily on first access — no {AbortController} is allocated +for requests that never use this property. This is useful for cancelling downstream asynchronous work such as database queries or `fetch` calls when a client disconnects mid-request. diff --git a/lib/_http_client.js b/lib/_http_client.js index 891da4e3f9a984..d97336bf92eb25 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -50,6 +50,7 @@ const { prepareError, kSkipPendingData, } = require('_http_common'); +const { kDetachAbortSignal } = require('_http_incoming'); const { kUniqueHeaders, parseUniqueHeadersOption, @@ -999,6 +1000,8 @@ function responseOnEnd() { const req = this.req; const socket = req.socket; + this[kDetachAbortSignal](); + 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..f2823307005c71 100644 --- a/lib/_http_incoming.js +++ b/lib/_http_incoming.js @@ -38,6 +38,11 @@ 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'); +const kAttachAbortSignal = Symbol('kAttachAbortSignal'); +const kDetachAbortSignal = Symbol('kDetachAbortSignal'); function readStart(socket) { if (socket && !socket._paused && socket.readable) @@ -94,6 +99,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 +203,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[kAttachAbortSignal](); } } return this[kAbortController].signal; }, }); +IncomingMessage.prototype[kAttachAbortSignal] = function() { + 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[kDetachAbortSignal] = function() { + 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 +283,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 +305,13 @@ IncomingMessage.prototype._destroy = function _destroy(err, cb) { } }; +function abortSignal(self) { + self[kDetachAbortSignal](); + if (self[kAbortController] !== null) { + self[kAbortController].abort(); + } +} + IncomingMessage.prototype._addHeaderLines = _addHeaderLines; function _addHeaderLines(headers, n) { if (headers?.length) { @@ -480,6 +529,7 @@ function onError(self, error, cb) { module.exports = { IncomingMessage, + kDetachAbortSignal, readStart, readStop, }; diff --git a/lib/_http_server.js b/lib/_http_server.js index 0f5865126689d3..e4e57092ee7a73 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -70,7 +70,10 @@ const { defaultTriggerAsyncIdScope, getOrSetAsyncId, } = require('internal/async_hooks'); -const { IncomingMessage } = require('_http_incoming'); +const { + IncomingMessage, + kDetachAbortSignal, +} = require('_http_incoming'); const { ConnResetException, codes: { @@ -1198,6 +1201,7 @@ function resOnFinish(req, res, socket, state, server) { // array will be empty. assert(state.incoming.length === 0 || state.incoming[0] === req); + req[kDetachAbortSignal](); 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(() => {