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: #64392Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -3010,13 +3010,20 @@ Calls `message.socket.setTimeout(msecs, callback)`.
30103010

30113011
<!-- YAML
30123012
added: v24.16.0
3013+
changes:
3014+
- version: REPLACEME
3015+
pr-url: https://github.com/nodejs/node/pull/64392
3016+
description: The signal is no longer aborted after the message
3017+
completes normally.
30133018
-->
30143019

30153020
* Type: {AbortSignal}
30163021

3017-
An {AbortSignal} that is aborted when the underlying socket closes or the
3018-
request is destroyed. The signal is created lazily on first access β€” no
3019-
{AbortController} is allocated for requests that never use this property.
3022+
An {AbortSignal} that is aborted when the message is destroyed before
3023+
completion or when its underlying socket closes before request handling or
3024+
response reading completes.
3025+
The signal is created lazily on first access β€” no {AbortController} is allocated
3026+
for requests that never use this property.
30203027

30213028
This is useful for cancelling downstream asynchronous work such as database
30223029
queries or `fetch` calls when a client disconnects mid-request.

β€Ž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
constreq=this.req;
10181019
constsocket=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
constkTrailersDistinct=Symbol('kTrailersDistinct');
3939
constkTrailersCount=Symbol('kTrailersCount');
4040
constkAbortController=Symbol('kAbortController');
41+
constkAbortSignalSocket=Symbol('kAbortSignalSocket');
42+
constkAbortSignalListener=Symbol('kAbortSignalListener');
43+
constkAbortSignalDetached=Symbol('kAbortSignalDetached');
44+
constkAttachAbortSignal=Symbol('kAttachAbortSignal');
45+
constkDetachAbortSignal=Symbol('kDetachAbortSignal');
4146

