From cb96305bcb5aa152eb19e28c2eda172078dbcba8 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Mon, 1 Aug 2016 18:43:58 -0700 Subject: [PATCH 1/2] 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. --- lib/_http_outgoing.js | 48 ++++++++-------- .../parallel/test-http-write-write-destroy.js | 57 +++++++++++++++++++ 2 files changed, 82 insertions(+), 23 deletions(-) create mode 100644 test/parallel/test-http-write-write-destroy.js diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index b85e3c54074d82..7004f7cc6f84af 100644 --- 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 new file mode 100644 index 00000000000000..ccc556efcedf37 --- /dev/null +++ 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(); + })); + }); +})); From 6acff8f77f63788de4771f2bf93f3f7db2fcdb19 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Tue, 2 Aug 2016 16:31:01 -0700 Subject: [PATCH 2/2] http: code cleanup & dry The first change in `_writeRaw`: This reduces drops an unnecessary if check (`outputLength`), because it is redone in `_flushOutput`. It also changes an if/else statement to an if statement, because the blocks were unrelated. The second change in `write`: This consolidates code in #write() that handled different string encodings and Buffers. There was no reason to handle the encodings differently, so after splitting them based on Buffer vs encoding, the code is consolidated. This might see a speedup. Shoutout to Ron Korving for spotting this. --- lib/_http_outgoing.js | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 7004f7cc6f84af..7b4eb81a084e5d 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -155,10 +155,10 @@ function _writeRaw(data, encoding, callback) { connection.writable && !connection.destroyed) { // There might be pending data in the this.output buffer. - var outputLength = this.output.length; - if (outputLength > 0) { - this._flushOutput(connection); - } else if (data.length === 0) { + this._flushOutput(connection); + + // Avoid writing empty messages, but trigger the callback. + if (data.length === 0) { if (typeof callback === 'function') process.nextTick(callback); return true; @@ -472,25 +472,16 @@ OutgoingMessage.prototype.write = function write(chunk, encoding, callback) { var len, ret; if (this.chunkedEncoding) { - if (typeof chunk === 'string' && - encoding !== 'hex' && - encoding !== 'base64' && - encoding !== 'latin1') { + if (typeof chunk === 'string') { len = Buffer.byteLength(chunk, encoding); - chunk = len.toString(16) + CRLF + chunk + CRLF; - ret = this._send(chunk, encoding, callback); } else { - // buffer, or a non-toString-friendly encoding - if (typeof chunk === 'string') - len = Buffer.byteLength(chunk, encoding); - else - len = chunk.length; - - this._send(len.toString(16), 'latin1', null); - this._send(crlf_buf, null, null); - this._send(chunk, encoding, null); - ret = this._send(crlf_buf, null, callback); + len = chunk.length; } + + this._send(len.toString(16), 'latin1', null); + this._send(crlf_buf, null, null); + this._send(chunk, encoding, null); + ret = this._send(crlf_buf, null, callback); } else { ret = this._send(chunk, encoding, callback); }