http: avoid aborting IncomingMessage signal on normal close · nodejs/node@ee54d9d · GitHub
Skip to content

Commit ee54d9d

Browse files
Archkonaduh95
authored andcommitted
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> PR-URL: #64392 Fixes: #64390 Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
1 parent b54aa83 commit ee54d9d

5 files changed

Lines changed: 153 additions & 13 deletions

File tree

doc/api/http.md

Lines changed: 10 additions & 3 deletions

lib/_http_client.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ const {
5050
prepareError,
5151
kSkipPendingData,
5252
} = require('_http_common');
53+
const { kDetachAbortSignal } = require('_http_incoming');
5354
const {
5455
kHighWaterMark,
5556
kUniqueHeaders,
@@ -1017,6 +1018,8 @@ function responseOnEnd() {
10171018
const req = this.req;
10181019
const socket = req.socket;
10191020

1021+
this[kDetachAbortSignal]();
1022+
10201023
if (socket) {
10211024
if (req.timeoutCb) socket.removeListener('timeout', emitRequestTimeout);
10221025
socket.removeListener('timeout', responseOnTimeout);

lib/_http_incoming.js

Lines changed: 54 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,11 @@ const kTrailers = Symbol('kTrailers');
3838
const kTrailersDistinct = Symbol('kTrailersDistinct');
3939
const kTrailersCount = Symbol('kTrailersCount');
4040
const kAbortController = Symbol('kAbortController');
41+
const kAbortSignalSocket = Symbol('kAbortSignalSocket');
42+
const kAbortSignalListener = Symbol('kAbortSignalListener');
43+
const kAbortSignalDetached = Symbol('kAbortSignalDetached');
44+
const kAttachAbortSignal = Symbol('kAttachAbortSignal');
45+
const kDetachAbortSignal = Symbol('kDetachAbortSignal');
4146

4247
function readStart(socket) {
4348
if (socket && !socket._paused && socket.readable)
@@ -94,6 +99,9 @@ function IncomingMessage(socket) {
9499
// read by the user, so there's no point continuing to handle it.
95100
this._dumped = false;
96101
this[kAbortController] = null;
102+
this[kAbortSignalSocket] = null;
103+
this[kAbortSignalListener] = null;
104+
this[kAbortSignalDetached] = false;
97105
}
98106
ObjectSetPrototypeOf(IncomingMessage.prototype, Readable.prototype);
99107
ObjectSetPrototypeOf(IncomingMessage, Readable);
@@ -195,18 +203,51 @@ ObjectDefineProperty(IncomingMessage.prototype, 'signal', {
195203
if (this[kAbortController] === null) {
196204
const ac = new AbortController();
197205
this[kAbortController] = ac;
198-
if (this.destroyed) {
206+
if (this.destroyed && (!this.readableEnded || !this.complete)) {
199207
ac.abort();
200208
} else {
201-
this.once('close', function() {
202-
ac.abort();
203-
});
209+
this[kAttachAbortSignal]();
204210
}
205211
}
206212
return this[kAbortController].signal;
207213
},
208214
});
209215

216+
IncomingMessage.prototype[kAttachAbortSignal] = function() {
217+
if (this[kAbortController].signal.aborted ||
218+
this[kAbortSignalDetached] ||
219+
this[kAbortSignalListener] !== null) {
220+
return;
221+
}
222+
223+
const socket = this.socket;
224+
if (!socket) {
225+
return;
226+
}
227+
228+
if (socket.destroyed) {
229+
abortSignal(this);
230+
return;
231+
}
232+
233+
this[kAbortSignalSocket] = socket;
234+
this[kAbortSignalListener] = () => {
235+
abortSignal(this);
236+
};
237+
socket.once('close', this[kAbortSignalListener]);
238+
};
239+
240+
IncomingMessage.prototype[kDetachAbortSignal] = function() {
241+
const socket = this[kAbortSignalSocket];
242+
const listener = this[kAbortSignalListener];
243+
this[kAbortSignalDetached] = true;
244+
this[kAbortSignalSocket] = null;
245+
this[kAbortSignalListener] = null;
246+
if (socket !== null && listener !== null) {
247+
socket.removeListener('close', listener);
248+
}
249+
};
250+
210251
IncomingMessage.prototype.setTimeout = function setTimeout(msecs, callback) {
211252
if (callback)
212253
this.on('timeout', callback);
@@ -234,6 +275,7 @@ IncomingMessage.prototype._destroy = function _destroy(err, cb) {
234275
if (!this.readableEnded || !this.complete) {
235276
this.aborted = true;
236277
this.emit('aborted');
278+
abortSignal(this);
237279
}
238280

239281
// If aborted and the underlying socket is not already destroyed,
@@ -255,6 +297,13 @@ IncomingMessage.prototype._destroy = function _destroy(err, cb) {
255297
}
256298
};
257299

300+
function abortSignal(self) {
301+
self[kDetachAbortSignal]();
302+
if (self[kAbortController] !== null) {
303+
self[kAbortController].abort();
304+
}
305+
}
306+
258307
IncomingMessage.prototype._addHeaderLines = _addHeaderLines;
259308
function _addHeaderLines(headers, n) {
260309
if (headers?.length) {
@@ -472,6 +521,7 @@ function onError(self, error, cb) {
472521

473522
module.exports = {
474523
IncomingMessage,
524+
kDetachAbortSignal,
475525
readStart,
476526
readStop,
477527
};

lib/_http_server.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,10 @@ const {
6868
defaultTriggerAsyncIdScope,
6969
getOrSetAsyncId,
7070
} = require('internal/async_hooks');
71-
const { IncomingMessage } = require('_http_incoming');
71+
const {
72+
IncomingMessage,
73+
kDetachAbortSignal,
74+
} = require('_http_incoming');
7275
const {
7376
ConnResetException,
7477
codes: {
@@ -1105,6 +1108,7 @@ function resOnFinish(req, res, socket, state, server) {
11051108
// array will be empty.
11061109
assert(state.incoming.length === 0 || state.incoming[0] === req);
11071110

1111+
req[kDetachAbortSignal]();
11081112
state.incoming.shift();
11091113

11101114
// If the user never called req.read(), and didn't pipe() or

test/parallel/test-http-request-signal.js

Lines changed: 81 additions & 5 deletions

0 commit comments

Comments
 (0)