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(() => {