From 8191c018782e822bb6aea8e9ed9b1cdef854bbb8 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Thu, 13 Jul 2017 10:36:44 -0800 Subject: [PATCH 1/4] net: fix bytesWritten during writev When a writev is caused on a socket (sometimes through corking and uncorking), previously net would call Buffer.byteLength on the array of buffers and chunks. This throws a TypeError, because Buffer.byteLength throws when passed a non-string. In dbfe8c4e, behavior changed to throw when passed a non-string. This is correct behavior. Previously, it would cast the argument to a string, so before this commit, bytesWritten would give an erroneous value. This commit corrects the behavior equally both before and after dbfe8c4e. This commit fixes this bug by iterating over each chunk in the pending stack and calculating the length individually. Also adds a regression test. Refs: https://github.com/nodejs/node/pull/2960 --- lib/net.js | 12 ++++++- test/parallel/test-net-socket-byteswritten.js | 36 +++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-net-socket-byteswritten.js diff --git a/lib/net.js b/lib/net.js index 5129a596421e1c..530b663caac65d 100644 --- a/lib/net.js +++ b/lib/net.js @@ -825,7 +825,17 @@ protoGetter('bytesWritten', function bytesWritten() { bytes += Buffer.byteLength(el.chunk, el.encoding); }); - if (data) { + if (Array.isArray(data)) { + // was a writev, iterate over chunks to get total length + for (var i = 0; i < data.length; i++) { + const chunk = data[i]; + + if (chunk instanceof Buffer) + bytes += chunk.length; + else + bytes += Buffer.byteLength(chunk.chunk, chunk.encoding); + } + } else if (data) { if (data instanceof Buffer) bytes += data.length; else diff --git a/test/parallel/test-net-socket-byteswritten.js b/test/parallel/test-net-socket-byteswritten.js new file mode 100644 index 00000000000000..38a0a05b3149b9 --- /dev/null +++ b/test/parallel/test-net-socket-byteswritten.js @@ -0,0 +1,36 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const net = require('net'); +const Buffer = require('buffer').Buffer; + +const server = net.createServer(function(socket) { + socket.end(); +}); + +server.listen(common.PORT, common.mustCall(function() { + const socket = net.connect(common.PORT); + + // Cork the socket, then write twice; this should cause a writev, which + // previously caused an err in the bytesWritten count. + socket.cork(); + + socket.write('one'); + socket.write(new Buffer('twø', 'utf8')); + + socket.uncork(); + + // one = 3 bytes, twø = 4 bytes + assert.strictEqual(socket.bytesWritten, 3 + 4); + + socket.on('connect', common.mustCall(function() { + assert.strictEqual(socket.bytesWritten, 3 + 4); + })); + + socket.on('end', common.mustCall(function() { + assert.strictEqual(socket.bytesWritten, 3 + 4); + + server.close(); + })); +})); From ab6917bddffa8587883f7993fa87f758d5238bb2 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Fri, 14 Jul 2017 19:22:07 -0700 Subject: [PATCH 2/4] !fixup fix test nits, net.js collapse if/else --- lib/net.js | 7 +++---- test/parallel/test-net-socket-byteswritten.js | 5 ++--- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/net.js b/lib/net.js index 530b663caac65d..610db229ea2ce9 100644 --- a/lib/net.js +++ b/lib/net.js @@ -835,11 +835,10 @@ protoGetter('bytesWritten', function bytesWritten() { else bytes += Buffer.byteLength(chunk.chunk, chunk.encoding); } + } else if (data instanceof Buffer) { + bytes += data.length; } else if (data) { - if (data instanceof Buffer) - bytes += data.length; - else - bytes += Buffer.byteLength(data, encoding); + bytes += Buffer.byteLength(data, encoding); } return bytes; diff --git a/test/parallel/test-net-socket-byteswritten.js b/test/parallel/test-net-socket-byteswritten.js index 38a0a05b3149b9..6f3ce8a3c6c8d1 100644 --- a/test/parallel/test-net-socket-byteswritten.js +++ b/test/parallel/test-net-socket-byteswritten.js @@ -3,14 +3,13 @@ const common = require('../common'); const assert = require('assert'); const net = require('net'); -const Buffer = require('buffer').Buffer; const server = net.createServer(function(socket) { socket.end(); }); -server.listen(common.PORT, common.mustCall(function() { - const socket = net.connect(common.PORT); +server.listen(0, common.mustCall(function() { + const socket = net.connect(server.address().port); // Cork the socket, then write twice; this should cause a writev, which // previously caused an err in the bytesWritten count. From 1979f7f2d5a5a7b15de90979dda43212d0096257 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Mon, 17 Jul 2017 17:23:31 -0700 Subject: [PATCH 3/4] !fixup avoid instanceof with data.allBuffers --- lib/net.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/net.js b/lib/net.js index 610db229ea2ce9..3cc5ebbbac8c3c 100644 --- a/lib/net.js +++ b/lib/net.js @@ -830,7 +830,7 @@ protoGetter('bytesWritten', function bytesWritten() { for (var i = 0; i < data.length; i++) { const chunk = data[i]; - if (chunk instanceof Buffer) + if (data.allBuffers || chunk instanceof Buffer) bytes += chunk.length; else bytes += Buffer.byteLength(chunk.chunk, chunk.encoding); From 9aadbf38cafcd788609e83e1e3fc04b6e5d7241f Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Wed, 26 Jul 2017 19:15:18 -0700 Subject: [PATCH 4/4] !fixup replace instanceof with typeof !== 'string' This should be faster. Because it's either a Buffer or a string, this check should be equivalent. --- lib/net.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/net.js b/lib/net.js index 3cc5ebbbac8c3c..77823b9f8d9fc9 100644 --- a/lib/net.js +++ b/lib/net.js @@ -835,10 +835,12 @@ protoGetter('bytesWritten', function bytesWritten() { else bytes += Buffer.byteLength(chunk.chunk, chunk.encoding); } - } else if (data instanceof Buffer) { - bytes += data.length; } else if (data) { - bytes += Buffer.byteLength(data, encoding); + // Writes are either a string or a Buffer. + if (typeof data !== 'string') + bytes += data.length; + else + bytes += Buffer.byteLength(data, encoding); } return bytes;