http: batch writes together more efficiently

This commit opts for a simpler way to batch writes to HTTP clients into fewer packets. Instead of the complicated snafu which was before, now OutgoingMessage#write automatically corks the socket and uncorks on the next tick, allowing streams to batch them efficiently. It also makes the code cleaner and removes an ugly-ish hack.
nodejs · brendanashworth · Aug 2, 2016 · Aug 2, 2016 · Dec 14, 2016 · cb96305bcb5aa152eb19e28c2eda172078dbcba8
commit cb96305bcb5aa152eb19e28c2eda172078dbcba8
diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js
@@ -83,6 +83,10 @@ function OutgoingMessage() {
   this._headers = null;
   this._headerNames = {};
 
+  // When this is written to, it will cork for a tick. This represents
+  // whether the socket is corked for that reason or not.
+  this._corkedForWrite = false;
+
   this._onPendingData = null;
 }
 util.inherits(OutgoingMessage, Stream);
@@ -112,33 +116,26 @@ OutgoingMessage.prototype.setTimeout = function setTimeout(msecs, callback) {
 // any messages, before ever calling this.  In that case, just skip
 // it, since something else is destroying this connection anyway.
 OutgoingMessage.prototype.destroy = function destroy(error) {
-  if (this.socket)
+  if (this.socket) {
+    // If the socket is corked from a write, uncork it before destroying it.
+    if (this._corkedForWrite)
+      this.socket.uncork();
+
     this.socket.destroy(error);
-  else
+  } else {
     this.once('socket', function(socket) {
       socket.destroy(error);
     });
+  }
 };
 
 
 // This abstract either writing directly to the socket or buffering it.
 OutgoingMessage.prototype._send = function _send(data, encoding, callback) {
-  // This is a shameful hack to get the headers and first body chunk onto
-  // the same packet. Future versions of Node are going to take care of
-  // this at a lower level and in a more general way.
+  // Send the headers before the body. OutgoingMessage#write will cork
+  // the stream before calling #_send, batching them in the same packet.
   if (!this._headerSent) {
-    if (typeof data === 'string' &&
-        encoding !== 'hex' &&
-        encoding !== 'base64') {
-      data = this._header + data;
-    } else {
-      this.output.unshift(this._header);
-      this.outputEncodings.unshift('latin1');
-      this.outputCallbacks.unshift(null);
-      this.outputSize += this._header.length;
-      if (typeof this._onPendingData === 'function')
-        this._onPendingData(this._header.length);
-    }
+    this._writeRaw(this._header, 'latin1', null);
     this._headerSent = true;
   }
   return this._writeRaw(data, encoding, callback);
@@ -465,6 +462,14 @@ OutgoingMessage.prototype.write = function write(chunk, encoding, callback) {
   // signal the user to keep writing.
   if (chunk.length === 0) return true;
 
+  // By corking the socket and uncorking after a tick, we manage to
+  // batch writes together, i.e. the header with the body.
+  if (this.connection && !this._corkedForWrite) {
+    this.connection.cork();
+    this._corkedForWrite = true;
+    process.nextTick(connectionCorkNT, this);
+  }
+
   var len, ret;
   if (this.chunkedEncoding) {
     if (typeof chunk === 'string' &&
@@ -481,10 +486,6 @@ OutgoingMessage.prototype.write = function write(chunk, encoding, callback) {
       else
         len = chunk.length;
 
-      if (this.connection && !this.connection.corked) {
-        this.connection.cork();
-        process.nextTick(connectionCorkNT, this.connection);
-      }
       this._send(len.toString(16), 'latin1', null);
       this._send(crlf_buf, null, null);
       this._send(chunk, encoding, null);
@@ -505,8 +506,9 @@ function writeAfterEndNT(self, err, callback) {
 }
 
 
-function connectionCorkNT(conn) {
-  conn.uncork();
+function connectionCorkNT(msg) {
+  msg.connection.uncork();
+  msg._corkedForWrite = false;
 }
 
 

diff --git a/test/parallel/test-http-write-write-destroy.js b/test/parallel/test-http-write-write-destroy.js
@@ -0,0 +1,57 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+// Ensure that, in a corked response, calling .destroy() will flush what was
+// previously written completely rather than partially or none at all. It
+// also makes sure it's written in a single packet.
+
+var hasFlushed = false;
+
+var server = http.createServer(common.mustCall((req, res) => {
+  res.write('first.');
+  res.write('.second', function() {
+    // Set the flag to prove that all data has been written.
+    hasFlushed = true;
+  });
+
+  res.destroy();
+
+  // If the second callback from the .write() calls hasn't executed before
+  // the next tick, then the write has been buffered and was sent
+  // asynchronously. This means it wouldn't have been written regardless of
+  // corking, making the test irrelevant, so skip it.
+  process.nextTick(function() {
+    if (hasFlushed) return;
+
+    common.skip('.write() executed asynchronously');
+    process.exit(0);
+    return;
+  });
+}));
+
+server.listen(0);
+
+server.on('listening', common.mustCall(() => {
+  // Send a request, and assert the response.
+  http.get({
+    port: server.address().port
+  }, (res) => {
+    var data = '';
+
+    // By ensuring that the 'data' event is only emitted once, we ensure that
+    // the socket was correctly corked and the data was batched.
+    res.on('data', common.mustCall(function(chunk) {
+      data += chunk.toString('latin1');
+    }, 2));
+
+    res.on('end', common.mustCall(function() {
+      assert.equal(data, 'first..second');
+
+      res.destroy();
+      server.close();
+    }));
+  });
+}));