net: fix bytesWritten during writev #14236

brendanashworth · 2017-07-14T18:12:21Z

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 dbfe8c4, 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 dbfe8c4.

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: #2960

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

net

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 dbfe8c4, 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 dbfe8c4. 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: nodejs#2960

lpinca · 2017-07-14T19:29:04Z

test/parallel/test-net-socket-byteswritten.js

+const common = require('../common');
+const assert = require('assert');
+const net = require('net');
+const Buffer = require('buffer').Buffer;


Nit: Using the global Buffer is fine in tests.

lpinca · 2017-07-14T19:33:27Z

test/parallel/test-net-socket-byteswritten.js

+  socket.end();
+});
+
+server.listen(common.PORT, common.mustCall(function() {


Please use a dynamic port for parallel tests.

Thanks! fixed

lpinca · 2017-07-14T19:39:28Z

I think this will have a non 0 performance impact as it uses istanceof for each chunk but I guess there is no way around that.

mscdex · 2017-07-14T20:43:31Z

lib/net.js

+      else
+        bytes += Buffer.byteLength(chunk.chunk, chunk.encoding);
+    }
+  } else if (data) {


Perhaps this can be simplified a bit by merging with the if/else below:

if (Array.isArray(data)) { // ... } else if (data instanceof Buffer) { // ... } else if (data) { // ... }

Good tip! I just collapsed the branches.

mscdex · 2017-07-14T20:49:22Z

lib/net.js

+    for (var i = 0; i < data.length; i++) {
+      const chunk = data[i];
+
+      if (chunk instanceof Buffer)


We can avoid this check for each item in the array if data.allBuffers === true. writeGeneric() uses this optimization already.

I think this will have a non 0 performance impact as it uses istanceof for each chunk but I guess there is no way around that.

/cc @lpinca ^

Is it worth optimizing? This is a pretty rarely-traveled code area, so I'd favor simplicity here, but I can change it if you'd like.

I don't know how much bytesWritten is used, but I mostly mentioned the optimization because of @lpinca's comment.

I think @mscdex’s suggestion makes sense. Also, using process.binding('util').isUint8Array(chunk) should be a faster alternative to a full instanceof check, that should be okay here.

I've added the data.allBuffers check, thanks.

@addaleax I'm not so sure that's faster — this benchmark says its a bit slower, probably because of going past the C++ boundary.

jasnell · 2017-07-17T15:26:31Z

lib/net.js

+      else
+        bytes += Buffer.byteLength(chunk.chunk, chunk.encoding);
+    }
+  } else if (data instanceof Buffer) {


These instanceof checks are going to be expensive on perf

This specific check wasn't added in this PR, it was just down-indented (no perf change).

I wonder if we could get away with using data != undefined && typeof data !== 'string' instead? If so, we could first branch on the first condition and then do a if (typeof data !== 'string') { ... } else { ... } to avoid duplicate if (data)-style checks.

@mscdex Yes, .write() will error if it's neither a Buffer nor a string, so this works! I've updated it with the optimization.

brendanashworth · 2017-07-18T00:28:48Z

I've added a way to avoid the instanceof check, PTAL. Please note that the only performance regression involved in this change is the new Array.isArray call, the rest helps/does not hurt previous performance.

edit CI: https://ci.nodejs.org/job/node-test-pull-request/9211/
.. edit second CI: https://ci.nodejs.org/job/node-test-pull-request/9278/

brendanashworth · 2017-07-25T23:14:01Z

The CI is green, minus OSX, which seems unrelated. Does this LGTY @mscdex @jasnell @lpinca ?

This should be faster. Because it's either a Buffer or a string, this check should be equivalent.

mscdex · 2017-07-27T02:24:36Z

LGTM for now if CI is still green: https://ci.nodejs.org/job/node-test-pull-request/9369/.

lpinca · 2017-07-27T05:36:54Z

Still LGTM.

brendanashworth · 2017-07-28T23:08:13Z

Thanks everyone. Two test failures:

ubuntu1604-arm64: not ok 1494 known_issues/test-stdout-buffer-flush-on-exit
win2008r2/vs2015/3: not ok 388 inspector/test-inspector-stop-profile-after-done

These failures are unrelated, so I'll merge tonight without objections.

tniessen

LGTM if CI passes.

brendanashworth · 2017-07-29T18:38:58Z

Landed in 85a5e5a, thanks everyone.

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 dbfe8c4, 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 dbfe8c4. This commit fixes this bug by iterating over each chunk in the pending stack and calculating the length individually. Also adds a regression test. This additionally changes an `instanceof Buffer` check to `typeof !== 'string'`, which should be equivalent. PR-URL: #14236 Reviewed-By: Brian White <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Refs: #2960

MylesBorins · 2017-08-16T08:19:25Z

should this be backported to v6.x?

brendanashworth · 2017-08-16T22:10:12Z

@MylesBorins possibly — nobody's reported it as a bug before — so it's up to you.

MylesBorins · 2017-09-19T17:18:58Z

the original references commit does not exist on v6.x, so I'm going to opt to not backport for now

@nodejs/tsc feel free to chime in if you think we should reconsider

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Jul 14, 2017

lpinca approved these changes Jul 14, 2017

View reviewed changes

mscdex reviewed Jul 14, 2017

View reviewed changes

!fixup fix test nits, net.js collapse if/else

ab6917b

jasnell reviewed Jul 17, 2017

View reviewed changes

jasnell approved these changes Jul 17, 2017

View reviewed changes

!fixup avoid instanceof with data.allBuffers

1979f7f

!fixup replace instanceof with typeof !== 'string'

9aadbf3

This should be faster. Because it's either a Buffer or a string, this check should be equivalent.

tniessen approved these changes Jul 29, 2017

View reviewed changes

brendanashworth closed this Jul 29, 2017

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

MylesBorins added the lts-watch-v6.x label Aug 16, 2017

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Sep 19, 2017

MylesBorins added dont-land-on-v6.x and removed land-on-v6.x labels Oct 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: fix bytesWritten during writev #14236

net: fix bytesWritten during writev #14236

brendanashworth commented Jul 14, 2017

lpinca Jul 14, 2017

brendanashworth Jul 15, 2017

lpinca Jul 14, 2017

brendanashworth Jul 15, 2017

lpinca commented Jul 14, 2017

mscdex Jul 14, 2017 •

edited

Loading

brendanashworth Jul 15, 2017

mscdex Jul 14, 2017 •

edited

Loading

brendanashworth Jul 15, 2017

mscdex Jul 15, 2017 •

edited

Loading

addaleax Jul 15, 2017

brendanashworth Jul 18, 2017

jasnell Jul 17, 2017

brendanashworth Jul 18, 2017

mscdex Jul 26, 2017

brendanashworth Jul 27, 2017

brendanashworth commented Jul 18, 2017 •

edited

Loading

brendanashworth commented Jul 25, 2017

mscdex commented Jul 27, 2017 •

edited

Loading

lpinca commented Jul 27, 2017

brendanashworth commented Jul 28, 2017

tniessen left a comment

brendanashworth commented Jul 29, 2017

MylesBorins commented Aug 16, 2017

brendanashworth commented Aug 16, 2017

MylesBorins commented Sep 19, 2017

net: fix bytesWritten during writev #14236

net: fix bytesWritten during writev #14236

Conversation

brendanashworth commented Jul 14, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca commented Jul 14, 2017

mscdex Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jul 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanashworth commented Jul 18, 2017 • edited Loading

brendanashworth commented Jul 25, 2017

mscdex commented Jul 27, 2017 • edited Loading

lpinca commented Jul 27, 2017

brendanashworth commented Jul 28, 2017

tniessen left a comment

Choose a reason for hiding this comment

brendanashworth commented Jul 29, 2017

MylesBorins commented Aug 16, 2017

brendanashworth commented Aug 16, 2017

MylesBorins commented Sep 19, 2017

mscdex Jul 14, 2017 •

edited

Loading

mscdex Jul 14, 2017 •

edited

Loading

mscdex Jul 15, 2017 •

edited

Loading

brendanashworth commented Jul 18, 2017 •

edited

Loading

mscdex commented Jul 27, 2017 •

edited

Loading