4247
functionreadStart(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
constac=newAbortController();
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
returnthis[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+
constsocket=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+
constsocket=this[kAbortSignalSocket];
242+
constlistener=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=functionsetTimeout(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+
functionabortSignal(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
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const common = require('../common');
44
constassert=require('assert');
55
consthttp=require('http');
66

7-
// Test 1: req.signal is an AbortSignal and aborts on 'close'
7+
// Test 1: req.signal is an AbortSignal and aborts on socket close
88
{
99
constserver=http.createServer(common.mustCall((req,res)=>{
1010
assert.ok(req.signalinstanceofAbortSignal);
@@ -21,21 +21,68 @@ const http = require('http');
2121
}));
2222
}
2323

24-
// Test 2: req.signal is aborted if accessed after destroy
24+
// Test 2: req.signal is not aborted when a request body completes normally.
25+
{
26+
constbody=JSON.stringify({hello: 'world'});
27+
constserver=http.createServer(common.mustCall((req,res)=>{
28+
assert.ok(req.signalinstanceofAbortSignal);
29+
assert.strictEqual(req.signal.aborted,false);
30+
req.signal.onabort=common.mustNotCall();
31+
32+
req.on('close',common.mustCall(()=>{
33+
assert.strictEqual(req.aborted,false);
34+
assert.strictEqual(req.complete,true);
35+
assert.strictEqual(req.signal.aborted,false);
36+
}));
37+
38+
req.on('end',common.mustCall(()=>{
39+
setTimeout(common.mustCall(()=>{
40+
assert.strictEqual(req.aborted,false);
41+
assert.strictEqual(req.complete,true);
42+
assert.strictEqual(req.signal.aborted,false);
43+
res.end('ok');
44+
}),10);
45+
}));
46+
req.resume();
47+
}));
48+
49+
server.listen(0,common.mustCall(()=>{
50+
constclientReq=http.request(
51+
{
52+
port: server.address().port,
53+
method: 'PATCH',
54+
path: '/tables/1',
55+
headers: {
56+
'content-type': 'application/json',
57+
'content-length': Buffer.byteLength(body),
58+
},
59+
},
60+
common.mustCall((res)=>{
61+
res.resume();
62+
res.on('end',common.mustCall(()=>{
63+
server.close();
64+
}));
65+
}),
66+
);
67+
clientReq.end(body);
68+
}));
69+
}
70+
71+
// Test 3: req.signal is aborted if accessed after destroy
2572
{
2673
constreq=newhttp.IncomingMessage(null);
2774
req.destroy();
2875
assert.strictEqual(req.signal.aborted,true);
2976
}
3077

31-
// Test 3: Multiple accesses return the same signal
78+
// Test 4: Multiple accesses return the same signal
3279
{
3380
constreq=newhttp.IncomingMessage(null);
3481
assert.strictEqual(req.signal,req.signal);
3582
}
3683

3784

38-
// Test 4: res.signal on a client-side http.request() response (IncomingMessage).
85+
// Test 5: res.signal on a client-side http.request() response (IncomingMessage).
3986
{
4087
constserver=http.createServer(common.mustCall((req,res)=>{
4188
res.writeHead(200);
@@ -61,7 +108,36 @@ const http = require('http');
61108
}));
62109
}
63110

64-
// Test 5: Client cancels a pending request.
111+
// Test 6: res.signal is not aborted when a response body completes normally.
112+
{
113+
constserver=http.createServer(common.mustCall((req,res)=>{
114+
res.end('ok');
115+
}));
116+
117+
server.listen(0,common.mustCall(()=>{
118+
constclientReq=http.request(
119+
{port: server.address().port},
120+
common.mustCall((res)=>{
121+
assert.ok(res.signalinstanceofAbortSignal);
122+
assert.strictEqual(res.signal.aborted,false);
123+
res.signal.onabort=common.mustNotCall();
124+
125+
res.resume();
126+
res.on('end',common.mustCall(()=>{
127+
assert.strictEqual(res.complete,true);
128+
assert.strictEqual(res.signal.aborted,false);
129+
}));
130+
res.on('close',common.mustCall(()=>{
131+
assert.strictEqual(res.signal.aborted,false);
132+
server.close();
133+
}));
134+
}),
135+
);
136+
clientReq.end();
137+
}));
138+
}
139+
140+
// Test 7: Client cancels a pending request.
65141
{
66142
constserver=http.createServer(common.mustCall((req,res)=>{
67143
req.signal.onabort=common.mustCall(()=>{

0 commit comments

Comments
Β (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
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: #64392Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -3010,13 +3010,20 @@ Calls `message.socket.setTimeout(msecs, callback)`.
30103010

30113011
<!-- YAML
30123012
added: v24.16.0
3013+
changes:
3014+
- version: REPLACEME
3015+
pr-url: https://github.com/nodejs/node/pull/64392
3016+
description: The signal is no longer aborted after the message
3017+
completes normally.
30133018
-->
30143019

30153020
* Type: {AbortSignal}
30163021

3017-
An {AbortSignal} that is aborted when the underlying socket closes or the
3018-
request is destroyed. The signal is created lazily on first access β€” no
3019-
{AbortController} is allocated for requests that never use this property.
3022+
An {AbortSignal} that is aborted when the message is destroyed before
3023+
completion or when its underlying socket closes before request handling or
3024+
response reading completes.
3025+
The signal is created lazily on first access β€” no {AbortController} is allocated
3026+
for requests that never use this property.
30203027

30213028
This is useful for cancelling downstream asynchronous work such as database
30223029
queries or `fetch` calls when a client disconnects mid-request.

β€Ž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
constreq=this.req;
10181019
constsocket=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
constkTrailersDistinct=Symbol('kTrailersDistinct');
3939
constkTrailersCount=Symbol('kTrailersCount');
4040
constkAbortController=Symbol('kAbortController');
41+
constkAbortSignalSocket=Symbol('kAbortSignalSocket');
42+
constkAbortSignalListener=Symbol('kAbortSignalListener');
43+
constkAbortSignalDetached=Symbol('kAbortSignalDetached');
44+
constkAttachAbortSignal=Symbol('kAttachAbortSignal');
45+
constkDetachAbortSignal=Symbol('kDetachAbortSignal');
4146

4247
functionreadStart(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
constac=newAbortController();
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
returnthis[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+
constsocket=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+
constsocket=this[kAbortSignalSocket];
242+
constlistener=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=functionsetTimeout(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+
functionabortSignal(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
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const common = require('../common');
44
constassert=require('assert');
55
consthttp=require('http');
66

7-
// Test 1: req.signal is an AbortSignal and aborts on 'close'
7+
// Test 1: req.signal is an AbortSignal and aborts on socket close
88
{
99
constserver=http.createServer(common.mustCall((req,res)=>{
1010
assert.ok(req.signalinstanceofAbortSignal);
@@ -21,21 +21,68 @@ const http = require('http');
2121
}));
2222
}
2323

24-
// Test 2: req.signal is aborted if accessed after destroy
24+
// Test 2: req.signal is not aborted when a request body completes normally.
25+
{
26+
constbody=JSON.stringify({hello: 'world'});
27+
constserver=http.createServer(common.mustCall((req,res)=>{
28+
assert.ok(req.signalinstanceofAbortSignal);
29+
assert.strictEqual(req.signal.aborted,false);
30+
req.signal.onabort=common.mustNotCall();
31+
32+
req.on('close',common.mustCall(()=>{
33+
assert.strictEqual(req.aborted,false);
34+
assert.strictEqual(req.complete,true);
35+
assert.strictEqual(req.signal.aborted,false);
36+
}));
37+
38+
req.on('end',common.mustCall(()=>{
39+
setTimeout(common.mustCall(()=>{
40+
assert.strictEqual(req.aborted,false);
41+
assert.strictEqual(req.complete,true);
42+
assert.strictEqual(req.signal.aborted,false);
43+
res.end('ok');
44+
}),10);
45+
}));
46+
req.resume();
47+
}));
48+
49+
server.listen(0,common.mustCall(()=>{
50+
constclientReq=http.request(
51+
{
52+
port: server.address().port,
53+
method: 'PATCH',
54+
path: '/tables/1',
55+
headers: {
56+
'content-type': 'application/json',
57+
'content-length': Buffer.byteLength(body),
58+
},
59+
},
60+
common.mustCall((res)=>{
61+
res.resume();
62+
res.on('end',common.mustCall(()=>{
63+
server.close();
64+
}));
65+
}),
66+
);
67+
clientReq.end(body);
68+
}));
69+
}
70+
71+
// Test 3: req.signal is aborted if accessed after destroy
2572
{
2673
constreq=newhttp.IncomingMessage(null);
2774
req.destroy();
2875
assert.strictEqual(req.signal.aborted,true);
2976
}
3077

31-
// Test 3: Multiple accesses return the same signal
78+
// Test 4: Multiple accesses return the same signal
3279
{
3380
constreq=newhttp.IncomingMessage(null);
3481
assert.strictEqual(req.signal,req.signal);
3582
}
3683

3784

38-
// Test 4: res.signal on a client-side http.request() response (IncomingMessage).
85+
// Test 5: res.signal on a client-side http.request() response (IncomingMessage).
3986
{
4087
constserver=http.createServer(common.mustCall((req,res)=>{
4188
res.writeHead(200);
@@ -61,7 +108,36 @@ const http = require('http');
61108
}));
62109
}
63110

64-
// Test 5: Client cancels a pending request.
111+
// Test 6: res.signal is not aborted when a response body completes normally.
112+
{
113+
constserver=http.createServer(common.mustCall((req,res)=>{
114+
res.end('ok');
115+
}));
116+
117+
server.listen(0,common.mustCall(()=>{
118+
constclientReq=http.request(
119+
{port: server.address().port},
120+
common.mustCall((res)=>{
121+
assert.ok(res.signalinstanceofAbortSignal);
122+
assert.strictEqual(res.signal.aborted,false);
123+
res.signal.onabort=common.mustNotCall();
124+
125+
res.resume();
126+
res.on('end',common.mustCall(()=>{
127+
assert.strictEqual(res.complete,true);
128+
assert.strictEqual(res.signal.aborted,false);
129+
}));
130+
res.on('close',common.mustCall(()=>{
131+
assert.strictEqual(res.signal.aborted,false);
132+
server.close();
133+
}));
134+
}),
135+
);
136+
clientReq.end();
137+
}));
138+
}
139+
140+
// Test 7: Client cancels a pending request.
65141
{
66142
constserver=http.createServer(common.mustCall((req,res)=>{
67143
req.signal.onabort=common.mustCall(()=>{

0 commit comments

Comments
Β (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
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: #64392Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -3010,13 +3010,20 @@ Calls `message.socket.setTimeout(msecs, callback)`.
30103010

30113011
<!-- YAML
30123012
added: v24.16.0
3013+
changes:
3014+
- version: REPLACEME
3015+
pr-url: https://github.com/nodejs/node/pull/64392
3016+
description: The signal is no longer aborted after the message
3017+
completes normally.
30133018
-->
30143019

30153020
* Type: {AbortSignal}
30163021

3017-
An {AbortSignal} that is aborted when the underlying socket closes or the
3018-
request is destroyed. The signal is created lazily on first access β€” no
3019-
{AbortController} is allocated for requests that never use this property.
3022+
An {AbortSignal} that is aborted when the message is destroyed before
3023+
completion or when its underlying socket closes before request handling or
3024+
response reading completes.
3025+
The signal is created lazily on first access β€” no {AbortController} is allocated
3026+
for requests that never use this property.
30203027

30213028
This is useful for cancelling downstream asynchronous work such as database
30223029
queries or `fetch` calls when a client disconnects mid-request.

β€Ž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
constreq=this.req;
10181019
constsocket=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
constkTrailersDistinct=Symbol('kTrailersDistinct');
3939
constkTrailersCount=Symbol('kTrailersCount');
4040
constkAbortController=Symbol('kAbortController');
41+
constkAbortSignalSocket=Symbol('kAbortSignalSocket');
42+
constkAbortSignalListener=Symbol('kAbortSignalListener');
43+
constkAbortSignalDetached=Symbol('kAbortSignalDetached');
44+
constkAttachAbortSignal=Symbol('kAttachAbortSignal');
45+
constkDetachAbortSignal=Symbol('kDetachAbortSignal');
4146

4247
functionreadStart(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
constac=newAbortController();
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
returnthis[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+
constsocket=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+
constsocket=this[kAbortSignalSocket];
242+
constlistener=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=functionsetTimeout(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+
functionabortSignal(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
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const common = require('../common');
44
constassert=require('assert');
55
consthttp=require('http');
66

7-
// Test 1: req.signal is an AbortSignal and aborts on 'close'
7+
// Test 1: req.signal is an AbortSignal and aborts on socket close
88
{
99
constserver=http.createServer(common.mustCall((req,res)=>{
1010
assert.ok(req.signalinstanceofAbortSignal);
@@ -21,21 +21,68 @@ const http = require('http');
2121
}));
2222
}
2323

24-
// Test 2: req.signal is aborted if accessed after destroy
24+
// Test 2: req.signal is not aborted when a request body completes normally.
25+
{
26+
constbody=JSON.stringify({hello: 'world'});
27+
constserver=http.createServer(common.mustCall((req,res)=>{
28+
assert.ok(req.signalinstanceofAbortSignal);
29+
assert.strictEqual(req.signal.aborted,false);
30+
req.signal.onabort=common.mustNotCall();
31+
32+
req.on('close',common.mustCall(()=>{
33+
assert.strictEqual(req.aborted,false);
34+
assert.strictEqual(req.complete,true);
35+
assert.strictEqual(req.signal.aborted,false);
36+
}));
37+
38+
req.on('end',common.mustCall(()=>{
39+
setTimeout(common.mustCall(()=>{
40+
assert.strictEqual(req.aborted,false);
41+
assert.strictEqual(req.complete,true);
42+
assert.strictEqual(req.signal.aborted,false);
43+
res.end('ok');
44+
}),10);
45+
}));
46+
req.resume();
47+
}));
48+
49+
server.listen(0,common.mustCall(()=>{
50+
constclientReq=http.request(
51+
{
52+
port: server.address().port,
53+
method: 'PATCH',
54+
path: '/tables/1',
55+
headers: {
56+
'content-type': 'application/json',
57+
'content-length': Buffer.byteLength(body),
58+
},
59+
},
60+
common.mustCall((res)=>{
61+
res.resume();
62+
res.on('end',common.mustCall(()=>{
63+
server.close();
64+
}));
65+
}),
66+
);
67+
clientReq.end(body);
68+
}));
69+
}
70+
71+
// Test 3: req.signal is aborted if accessed after destroy
2572
{
2673
constreq=newhttp.IncomingMessage(null);
2774
req.destroy();
2875
assert.strictEqual(req.signal.aborted,true);
2976
}
3077

31-
// Test 3: Multiple accesses return the same signal
78+
// Test 4: Multiple accesses return the same signal
3279
{
3380
constreq=newhttp.IncomingMessage(null);
3481
assert.strictEqual(req.signal,req.signal);
3582
}
3683

3784

38-
// Test 4: res.signal on a client-side http.request() response (IncomingMessage).
85+
// Test 5: res.signal on a client-side http.request() response (IncomingMessage).
3986
{
4087
constserver=http.createServer(common.mustCall((req,res)=>{
4188
res.writeHead(200);
@@ -61,7 +108,36 @@ const http = require('http');
61108
}));
62109
}
63110

64-
// Test 5: Client cancels a pending request.
111+
// Test 6: res.signal is not aborted when a response body completes normally.
112+
{
113+
constserver=http.createServer(common.mustCall((req,res)=>{
114+
res.end('ok');
115+
}));
116+
117+
server.listen(0,common.mustCall(()=>{
118+
constclientReq=http.request(
119+
{port: server.address().port},
120+
common.mustCall((res)=>{
121+
assert.ok(res.signalinstanceofAbortSignal);
122+
assert.strictEqual(res.signal.aborted,false);
123+
res.signal.onabort=common.mustNotCall();
124+
125+
res.resume();
126+
res.on('end',common.mustCall(()=>{
127+
assert.strictEqual(res.complete,true);
128+
assert.strictEqual(res.signal.aborted,false);
129+
}));
130+
res.on('close',common.mustCall(()=>{
131+
assert.strictEqual(res.signal.aborted,false);
132+
server.close();
133+
}));
134+
}),
135+
);
136+
clientReq.end();
137+
}));
138+
}
139+
140+
// Test 7: Client cancels a pending request.
65141
{
66142
constserver=http.createServer(common.mustCall((req,res)=>{
67143
req.signal.onabort=common.mustCall(()=>{

0 commit comments

Comments
Β (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
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: #64392Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -3010,13 +3010,20 @@ Calls `message.socket.setTimeout(msecs, callback)`.
30103010

30113011
<!-- YAML
30123012
added: v24.16.0
3013+
changes:
3014+
- version: REPLACEME
3015+
pr-url: https://github.com/nodejs/node/pull/64392
3016+
description: The signal is no longer aborted after the message
3017+
completes normally.
30133018
-->
30143019

30153020
* Type: {AbortSignal}
30163021

3017-
An {AbortSignal} that is aborted when the underlying socket closes or the
3018-
request is destroyed. The signal is created lazily on first access β€” no
3019-
{AbortController} is allocated for requests that never use this property.
3022+
An {AbortSignal} that is aborted when the message is destroyed before
3023+
completion or when its underlying socket closes before request handling or
3024+
response reading completes.
3025+
The signal is created lazily on first access β€” no {AbortController} is allocated
3026+
for requests that never use this property.
30203027

30213028
This is useful for cancelling downstream asynchronous work such as database
30223029
queries or `fetch` calls when a client disconnects mid-request.

β€Ž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
constreq=this.req;
10181019
constsocket=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
constkTrailersDistinct=Symbol('kTrailersDistinct');
3939
constkTrailersCount=Symbol('kTrailersCount');
4040
constkAbortController=Symbol('kAbortController');
41+
constkAbortSignalSocket=Symbol('kAbortSignalSocket');
42+
constkAbortSignalListener=Symbol('kAbortSignalListener');
43+
constkAbortSignalDetached=Symbol('kAbortSignalDetached');
44+
constkAttachAbortSignal=Symbol('kAttachAbortSignal');
45+
constkDetachAbortSignal=Symbol('kDetachAbortSignal');
4146

4247
functionreadStart(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
constac=newAbortController();
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
returnthis[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+
constsocket=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+
constsocket=this[kAbortSignalSocket];
242+
constlistener=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=functionsetTimeout(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+
functionabortSignal(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
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const common = require('../common');
44
constassert=require('assert');
55
consthttp=require('http');
66

7-
// Test 1: req.signal is an AbortSignal and aborts on 'close'
7+
// Test 1: req.signal is an AbortSignal and aborts on socket close
88
{
99
constserver=http.createServer(common.mustCall((req,res)=>{
1010
assert.ok(req.signalinstanceofAbortSignal);
@@ -21,21 +21,68 @@ const http = require('http');
2121
}));
2222
}
2323

24-
// Test 2: req.signal is aborted if accessed after destroy
24+
// Test 2: req.signal is not aborted when a request body completes normally.
25+
{
26+
constbody=JSON.stringify({hello: 'world'});
27+
constserver=http.createServer(common.mustCall((req,res)=>{
28+
assert.ok(req.signalinstanceofAbortSignal);
29+
assert.strictEqual(req.signal.aborted,false);
30+
req.signal.onabort=common.mustNotCall();
31+
32+
req.on('close',common.mustCall(()=>{
33+
assert.strictEqual(req.aborted,false);
34+
assert.strictEqual(req.complete,true);
35+
assert.strictEqual(req.signal.aborted,false);
36+
}));
37+
38+
req.on('end',common.mustCall(()=>{
39+
setTimeout(common.mustCall(()=>{
40+
assert.strictEqual(req.aborted,false);
41+
assert.strictEqual(req.complete,true);
42+
assert.strictEqual(req.signal.aborted,false);
43+
res.end('ok');
44+
}),10);
45+
}));
46+
req.resume();
47+
}));
48+
49+
server.listen(0,common.mustCall(()=>{
50+
constclientReq=http.request(
51+
{
52+
port: server.address().port,
53+
method: 'PATCH',
54+
path: '/tables/1',
55+
headers: {
56+
'content-type': 'application/json',
57+
'content-length': Buffer.byteLength(body),
58+
},
59+
},
60+
common.mustCall((res)=>{
61+
res.resume();
62+
res.on('end',common.mustCall(()=>{
63+
server.close();
64+
}));
65+
}),
66+
);
67+
clientReq.end(body);
68+
}));
69+
}
70+
71+
// Test 3: req.signal is aborted if accessed after destroy
2572
{
2673
constreq=newhttp.IncomingMessage(null);
2774
req.destroy();
2875
assert.strictEqual(req.signal.aborted,true);
2976
}
3077

31-
// Test 3: Multiple accesses return the same signal
78+
// Test 4: Multiple accesses return the same signal
3279
{
3380
constreq=newhttp.IncomingMessage(null);
3481
assert.strictEqual(req.signal,req.signal);
3582
}
3683

3784

38-
// Test 4: res.signal on a client-side http.request() response (IncomingMessage).
85+
// Test 5: res.signal on a client-side http.request() response (IncomingMessage).
3986
{
4087
constserver=http.createServer(common.mustCall((req,res)=>{
4188
res.writeHead(200);
@@ -61,7 +108,36 @@ const http = require('http');
61108
}));
62109
}
63110

64-
// Test 5: Client cancels a pending request.
111+
// Test 6: res.signal is not aborted when a response body completes normally.
112+
{
113+
constserver=http.createServer(common.mustCall((req,res)=>{
114+
res.end('ok');
115+
}));
116+
117+
server.listen(0,common.mustCall(()=>{
118+
constclientReq=http.request(
119+
{port: server.address().port},
120+
common.mustCall((res)=>{
121+
assert.ok(res.signalinstanceofAbortSignal);
122+
assert.strictEqual(res.signal.aborted,false);
123+
res.signal.onabort=common.mustNotCall();
124+
125+
res.resume();
126+
res.on('end',common.mustCall(()=>{
127+
assert.strictEqual(res.complete,true);
128+
assert.strictEqual(res.signal.aborted,false);
129+
}));
130+
res.on('close',common.mustCall(()=>{
131+
assert.strictEqual(res.signal.aborted,false);
132+
server.close();
133+
}));
134+
}),
135+
);
136+
clientReq.end();
137+
}));
138+
}
139+
140+
// Test 7: Client cancels a pending request.
65141
{
66142
constserver=http.createServer(common.mustCall((req,res)=>{
67143
req.signal.onabort=common.mustCall(()=>{

0 commit comments

Comments
Β (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
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: #64392Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -3010,13 +3010,20 @@ Calls `message.socket.setTimeout(msecs, callback)`.
30103010

30113011
<!-- YAML
30123012
added: v24.16.0
3013+
changes:
3014+
- version: REPLACEME
3015+
pr-url: https://github.com/nodejs/node/pull/64392
3016+
description: The signal is no longer aborted after the message
3017+
completes normally.
30133018
-->
30143019

30153020
* Type: {AbortSignal}
30163021

3017-
An {AbortSignal} that is aborted when the underlying socket closes or the
3018-
request is destroyed. The signal is created lazily on first access β€” no
3019-
{AbortController} is allocated for requests that never use this property.
3022+
An {AbortSignal} that is aborted when the message is destroyed before
3023+
completion or when its underlying socket closes before request handling or
3024+
response reading completes.
3025+
The signal is created lazily on first access β€” no {AbortController} is allocated
3026+
for requests that never use this property.
30203027

30213028
This is useful for cancelling downstream asynchronous work such as database
30223029
queries or `fetch` calls when a client disconnects mid-request.

β€Ž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
constreq=this.req;
10181019
constsocket=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
constkTrailersDistinct=Symbol('kTrailersDistinct');
3939
constkTrailersCount=Symbol('kTrailersCount');
4040
constkAbortController=Symbol('kAbortController');
41+
constkAbortSignalSocket=Symbol('kAbortSignalSocket');
42+
constkAbortSignalListener=Symbol('kAbortSignalListener');
43+
constkAbortSignalDetached=Symbol('kAbortSignalDetached');
44+
constkAttachAbortSignal=Symbol('kAttachAbortSignal');
45+
constkDetachAbortSignal=Symbol('kDetachAbortSignal');
4146

4247
functionreadStart(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
constac=newAbortController();
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
returnthis[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+
constsocket=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+
constsocket=this[kAbortSignalSocket];
242+
constlistener=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=functionsetTimeout(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+
functionabortSignal(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
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const common = require('../common');
44
constassert=require('assert');
55
consthttp=require('http');
66

7-
// Test 1: req.signal is an AbortSignal and aborts on 'close'
7+
// Test 1: req.signal is an AbortSignal and aborts on socket close
88
{
99
constserver=http.createServer(common.mustCall((req,res)=>{
1010
assert.ok(req.signalinstanceofAbortSignal);
@@ -21,21 +21,68 @@ const http = require('http');
2121
}));
2222
}
2323

24-
// Test 2: req.signal is aborted if accessed after destroy
24+
// Test 2: req.signal is not aborted when a request body completes normally.
25+
{
26+
constbody=JSON.stringify({hello: 'world'});
27+
constserver=http.createServer(common.mustCall((req,res)=>{
28+
assert.ok(req.signalinstanceofAbortSignal);
29+
assert.strictEqual(req.signal.aborted,false);
30+
req.signal.onabort=common.mustNotCall();
31+
32+
req.on('close',common.mustCall(()=>{
33+
assert.strictEqual(req.aborted,false);
34+
assert.strictEqual(req.complete,true);
35+
assert.strictEqual(req.signal.aborted,false);
36+
}));
37+
38+
req.on('end',common.mustCall(()=>{
39+
setTimeout(common.mustCall(()=>{
40+
assert.strictEqual(req.aborted,false);
41+
assert.strictEqual(req.complete,true);
42+
assert.strictEqual(req.signal.aborted,false);
43+
res.end('ok');
44+
}),10);
45+
}));
46+
req.resume();
47+
}));
48+
49+
server.listen(0,common.mustCall(()=>{
50+
constclientReq=http.request(
51+
{
52+
port: server.address().port,
53+
method: 'PATCH',
54+
path: '/tables/1',
55+
headers: {
56+
'content-type': 'application/json',
57+
'content-length': Buffer.byteLength(body),
58+
},
59+
},
60+
common.mustCall((res)=>{
61+
res.resume();
62+
res.on('end',common.mustCall(()=>{
63+
server.close();
64+
}));
65+
}),
66+
);
67+
clientReq.end(body);
68+
}));
69+
}
70+
71+
// Test 3: req.signal is aborted if accessed after destroy
2572
{
2673
constreq=newhttp.IncomingMessage(null);
2774
req.destroy();
2875
assert.strictEqual(req.signal.aborted,true);
2976
}
3077

31-
// Test 3: Multiple accesses return the same signal
78+
// Test 4: Multiple accesses return the same signal
3279
{
3380
constreq=newhttp.IncomingMessage(null);
3481
assert.strictEqual(req.signal,req.signal);
3582
}
3683

3784

38-
// Test 4: res.signal on a client-side http.request() response (IncomingMessage).
85+
// Test 5: res.signal on a client-side http.request() response (IncomingMessage).
3986
{
4087
constserver=http.createServer(common.mustCall((req,res)=>{
4188
res.writeHead(200);
@@ -61,7 +108,36 @@ const http = require('http');
61108
}));
62109
}
63110

64-
// Test 5: Client cancels a pending request.
111+
// Test 6: res.signal is not aborted when a response body completes normally.
112+
{
113+
constserver=http.createServer(common.mustCall((req,res)=>{
114+
res.end('ok');
115+
}));
116+
117+
server.listen(0,common.mustCall(()=>{
118+
constclientReq=http.request(
119+
{port: server.address().port},
120+
common.mustCall((res)=>{
121+
assert.ok(res.signalinstanceofAbortSignal);
122+
assert.strictEqual(res.signal.aborted,false);
123+
res.signal.onabort=common.mustNotCall();
124+
125+
res.resume();
126+
res.on('end',common.mustCall(()=>{
127+
assert.strictEqual(res.complete,true);
128+
assert.strictEqual(res.signal.aborted,false);
129+
}));
130+
res.on('close',common.mustCall(()=>{
131+
assert.strictEqual(res.signal.aborted,false);
132+
server.close();
133+
}));
134+
}),
135+
);
136+
clientReq.end();
137+
}));
138+
}
139+
140+
// Test 7: Client cancels a pending request.
65141
{
66142
constserver=http.createServer(common.mustCall((req,res)=>{
67143
req.signal.onabort=common.mustCall(()=>{

0 commit comments

Comments
Β (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
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: #64392Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -3010,13 +3010,20 @@ Calls `message.socket.setTimeout(msecs, callback)`.
30103010

30113011
<!-- YAML
30123012
added: v24.16.0
3013+
changes:
3014+
- version: REPLACEME
3015+
pr-url: https://github.com/nodejs/node/pull/64392
3016+
description: The signal is no longer aborted after the message
3017+
completes normally.
30133018
-->
30143019

30153020
* Type: {AbortSignal}
30163021

3017-
An {AbortSignal} that is aborted when the underlying socket closes or the
3018-
request is destroyed. The signal is created lazily on first access β€” no
3019-
{AbortController} is allocated for requests that never use this property.
3022+
An {AbortSignal} that is aborted when the message is destroyed before
3023+
completion or when its underlying socket closes before request handling or
3024+
response reading completes.
3025+
The signal is created lazily on first access β€” no {AbortController} is allocated
3026+
for requests that never use this property.
30203027

30213028
This is useful for cancelling downstream asynchronous work such as database
30223029
queries or `fetch` calls when a client disconnects mid-request.

β€Ž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
constreq=this.req;
10181019
constsocket=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
constkTrailersDistinct=Symbol('kTrailersDistinct');
3939
constkTrailersCount=Symbol('kTrailersCount');
4040
constkAbortController=Symbol('kAbortController');
41+
constkAbortSignalSocket=Symbol('kAbortSignalSocket');
42+
constkAbortSignalListener=Symbol('kAbortSignalListener');
43+
constkAbortSignalDetached=Symbol('kAbortSignalDetached');
44+
constkAttachAbortSignal=Symbol('kAttachAbortSignal');
45+
constkDetachAbortSignal=Symbol('kDetachAbortSignal');
4146

4247
functionreadStart(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
constac=newAbortController();
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
returnthis[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+
constsocket=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+
constsocket=this[kAbortSignalSocket];
242+
constlistener=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=functionsetTimeout(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+
functionabortSignal(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
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const common = require('../common');
44
constassert=require('assert');
55
consthttp=require('http');
66

7-
// Test 1: req.signal is an AbortSignal and aborts on 'close'
7+
// Test 1: req.signal is an AbortSignal and aborts on socket close
88
{
99
constserver=http.createServer(common.mustCall((req,res)=>{
1010
assert.ok(req.signalinstanceofAbortSignal);
@@ -21,21 +21,68 @@ const http = require('http');
2121
}));
2222
}
2323

24-
// Test 2: req.signal is aborted if accessed after destroy
24+
// Test 2: req.signal is not aborted when a request body completes normally.
25+
{
26+
constbody=JSON.stringify({hello: 'world'});
27+
constserver=http.createServer(common.mustCall((req,res)=>{
28+
assert.ok(req.signalinstanceofAbortSignal);
29+
assert.strictEqual(req.signal.aborted,false);
30+
req.signal.onabort=common.mustNotCall();
31+
32+
req.on('close',common.mustCall(()=>{
33+
assert.strictEqual(req.aborted,false);
34+
assert.strictEqual(req.complete,true);
35+
assert.strictEqual(req.signal.aborted,false);
36+
}));
37+
38+
req.on('end',common.mustCall(()=>{
39+
setTimeout(common.mustCall(()=>{
40+
assert.strictEqual(req.aborted,false);
41+
assert.strictEqual(req.complete,true);
42+
assert.strictEqual(req.signal.aborted,false);
43+
res.end('ok');
44+
}),10);
45+
}));
46+
req.resume();
47+
}));
48+
49+
server.listen(0,common.mustCall(()=>{
50+
constclientReq=http.request(
51+
{
52+
port: server.address().port,
53+
method: 'PATCH',
54+
path: '/tables/1',
55+
headers: {
56+
'content-type': 'application/json',
57+
'content-length': Buffer.byteLength(body),
58+
},
59+
},
60+
common.mustCall((res)=>{
61+
res.resume();
62+
res.on('end',common.mustCall(()=>{
63+
server.close();
64+
}));
65+
}),
66+
);
67+
clientReq.end(body);
68+
}));
69+
}
70+
71+
// Test 3: req.signal is aborted if accessed after destroy
2572
{
2673
constreq=newhttp.IncomingMessage(null);
2774
req.destroy();
2875
assert.strictEqual(req.signal.aborted,true);
2976
}
3077

31-
// Test 3: Multiple accesses return the same signal
78+
// Test 4: Multiple accesses return the same signal
3279
{
3380
constreq=newhttp.IncomingMessage(null);
3481
assert.strictEqual(req.signal,req.signal);
3582
}
3683

3784

38-
// Test 4: res.signal on a client-side http.request() response (IncomingMessage).
85+
// Test 5: res.signal on a client-side http.request() response (IncomingMessage).
3986
{
4087
constserver=http.createServer(common.mustCall((req,res)=>{
4188
res.writeHead(200);
@@ -61,7 +108,36 @@ const http = require('http');
61108
}));
62109
}
63110

64-
// Test 5: Client cancels a pending request.
111+
// Test 6: res.signal is not aborted when a response body completes normally.
112+
{
113+
constserver=http.createServer(common.mustCall((req,res)=>{
114+
res.end('ok');
115+
}));
116+
117+
server.listen(0,common.mustCall(()=>{
118+
constclientReq=http.request(
119+
{port: server.address().port},
120+
common.mustCall((res)=>{
121+
assert.ok(res.signalinstanceofAbortSignal);
122+
assert.strictEqual(res.signal.aborted,false);
123+
res.signal.onabort=common.mustNotCall();
124+
125+
res.resume();
126+
res.on('end',common.mustCall(()=>{
127+
assert.strictEqual(res.complete,true);
128+
assert.strictEqual(res.signal.aborted,false);
129+
}));
130+
res.on('close',common.mustCall(()=>{
131+
assert.strictEqual(res.signal.aborted,false);
132+
server.close();
133+
}));
134+
}),
135+
);
136+
clientReq.end();
137+
}));
138+
}
139+
140+
// Test 7: Client cancels a pending request.
65141
{
66142
constserver=http.createServer(common.mustCall((req,res)=>{
67143
req.signal.onabort=common.mustCall(()=>{

0 commit comments

Comments
Β (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
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: #64392Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -3010,13 +3010,20 @@ Calls `message.socket.setTimeout(msecs, callback)`.
30103010

30113011
<!-- YAML
30123012
added: v24.16.0
3013+
changes:
3014+
- version: REPLACEME
3015+
pr-url: https://github.com/nodejs/node/pull/64392
3016+
description: The signal is no longer aborted after the message
3017+
completes normally.
30133018
-->
30143019

30153020
* Type: {AbortSignal}
30163021

3017-
An {AbortSignal} that is aborted when the underlying socket closes or the
3018-
request is destroyed. The signal is created lazily on first access β€” no
3019-
{AbortController} is allocated for requests that never use this property.
3022+
An {AbortSignal} that is aborted when the message is destroyed before
3023+
completion or when its underlying socket closes before request handling or
3024+
response reading completes.
3025+
The signal is created lazily on first access β€” no {AbortController} is allocated
3026+
for requests that never use this property.
30203027

30213028
This is useful for cancelling downstream asynchronous work such as database
30223029
queries or `fetch` calls when a client disconnects mid-request.

β€Ž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
constreq=this.req;
10181019
constsocket=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
constkTrailersDistinct=Symbol('kTrailersDistinct');
3939
constkTrailersCount=Symbol('kTrailersCount');
4040
constkAbortController=Symbol('kAbortController');
41+
constkAbortSignalSocket=Symbol('kAbortSignalSocket');
42+
constkAbortSignalListener=Symbol('kAbortSignalListener');
43+
constkAbortSignalDetached=Symbol('kAbortSignalDetached');
44+
constkAttachAbortSignal=Symbol('kAttachAbortSignal');
45+
constkDetachAbortSignal=Symbol('kDetachAbortSignal');
4146

4247
functionreadStart(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
constac=newAbortController();
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
returnthis[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+
constsocket=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+
constsocket=this[kAbortSignalSocket];
242+
constlistener=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=functionsetTimeout(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+
functionabortSignal(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
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const common = require('../common');
44
constassert=require('assert');
55
consthttp=require('http');
66

7-
// Test 1: req.signal is an AbortSignal and aborts on 'close'
7+
// Test 1: req.signal is an AbortSignal and aborts on socket close
88
{
99
constserver=http.createServer(common.mustCall((req,res)=>{
1010
assert.ok(req.signalinstanceofAbortSignal);
@@ -21,21 +21,68 @@ const http = require('http');
2121
}));
2222
}
2323

24-
// Test 2: req.signal is aborted if accessed after destroy
24+
// Test 2: req.signal is not aborted when a request body completes normally.
25+
{
26+
constbody=JSON.stringify({hello: 'world'});
27+
constserver=http.createServer(common.mustCall((req,res)=>{
28+
assert.ok(req.signalinstanceofAbortSignal);
29+
assert.strictEqual(req.signal.aborted,false);
30+
req.signal.onabort=common.mustNotCall();
31+
32+
req.on('close',common.mustCall(()=>{
33+
assert.strictEqual(req.aborted,false);
34+
assert.strictEqual(req.complete,true);
35+
assert.strictEqual(req.signal.aborted,false);
36+
}));
37+
38+
req.on('end',common.mustCall(()=>{
39+
setTimeout(common.mustCall(()=>{
40+
assert.strictEqual(req.aborted,false);
41+
assert.strictEqual(req.complete,true);
42+
assert.strictEqual(req.signal.aborted,false);
43+
res.end('ok');
44+
}),10);
45+
}));
46+
req.resume();
47+
}));
48+
49+
server.listen(0,common.mustCall(()=>{
50+
constclientReq=http.request(
51+
{
52+
port: server.address().port,
53+
method: 'PATCH',
54+
path: '/tables/1',
55+
headers: {
56+
'content-type': 'application/json',
57+
'content-length': Buffer.byteLength(body),
58+
},
59+
},
60+
common.mustCall((res)=>{
61+
res.resume();
62+
res.on('end',common.mustCall(()=>{
63+
server.close();
64+
}));
65+
}),
66+
);
67+
clientReq.end(body);
68+
}));
69+
}
70+
71+
// Test 3: req.signal is aborted if accessed after destroy
2572
{
2673
constreq=newhttp.IncomingMessage(null);
2774
req.destroy();
2875
assert.strictEqual(req.signal.aborted,true);
2976
}
3077

31-
// Test 3: Multiple accesses return the same signal
78+
// Test 4: Multiple accesses return the same signal
3279
{
3380
constreq=newhttp.IncomingMessage(null);
3481
assert.strictEqual(req.signal,req.signal);
3582
}
3683

3784

38-
// Test 4: res.signal on a client-side http.request() response (IncomingMessage).
85+
// Test 5: res.signal on a client-side http.request() response (IncomingMessage).
3986
{
4087
constserver=http.createServer(common.mustCall((req,res)=>{
4188
res.writeHead(200);
@@ -61,7 +108,36 @@ const http = require('http');
61108
}));
62109
}
63110

64-
// Test 5: Client cancels a pending request.
111+
// Test 6: res.signal is not aborted when a response body completes normally.
112+
{
113+
constserver=http.createServer(common.mustCall((req,res)=>{
114+
res.end('ok');
115+
}));
116+
117+
server.listen(0,common.mustCall(()=>{
118+
constclientReq=http.request(
119+
{port: server.address().port},
120+
common.mustCall((res)=>{
121+
assert.ok(res.signalinstanceofAbortSignal);
122+
assert.strictEqual(res.signal.aborted,false);
123+
res.signal.onabort=common.mustNotCall();
124+
125+
res.resume();
126+
res.on('end',common.mustCall(()=>{
127+
assert.strictEqual(res.complete,true);
128+
assert.strictEqual(res.signal.aborted,false);
129+
}));
130+
res.on('close',common.mustCall(()=>{
131+
assert.strictEqual(res.signal.aborted,false);
132+
server.close();
133+
}));
134+
}),
135+
);
136+
clientReq.end();
137+
}));
138+
}
139+
140+
// Test 7: Client cancels a pending request.
65141
{
66142
constserver=http.createServer(common.mustCall((req,res)=>{
67143
req.signal.onabort=common.mustCall(()=>{

0 commit comments

Comments
Β (0)
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
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: #64392Fixes: #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
Original file line numberDiff line numberDiff line change
@@ -3010,13 +3010,20 @@ Calls `message.socket.setTimeout(msecs, callback)`.
30103010

30113011
<!-- YAML
30123012
added: v24.16.0
3013+
changes:
3014+
- version: REPLACEME
3015+
pr-url: https://github.com/nodejs/node/pull/64392
3016+
description: The signal is no longer aborted after the message
3017+
completes normally.
30133018
-->
30143019

30153020
* Type: {AbortSignal}
30163021

3017-
An {AbortSignal} that is aborted when the underlying socket closes or the
3018-
request is destroyed. The signal is created lazily on first access β€” no
3019-
{AbortController} is allocated for requests that never use this property.
3022+
An {AbortSignal} that is aborted when the message is destroyed before
3023+
completion or when its underlying socket closes before request handling or
3024+
response reading completes.
3025+
The signal is created lazily on first access β€” no {AbortController} is allocated
3026+
for requests that never use this property.
30203027

30213028
This is useful for cancelling downstream asynchronous work such as database
30223029
queries or `fetch` calls when a client disconnects mid-request.

β€Ž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
constreq=this.req;
10181019
constsocket=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
constkTrailersDistinct=Symbol('kTrailersDistinct');
3939
constkTrailersCount=Symbol('kTrailersCount');
4040
constkAbortController=Symbol('kAbortController');
41+
constkAbortSignalSocket=Symbol('kAbortSignalSocket');
42+
constkAbortSignalListener=Symbol('kAbortSignalListener');
43+
constkAbortSignalDetached=Symbol('kAbortSignalDetached');
44+
constkAttachAbortSignal=Symbol('kAttachAbortSignal');
45+
constkDetachAbortSignal=Symbol('kDetachAbortSignal');
4146

4247
functionreadStart(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
constac=newAbortController();
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
returnthis[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+
constsocket=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+
constsocket=this[kAbortSignalSocket];
242+
constlistener=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=functionsetTimeout(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+
functionabortSignal(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
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const common = require('../common');
44
constassert=require('assert');
55
consthttp=require('http');
66

7-
// Test 1: req.signal is an AbortSignal and aborts on 'close'
7+
// Test 1: req.signal is an AbortSignal and aborts on socket close
88
{
99
constserver=http.createServer(common.mustCall((req,res)=>{
1010
assert.ok(req.signalinstanceofAbortSignal);
@@ -21,21 +21,68 @@ const http = require('http');
2121
}));
2222
}
2323

24-
// Test 2: req.signal is aborted if accessed after destroy
24+
// Test 2: req.signal is not aborted when a request body completes normally.
25+
{
26+
constbody=JSON.stringify({hello: 'world'});
27+
constserver=http.createServer(common.mustCall((req,res)=>{
28+
assert.ok(req.signalinstanceofAbortSignal);
29+
assert.strictEqual(req.signal.aborted,false);
30+
req.signal.onabort=common.mustNotCall();
31+
32+
req.on('close',common.mustCall(()=>{
33+
assert.strictEqual(req.aborted,false);
34+
assert.strictEqual(req.complete,true);
35+
assert.strictEqual(req.signal.aborted,false);
36+
}));
37+
38+
req.on('end',common.mustCall(()=>{
39+
setTimeout(common.mustCall(()=>{
40+
assert.strictEqual(req.aborted,false);
41+
assert.strictEqual(req.complete,true);
42+
assert.strictEqual(req.signal.aborted,false);
43+
res.end('ok');
44+
}),10);
45+
}));
46+
req.resume();
47+
}));
48+
49+
server.listen(0,common.mustCall(()=>{
50+
constclientReq=http.request(
51+
{
52+
port: server.address().port,
53+
method: 'PATCH',
54+
path: '/tables/1',
55+
headers: {
56+
'content-type': 'application/json',
57+
'content-length': Buffer.byteLength(body),
58+
},
59+
},
60+
common.mustCall((res)=>{
61+
res.resume();
62+
res.on('end',common.mustCall(()=>{
63+
server.close();
64+
}));
65+
}),
66+
);
67+
clientReq.end(body);
68+
}));
69+
}
70+
71+
// Test 3: req.signal is aborted if accessed after destroy
2572
{
2673
constreq=newhttp.IncomingMessage(null);
2774
req.destroy();
2875
assert.strictEqual(req.signal.aborted,true);
2976
}
3077

31-
// Test 3: Multiple accesses return the same signal
78+
// Test 4: Multiple accesses return the same signal
3279
{
3380
constreq=newhttp.IncomingMessage(null);
3481
assert.strictEqual(req.signal,req.signal);
3582
}
3683

3784

38-
// Test 4: res.signal on a client-side http.request() response (IncomingMessage).
85+
// Test 5: res.signal on a client-side http.request() response (IncomingMessage).
3986
{
4087
constserver=http.createServer(common.mustCall((req,res)=>{
4188
res.writeHead(200);
@@ -61,7 +108,36 @@ const http = require('http');
61108
}));
62109
}
63110

64-
// Test 5: Client cancels a pending request.
111+
// Test 6: res.signal is not aborted when a response body completes normally.
112+
{
113+
constserver=http.createServer(common.mustCall((req,res)=>{
114+
res.end('ok');
115+
}));
116+
117+
server.listen(0,common.mustCall(()=>{
118+
constclientReq=http.request(
119+
{port: server.address().port},
120+
common.mustCall((res)=>{
121+
assert.ok(res.signalinstanceofAbortSignal);
122+
assert.strictEqual(res.signal.aborted,false);
123+
res.signal.onabort=common.mustNotCall();
124+
125+
res.resume();
126+
res.on('end',common.mustCall(()=>{
127+
assert.strictEqual(res.complete,true);
128+
assert.strictEqual(res.signal.aborted,false);
129+
}));
130+
res.on('close',common.mustCall(()=>{
131+
assert.strictEqual(res.signal.aborted,false);
132+
server.close();
133+
}));
134+
}),
135+
);
136+
clientReq.end();
137+
}));
138+
}
139+
140+
// Test 7: Client cancels a pending request.
65141
{
66142
constserver=http.createServer(common.mustCall((req,res)=>{
67143
req.signal.onabort=common.mustCall(()=>{

0 commit comments

Comments
Β (0)