Skip to content

Commit 60a585e

Browse files
ronagaduh95
authored andcommitted
http: cleanup pipeline queue
When a socket with pipelined requests is destroyed then some requests will leak. PR-URL: #62534 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
1 parent 9047ec1 commit 60a585e

4 files changed

Lines changed: 92 additions & 4 deletions

File tree

‎lib/_http_outgoing.js‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -328,14 +328,19 @@ OutgoingMessage.prototype.destroy = function destroy(error) {
328328
if(this[kSocket]){
329329
this[kSocket].destroy(error);
330330
}else{
331-
this.once('socket',functionsocketDestroyOnConnect(socket){
332-
socket.destroy(error);
333-
});
331+
process.nextTick(emitDestroyNT,this);
334332
}
335333

336334
returnthis;
337335
};
338336

337+
functionemitDestroyNT(self){
338+
if(!self._closed){
339+
self._closed=true;
340+
self.emit('close');
341+
}
342+
}
343+
339344

340345
// This abstract either writing directly to the socket or buffering it.
341346
OutgoingMessage.prototype._send=function_send(data,encoding,callback,byteLength){

‎lib/_http_server.js‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -841,14 +841,21 @@ function socketOnClose(socket, state) {
841841
debug('server socket close');
842842
freeParser(socket.parser,null,socket);
843843
abortIncoming(state.incoming);
844+
abortOutgoing(state.outgoing);
844845
}
845846

846847
functionabortIncoming(incoming){
847848
while(incoming.length){
848849
constreq=incoming.shift();
849850
req.destroy(newConnResetException('aborted'));
850851
}
851-
// Abort socket._httpMessage ?
852+
}
853+
854+
functionabortOutgoing(outgoing){
855+
while(outgoing.length){
856+
constreq=outgoing.shift();
857+
req.destroy(newConnResetException('aborted'));
858+
}
852859
}
853860

854861
functionsocketOnEnd(server,socket,parser,state){

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,32 @@
22
constcommon=require('../common');
33
consthttp=require('http');
44
constassert=require('assert');
5+
const{ OutgoingMessage }=require('http');
6+
7+
// OutgoingMessage.destroy() with no socket should emit 'close' and set closed.
8+
{
9+
constmsg=newOutgoingMessage();
10+
assert.strictEqual(msg.destroyed,false);
11+
assert.strictEqual(msg.closed,false);
12+
msg.on('close',common.mustCall(()=>{
13+
assert.strictEqual(msg.destroyed,true);
14+
assert.strictEqual(msg.closed,true);
15+
}));
16+
msg.destroy();
17+
assert.strictEqual(msg.destroyed,true);
18+
}
19+
20+
// OutgoingMessage.destroy(err) with no socket should set errored and emit 'close'.
21+
{
22+
constmsg=newOutgoingMessage();
23+
consterr=newError('test destroy');
24+
msg.on('close',common.mustCall(()=>{
25+
assert.strictEqual(msg.closed,true);
26+
assert.strictEqual(msg.errored,err);
27+
}));
28+
msg.destroy(err);
29+
assert.strictEqual(msg.errored,err);
30+
}
531

632
{
733
constserver=http.createServer(common.mustCall((req,res)=>{
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
'use strict';
2+
// Test that queued (pipelined) outgoing responses are destroyed when the
3+
// socket closes before the first response has finished. Previously,
4+
// socketOnClose only aborted state.incoming (pending requests) but left
5+
// state.outgoing responses with socket=null alive forever.
6+
7+
constcommon=require('../common');
8+
consthttp=require('http');
9+
constnet=require('net');
10+
constassert=require('assert');
11+
12+
letrequestCount=0;
13+
14+
constserver=http.createServer(common.mustCall((req,res)=>{
15+
requestCount++;
16+
17+
if(requestCount===1){
18+
// Keep the first response open so the second response is queued in
19+
// state.outgoing with socket === null.
20+
res.writeHead(200);
21+
res.write('start');
22+
// Intentionally do not call res.end().
23+
}else{
24+
// The second response should be queued — no socket assigned yet.
25+
assert.strictEqual(res.socket,null);
26+
assert.strictEqual(res.destroyed,false);
27+
assert.strictEqual(res.closed,false);
28+
29+
res.on('close',common.mustCall(()=>{
30+
assert.strictEqual(res.destroyed,true);
31+
assert.strictEqual(res.closed,true);
32+
server.close();
33+
}));
34+
35+
// Simulate client dying while first response is still in flight.
36+
req.socket.destroy();
37+
}
38+
},2));
39+
40+
server.listen(0,common.mustCall(function(){
41+
constport=this.address().port;
42+
constclient=net.connect(port);
43+
44+
// Send two pipelined HTTP/1.1 requests at once.
45+
client.write(
46+
`GET /1 HTTP/1.1\r\nHost: localhost:${port}\r\n\r\n`+
47+
`GET /2 HTTP/1.1\r\nHost: localhost:${port}\r\n\r\n`,
48+
);
49+
client.resume();
50+
}));

0 commit comments

Comments
 (0)