Skip to content

Commit 173d044

Browse files
ronagronag
committed
http: align OutgoingMessage and ClientRequest destroy
Added .destroyed property to OutgoingMessage and ClientRequest to align with streams. Fixed ClientRequest.destroy to dump res and re-use socket in agent pool aligning it with abort. PR-URL: #32148 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
1 parent de8fab9 commit 173d044

File tree

7 files changed

+155
-38
lines changed

7 files changed

+155
-38
lines changed

‎lib/_http_client.js‎

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ const{
2929
ObjectAssign,
3030
ObjectKeys,
3131
ObjectSetPrototypeOf,
32+
Symbol
3233
}=primordials;
3334

3435
constnet=require('net');
@@ -65,6 +66,7 @@ const{
6566
}=require('internal/dtrace');
6667

6768
constINVALID_PATH_REGEX=/[^\u0021-\u00ff]/;
69+
constkError=Symbol('kError');
6870

6971
functionvalidateHost(host,name){
7072
if(host!==null&&host!==undefined&&typeofhost!=='string'){
@@ -337,10 +339,19 @@ ClientRequest.prototype._implicitHeader = function _implicitHeader(){
337339
};
338340

339341
ClientRequest.prototype.abort=functionabort(){
340-
if(!this.aborted){
341-
process.nextTick(emitAbortNT,this);
342+
if(this.aborted){
343+
return;
342344
}
343345
this.aborted=true;
346+
process.nextTick(emitAbortNT,this);
347+
this.destroy();
348+
};
349+
350+
ClientRequest.prototype.destroy=functiondestroy(err){
351+
if(this.destroyed){
352+
return;
353+
}
354+
this.destroyed=true;
344355

345356
// If we're aborting, we don't care about any more response data.
346357
if(this.res){
@@ -350,11 +361,29 @@ ClientRequest.prototype.abort = function abort(){
350361
// In the event that we don't have a socket, we will pop out of
351362
// the request queue through handling in onSocket.
352363
if(this.socket){
353-
// in-progress
354-
this.socket.destroy();
364+
_destroy(this,this.socket,err);
365+
}elseif(err){
366+
this[kError]=err;
355367
}
356368
};
357369

370+
function_destroy(req,socket,err){
371+
// TODO (ronag): Check if socket was used at all (e.g. headersSent) and
372+
// re-use it in that case. `req.socket` just checks whether the socket was
373+
// assigned to the request and *might* have been used.
374+
if(!req.agent||req.socket){
375+
socket.destroy(err);
376+
}else{
377+
socket.emit('free');
378+
if(!req.aborted&&!err){
379+
err=connResetException('socket hang up');
380+
}
381+
if(err){
382+
req.emit('error',err);
383+
}
384+
req.emit('close');
385+
}
386+
}
358387

359388
functionemitAbortNT(req){
360389
req.emit('abort');
@@ -750,14 +779,8 @@ ClientRequest.prototype.onSocket = function onSocket(socket){
750779
};
751780

752781
functiononSocketNT(req,socket){
753-
if(req.aborted){
754-
// If we were aborted while waiting for a socket, skip the whole thing.
755-
if(!req.agent){
756-
socket.destroy();
757-
}else{
758-
req.emit('close');
759-
socket.emit('free');
760-
}
782+
if(req.destroyed){
783+
_destroy(req,socket,req[kError]);
761784
}else{
762785
tickOnSocket(req,socket);
763786
}

‎lib/_http_outgoing.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ function OutgoingMessage(){
9393
this.outputSize=0;
9494

9595
this.writable=true;
96+
this.destroyed=false;
9697

9798
this._last=false;
9899
this.chunkedEncoding=false;
@@ -277,6 +278,11 @@ OutgoingMessage.prototype.setTimeout = function setTimeout(msecs, callback){
277278
// any messages, before ever calling this. In that case, just skip
278279
// it, since something else is destroying this connection anyway.
279280
OutgoingMessage.prototype.destroy=functiondestroy(error){
281+
if(this.destroyed){
282+
return;
283+
}
284+
this.destroyed=true;
285+
280286
if(this.socket){
281287
this.socket.destroy(error);
282288
}else{

‎lib/internal/streams/destroy.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,6 @@ function isRequest(stream){
163163

164164
// Normalize destroy for legacy.
165165
functiondestroyer(stream,err){
166-
// request.destroy just do .end - .abort is what we want
167166
if(isRequest(stream))returnstream.abort();
168167
if(isRequest(stream.req))returnstream.req.abort();
169168
if(typeofstream.destroy==='function')returnstream.destroy(err);

‎test/parallel/test-http-client-abort-destroy.js‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
'use strict';
2+
constcommon=require('../common');
3+
consthttp=require('http');
4+
constassert=require('assert');
5+
6+
{
7+
// abort
8+
9+
constserver=http.createServer(common.mustCall((req,res)=>{
10+
res.end('Hello');
11+
}));
12+
13+
server.listen(0,common.mustCall(()=>{
14+
constoptions={port: server.address().port};
15+
constreq=http.get(options,common.mustCall((res)=>{
16+
res.on('data',(data)=>{
17+
req.abort();
18+
assert.strictEqual(req.aborted,true);
19+
assert.strictEqual(req.destroyed,true);
20+
server.close();
21+
});
22+
}));
23+
req.on('error',common.mustNotCall());
24+
assert.strictEqual(req.aborted,false);
25+
assert.strictEqual(req.destroyed,false);
26+
}));
27+
}
28+
29+
{
30+
// destroy + res
31+
32+
constserver=http.createServer(common.mustCall((req,res)=>{
33+
res.end('Hello');
34+
}));
35+
36+
server.listen(0,common.mustCall(()=>{
37+
constoptions={port: server.address().port};
38+
constreq=http.get(options,common.mustCall((res)=>{
39+
res.on('data',(data)=>{
40+
req.destroy();
41+
assert.strictEqual(req.aborted,false);
42+
assert.strictEqual(req.destroyed,true);
43+
server.close();
44+
});
45+
}));
46+
req.on('error',common.mustNotCall());
47+
assert.strictEqual(req.aborted,false);
48+
assert.strictEqual(req.destroyed,false);
49+
}));
50+
}
51+
52+
{
53+
// destroy
54+
55+
constserver=http.createServer(common.mustNotCall((req,res)=>{
56+
}));
57+
58+
server.listen(0,common.mustCall(()=>{
59+
constoptions={port: server.address().port};
60+
constreq=http.get(options,common.mustNotCall());
61+
req.on('error',common.mustCall((err)=>{
62+
assert.strictEqual(err.code,'ECONNRESET');
63+
server.close();
64+
}));
65+
assert.strictEqual(req.aborted,false);
66+
assert.strictEqual(req.destroyed,false);
67+
req.destroy();
68+
assert.strictEqual(req.aborted,false);
69+
assert.strictEqual(req.destroyed,true);
70+
}));
71+
}

‎test/parallel/test-http-client-abort-keep-alive-queued-tcp-socket.js‎

Lines changed: 34 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -3,34 +3,45 @@ const common = require('../common');
33
constassert=require('assert');
44
consthttp=require('http');
55

6-
letsocketsCreated=0;
76

8-
classAgentextendshttp.Agent{
9-
createConnection(options,oncreate){
10-
constsocket=super.createConnection(options,oncreate);
11-
socketsCreated++;
12-
returnsocket;
7+
for(constdestroyerof['destroy','abort']){
8+
letsocketsCreated=0;
9+
10+
classAgentextendshttp.Agent{
11+
createConnection(options,oncreate){
12+
constsocket=super.createConnection(options,oncreate);
13+
socketsCreated++;
14+
returnsocket;
15+
}
1316
}
14-
}
1517

16-
constserver=http.createServer((req,res)=>res.end());
18+
constserver=http.createServer((req,res)=>res.end());
1719

18-
server.listen(0,common.mustCall(()=>{
19-
constport=server.address().port;
20-
constagent=newAgent({
21-
keepAlive: true,
22-
maxSockets: 1
23-
});
20+
server.listen(0,common.mustCall(()=>{
21+
constport=server.address().port;
22+
constagent=newAgent({
23+
keepAlive: true,
24+
maxSockets: 1
25+
});
2426

25-
http.get({ agent, port },(res)=>res.resume());
27+
http.get({ agent, port },(res)=>res.resume());
2628

27-
constreq=http.get({ agent, port },common.mustNotCall());
28-
req.abort();
29+
constreq=http.get({ agent, port },common.mustNotCall());
30+
req[destroyer]();
2931

30-
http.get({ agent, port },common.mustCall((res)=>{
31-
res.resume();
32-
assert.strictEqual(socketsCreated,1);
33-
agent.destroy();
34-
server.close();
32+
if(destroyer==='destroy'){
33+
req.on('error',common.mustCall((err)=>{
34+
assert.strictEqual(err.code,'ECONNRESET');
35+
}));
36+
}else{
37+
req.on('error',common.mustNotCall());
38+
}
39+
40+
http.get({ agent, port },common.mustCall((res)=>{
41+
res.resume();
42+
assert.strictEqual(socketsCreated,1);
43+
agent.destroy();
44+
server.close();
45+
}));
3546
}));
36-
}));
47+
}

‎test/parallel/test-http-client-close-event.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,12 @@ server.listen(0, common.mustCall(() =>{
1414
constreq=http.get({port: server.address().port},common.mustNotCall());
1515
leterrorEmitted=false;
1616

17-
req.on('error',(err)=>{
17+
req.on('error',common.mustCall((err)=>{
1818
errorEmitted=true;
1919
assert.strictEqual(err.constructor,Error);
2020
assert.strictEqual(err.message,'socket hang up');
2121
assert.strictEqual(err.code,'ECONNRESET');
22-
});
22+
}));
2323

2424
req.on('close',common.mustCall(()=>{
2525
assert.strictEqual(errorEmitted,true);

‎test/parallel/test-http-outgoing-proto.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,3 +122,10 @@ assert.throws(() =>{
122122
name: 'TypeError',
123123
message: 'Invalid character in trailer content ["404"]'
124124
});
125+
126+
{
127+
constoutgoingMessage=newOutgoingMessage();
128+
assert.strictEqual(outgoingMessage.destroyed,false);
129+
outgoingMessage.destroy();
130+
assert.strictEqual(outgoingMessage.destroyed,true);
131+
}

0 commit comments

Comments
(0)