http: wait for both prefinish/end to keepalive

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.

Author: indutny

lib/_http_client.js

@@ -195,6 +195,8 @@ function ClientRequest(options, cb) {
     self._flush();
     self = null;
   });
+
+  this._ended = false;
 }
 
 util.inherits(ClientRequest, OutgoingMessage);
@@ -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
@@ -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) {
@@ -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');
 }

test/parallel/test-http-client-keep-alive-release-before-finish.js

@@ -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();
+}));