Dgram buffers #13623

DamienOReilly · 2017-06-11T22:59:28Z

Add support to allow the dgram lib to set the SO_SNDBUF & SO_RCVBUF socket buffer sizes.

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)

doc, dgram

refack · 2017-06-11T23:37:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8605/

refack · 2017-06-11T23:44:05Z

Quick CI: https://ci.nodejs.org/job/node-test-commit-linuxone/6560/

refack

LGTM % Some nits

refack · 2017-06-11T23:40:21Z

doc/api/dgram.md

@@ -396,6 +396,26 @@ decremented to 0 by a router, it will not be forwarded.
 The argument passed to to `socket.setMulticastTTL()` is a number of hops
 between 0 and 255. The default on most systems is `1` but can vary.

+### socket.setRecvBufferSize(size)
+<!-- YAML
+added: vx.x.x


put REPLACEME we have a macro that fills it it

refack · 2017-06-11T23:40:30Z

doc/api/dgram.md

+
+### socket.setSendBufferSize(size)
+<!-- YAML
+added: vx.x.x


refack · 2017-06-11T23:52:05Z

CI sais 🔴
Error code is probably OS dependent, not sure what's the policy, probably you should use a regex like /^Error: setRecvBufferSize [A-Z]+^/

339	parallel/test-dgram-set-socket-buffer-size	
duration_ms	0.64
severity	fail
stack	
assert.js:569
    throw actual;
    ^

Error: setRecvBufferSize EBADF
    at exports._errnoException (util.js:1014:11)
    at Socket.setRecvBufferSize (dgram.js:647:11)
    at assert.throws (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-dgram-set-socket-buffer-size.js:12:12)
    at _tryBlock (assert.js:526:5)
    at _throws (assert.js:547:12)
    at Function.throws (assert.js:577:3)
    at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-dgram-set-socket-buffer-size.js:11:10)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)

Trott · 2017-06-12T00:08:03Z

/^Error: setRecvBufferSize [A-Z]+^/

...or even: /^Error: setRecvBufferSize E[A-Z]+^/

(Right? It will always start with E?)

TimothyGu · 2017-06-12T00:39:19Z

src/udp_wrap.cc

+                          args.Holder(),
+                          args.GetReturnValue().Set(UV_EBADF));
+
+  CHECK(args[0]->IsUint32());


We probably don't want to crash when the user does socket.setRecvBufferSize(Infinity). Throwing a TypeError should be a better choice.

@TimothyGu you mean on the JS side, right?

Noted, do you recommend:

if (!args[0]->IsUint32()) { return env->ThrowTypeError("size must be an uint32"); }

Recommend doing the type check in JS. Just something simple like:

const errors = require('internal/errors'); // then within the the functions if (typeof size !== 'number') throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'size', 'number');

Thanks for your input. Do you think I need to do any safety checking in udp_wrap.cc SetRecvBufferSize() or we assume only dgram.js will interact with udp_wrap ?

Yes, once the type checking is added to the js side, I would keep the CHECK in place on the native side. That should only be hit if we do something wrong in core or if the user is doing something they really shouldn't.

TimothyGu · 2017-06-12T00:42:59Z

src/udp_wrap.cc

+
+  CHECK(args[0]->IsUint32());
+
+  int inputSize = args[0]->Uint32Value();


Uint32Value without v8::Context specified is deprecated. Use args[0]->Uint32Value(info.GetIsolate()->GetCurrentContext()).

I think we generally prefer camelCase for JavaScript and snake case for C++.

Yes, will see to this.

Use args[0]->Uint32Value(info.GetIsolate()->GetCurrentContext()).

That’s not necessary if you already checked args[0]->IsUint32(),args[0].As<Uint32>()->Value() should do instead.

jasnell · 2017-06-12T14:57:46Z

lib/dgram.js

