Skip to content

Commit

Permalink
http: wait for both prefinish/end to keepalive
Browse files Browse the repository at this point in the history
When `free`ing the socket to be reused in keep-alive Agent wait for both
`prefinish` and `end` events. Otherwise the next request may be written
before the previous one has finished sending the body, leading to a
parser errors.
  • Loading branch information
indutny committed Jun 4, 2016
1 parent edbb092 commit 492bdde
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 3 deletions.
27 changes: 24 additions & 3 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,8 @@ function ClientRequest(options, cb) {
self._flush();
self = null;
});

this._ended = false;
}

util.inherits(ClientRequest, OutgoingMessage);
Expand Down Expand Up @@ -466,6 +468,7 @@ function parserOnIncomingClient(res, shouldKeepAlive) {

// add our listener first, so that we guarantee socket cleanup
res.on('end', responseOnEnd);
req.on('prefinish', requestOnPrefinish);
var handled = req.emit('response', res);

// If the user did not listen for the 'response' event, then they
Expand All @@ -478,9 +481,7 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
}

// client
function responseOnEnd() {
var res = this;
var req = res.req;
function responseKeepAlive(res, req) {
var socket = req.socket;

if (!req.shouldKeepAlive) {
Expand All @@ -504,6 +505,26 @@ function responseOnEnd() {
}
}

function responseOnEnd() {
const res = this;
const req = this.req;

req._ended = true;
if (!req.shouldKeepAlive || req.finished)
responseKeepAlive(res, req);
}

function requestOnPrefinish() {
const req = this;
const res = this.res;

if (!req.shouldKeepAlive)
return;

if (req._ended)
responseKeepAlive(res, req);
}

function emitFreeNT(socket) {
socket.emit('free');
}
Expand Down
38 changes: 38 additions & 0 deletions test/parallel/test-http-client-keep-alive-release-before-finish.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';
const common = require('../common');
const http = require('http');

const server = http.createServer((req, res) => {
res.end();
}).listen(common.PORT, common.mustCall(() => {
const agent = new http.Agent({
maxSockets: 1,
keepAlive: true
});

const post = http.request({
agent: agent,
method: 'POST',
port: common.PORT,
}, common.mustCall((res) => {
res.resume();
}));

/* What happens here is that the server `end`s the response before we send
* `something`, and the client thought that this is a green light for sending
* next GET request
*/
post.write(Buffer.alloc(16 * 1024).fill('X'));
setTimeout(() => {
post.end('something');
}, 100);

http.request({
agent: agent,
method: 'GET',
port: common.PORT,
}, common.mustCall((res) => {
server.close();
res.connection.end();
})).end();
}));

0 comments on commit 492bdde

Please sign in to comment.