+  var err = this._handle.setRecvBufferSize(size);
+
+  if (err) {
+    throw errnoException(err, 'setRecvBufferSize');


Would prefer using internal/errors here since this introduces new errors. The errno can be added.

@DamienOReilly add the new error code at https://github.com/nodejs/node/blob/master/lib/internal/errors.js#L164 (you can put placeholders in the message like in https://github.com/nodejs/node/blob/master/lib/internal/errors.js#L121

jasnell · 2017-06-12T15:00:13Z

lib/dgram.js

+
+
+Socket.prototype.setSendBufferSize = function(size) {
+  var err = this._handle.setSendBufferSize(size);


Because _handle.setSendBufferSize() includes a CHECK that will abort if size is not UInt32, these should do a proper type check in the js side. Otherwise users will not be happy.

I think we'd prefer a check in JS that will throw an internal/errors.TypeError
this is a good check Number.isFinite(num) && num <= Number.MAX_SAFE_INTEGER && num >= 0;
Ref: https://github.com/nodejs/node/blob/master/lib/internal/process.js#L31
Ref: https://github.com/nodejs/node/blob/master/lib/internal/process.js#L62

this is a good check Number.isFinite(num) && num <= Number.MAX_SAFE_INTEGER && num >= 0;

It’s not an uint32 check, though. I think num >>> 0 === num would work.

Wow, completely forgot about the existence of >>>
So much fun working with talented people 🕺

jasnell · 2017-06-12T15:01:45Z

doc/api/dgram.md

@@ -396,6 +396,26 @@ decremented to 0 by a router, it will not be forwarded.
 The argument passed to to `socket.setMulticastTTL()` is a number of hops
 between 0 and 255. The default on most systems is `1` but can vary.

+### socket.setRecvBufferSize(size)
+<!-- YAML


Would it make sense to also include these as options when the Socket is created?

Yes this is a good idea.

cjihrig · 2017-06-12T15:04:39Z

src/udp_wrap.cc

+
+  CHECK(args[0]->IsUint32());
+
+  int inputSize = args[0]->Uint32Value();


I think we generally prefer camelCase for JavaScript and snake case for C++.

cjihrig · 2017-06-12T16:32:48Z

lib/dgram.js

+Socket.prototype.setRecvBufferSize = function(size) {
+  var err = this._handle.setRecvBufferSize(size);
+
+  if (err) {


Style nit: Core prefers no curly braces on single line ifs.

cjihrig · 2017-06-12T20:57:55Z

doc/api/dgram.md

@@ -454,10 +474,14 @@ s.bind(1234, () => {

 ### dgram.createSocket(options[, callback])
 <!-- YAML
-added: v0.11.13
+added: REPLACEME


You shouldn't change the version this was added. Each function does support it's own changelog though. See the child_process.spawn() docs for example.

Perfect, thanks.

cjihrig · 2017-06-12T21:01:56Z

lib/dgram.js

+  return Number.isFinite(size) &&
+          size <= Number.MAX_SAFE_INTEGER &&
+          size >= 0 &&
+          size >>> 0 === size;


This function seems like a bit of overkill for detecting an unsigned int.

I think the last condition implies all the others, so it is sufficient.

cjihrig · 2017-06-12T21:03:37Z

lib/dgram.js

@@ -121,6 +122,12 @@ function Socket(type, listener) {

  // If true - UV_UDP_REUSEADDR flag will be set
  this._reuseAddr = options && options.reuseAddr;
+
+  if (options && options.recvBufferSize)


If we're adding more options, it might be better to just make sure options is an object. Adding these underscored properties also means we have to maintain them indefinitely.

Technically, we could use the libuv calls to with value === 0 to get the current value.

Also, it looks like these underscored properties aren't used in setRecvBufferSize() and setSendBufferSize().

@DamienOReilly tl;dr, if you don't need them later, no need to store them.

They are used in startListening() which is called after the socket has been bound. They will in turn determine if setRecvBufferSize() and setSendBufferSize() are called if the specific options have been set against the Socket created with dgram.createSocket(). Otherwise setRecvBufferSize() and setSendBufferSize() can be called on the Socket object.

If we have to store these, I'd say use symbols. It's kind of ugly though because if the values are updated later, these values aren't kept in sync. And, libuv provides APIs to get those values.

@DamienOReilly example Symbol use https://github.com/DamienOReilly/node/blob/master/lib/net.js#L176

@cjihrig I guess extending this PR to add functionality to the interface in JS layer to use getsockopt down in libuv might be a good idea (e.g. getRecvBufferSize()). Therefore the symbol values holding initial options can be ignored after they are set against the socket when the listening event triggers.

cjihrig · 2017-06-12T21:04:12Z

lib/dgram.js

@@ -140,6 +147,12 @@ function startListening(socket) {
  socket._receiving = true;
  socket._bindState = BIND_STATE_BOUND;
  socket.fd = -42; // compatibility hack
+
+  if (socket._recvBufferSize)


Any reason these ifs can't be combined with the other ones?

The buffer sizes can only be set after the socket has been created.

Ah, right, sorry. I was reading the diff as if the changes were in the same function. Might be a good case for #5496 though.

No problem. Might look at that next!

cjihrig · 2017-06-12T21:06:12Z

test/parallel/test-dgram-set-socket-buffer-size.js

+    type: Error,
+    message: /^Coud not set buffer size: E[A-Z]+$/
+  }));
+


Can you remove these extra blank lines.

refack

Getting close 👍

refack · 2017-06-12T21:16:20Z

lib/dgram.js

@@ -121,6 +122,12 @@ function Socket(type, listener) {

  // If true - UV_UDP_REUSEADDR flag will be set
  this._reuseAddr = options && options.reuseAddr;
+
+  if (options && options.recvBufferSize)


@DamienOReilly tl;dr, if you don't need them later, no need to store them.

refack · 2017-06-12T21:17:29Z

lib/dgram.js

+  var err = this._handle.setSendBufferSize(size);
+
+  if (err)
+    throw new errors.TypeError('ERR_SOCKET_SET_BUFFER_SIZE', 


INHO this should be a regular error.Error (same signature)

cjihrig · 2017-06-14T15:44:38Z

lib/dgram.js

@@ -326,6 +350,11 @@ function enqueue(self, toEnqueue) {
 }


+function isValidSize(size) {


We can just inline this in the only place it's used.

Sure thing. It makes sense.

cjihrig · 2017-06-14T15:46:26Z

doc/api/dgram.md

+added: REPLACEME
+-->
+
+* Returns {number} the `SO_RCVBUF` socket receive buffer size in bytes.


Just a thought, but we might not need to mention SO_RCVBUF and SO_SNDBUF at all. They aren't really needed to work with the API.

They are not needed, but I though it was no harm having them mentioned in the docs. I'd like to get a few people's opinion on this.

I see that googling SO_RCVBUF gives very good results (linux man and MSDN) so I'm +0.5

You can always add something like (see socket(7)) if you like, the doctool will expand that into a link.

Just for the record, I'm not opposed to including them. I just thought we might be able to get by without them. I'm open to whatever everyone thinks is best here.

Oh I know, the only reason I included them was that I was familiar with setting the buffer sizes with the JDK, and the javadocs mentioned these property names. When I started working on a small app to get more familiar with javascript, I was goggle'in around "node.js SO_RCVBUF" to see if anyone had already added or was in the process of adding buffer size setting support in node.js.

cjihrig · 2017-06-14T15:49:18Z

src/udp_wrap.cc

+  CHECK(args[1]->IsUint32());
+  int size = (int)args[0].As<Uint32>()->Value();
+
+  int err = 0;


You don't really need to initialize this to zero. It will always be set by one branch of the following if statement.

cjihrig · 2017-06-14T15:49:49Z

src/udp_wrap.cc

+  int err = 0;
+  if (args[1].As<Uint32>()->Value() == 0)
+    err = uv_recv_buffer_size(reinterpret_cast<uv_handle_t*>(&wrap->handle_),
+                                &size);


Can you line up &size with reinterpret_cast.

cjihrig · 2017-06-14T15:50:00Z

src/udp_wrap.cc

+    err = uv_recv_buffer_size(reinterpret_cast<uv_handle_t*>(&wrap->handle_),
+                                &size);
+  else
+    err = uv_send_buffer_size(reinterpret_cast<uv_handle_t*>(&wrap->handle_),


Same comment here about alignment.

cjihrig · 2017-06-14T15:54:00Z

src/udp_wrap.cc

+  if (err != 0) {
+    std::string syscall;
+    if (args[1].As<Uint32>()->Value() == 0)
+      syscall = "uv_recv_buffer_size";


Maybe drop syscall and just duplicate the env->ThrowUVException() call.

Hehe yes, I originally had it this way but the duplication itched at me!

cjihrig · 2017-06-14T15:54:54Z

test/parallel/test-dgram-createSocket-type.js

+
+// Ensure buffer sizes can be set
+{
+  const socket = dgram.createSocket({ type: 'udp4',


I'm a little surprised the linter doesn't complain about this. Can you move type to the next line with two spaces indentation.

cjihrig · 2017-06-14T15:57:21Z

test/parallel/test-dgram-socket-buffer-size.js

+  // Should throw error if the socket is never bound.
+  const socket = dgram.createSocket('udp4');
+
+  assert.throws(() => {


The duplication here could probably be reduced.

cjihrig · 2017-06-14T15:58:27Z

test/parallel/test-dgram-socket-buffer-size.js

+      socket.setRecvBufferSize(-1);
+    }, common.expectsError({
+      code: 'ERR_SOCKET_BAD_BUFFER_SIZE',
+      type: Error,


Shouldn't this be a TypeError?

Yes it should, I am throwing TypeError in dgram.js, however the test is passing. Regardless I will update the test.

common.expectsError() is subtly broken. It relies on instanceof to check the error type. So a TypeError is an instance of Error, which is why this passed. The simplest fix might be to check error.constructor inside of common.expectsError(). But that will require updating a bunch of tests because we're now using custom errors all over the place.

cc: @nodejs/testing

common.expectsError() is subtly broken

Ref: #13682

cjihrig · 2017-06-14T15:59:29Z

test/parallel/test-dgram-socket-buffer-size.js

+  const socket = dgram.createSocket('udp4');
+
+  socket.bind(common.mustCall(() => {
+    assert.throws(() => {


Same comment about duplication here.

refack

I like 💯

refack · 2017-06-14T22:45:43Z

doc/api/dgram.md

+added: REPLACEME
+-->
+
+* Returns {number} the `SO_RCVBUF` socket receive buffer size in bytes.


I see that googling SO_RCVBUF gives very good results (linux man and MSDN) so I'm +0.5

DamienOReilly · 2017-06-20T09:32:31Z

Hi all, are there any outstanding comments you need addressed in this PR?

Trott · 2017-06-20T12:01:58Z

Hi all, are there any outstanding comments you need addressed in this PR?

@cjihrig You currently have the blocking red X on this one...

Trott · 2017-06-20T12:04:15Z

Two things:

We'll probably want to benchmark this change before landing.
lib/dgram.js currently has 100% test coverage. We should probably run the coverage tool on these changes to make sure there aren't untested code paths with this change.

Will break YAML parsing! Original Node error: ``` Jonathans-MBP:node jon$ node tools/doc/generate.js --format=json doc/api/dgram.md Input file = doc/api/dgram.md { Error at generateError (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:165:10) at throwError (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:171:9) at readBlockSequence (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:935:7) at composeNode (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1331:12) at readBlockMapping (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1062:11) at composeNode (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1332:12) at readDocument (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1492:3) at loadDocuments (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1548:5) at load (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1569:19) at Object.safeLoad (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1591:10) name: 'YAMLException', reason: 'bad indentation of a sequence entry', mark: Mark { name: null, buffer: '\nadded: v0.11.13\nchanges:\n - version: REPLACEME\n pr-url: nodejs#14560 description: The `lookup` option is supported.\n - version: REPLACEME\n pr-url: nodejs#13623 description: `recvBufferSize` and `sendBufferSize` options are supported now.\n\u0000', position: 248, line: 8, column: 17 }, message: 'bad indentation of a sequence entry at line 9, column 18:\n description: `recvBufferSize` and `sendBuffer ... \n ^' } ``` Then I extracted out the problematic YAML, and tried running through Ruby as a separate `.yml` file: ``` Psych::SyntaxError: (<unknown>): found character that cannot start any token while scanning for the next token at line 8 column 18 from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:377:in `parse' from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:377:in `parse_stream' from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:325:in `parse' from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:252:in `load' from (irb):7 from /Users/jon/.rbenv/versions/2.4.0/bin/irb:11:in `<main>' ```

jasnell · 2017-09-20T15:45:50Z

this would need to be backported for v8.x

jasnell · 2017-09-20T17:10:44Z

This is not landing cleanly in v8.x because #14560 has not yet been backported.

jasnell · 2017-09-20T17:16:14Z

#15402 needs to be backported along with this.

* setRecvBufferSize(int) and setSendBufferSize(int) * added docs for send/receive buffer sizes * Added options support to set buffer sizes in dgram.createSocket(). PR-URL: nodejs/node#13623 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This commit refactors the get/set send/receive buffer size methods in the following ways: - Use booleans instead of strings and numbers to differentiate between the send and receive buffers. - Reduce the amount of branching and complexity in the C++ code. Refs: nodejs#13623 PR-URL: nodejs#15483 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

This commit refactors the get/set send/receive buffer size methods in the following ways: - Use booleans instead of strings and numbers to differentiate between the send and receive buffers. - Reduce the amount of branching and complexity in the C++ code. Refs: nodejs/node#13623 PR-URL: nodejs/node#15483 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>