Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dgram buffers #13623

Closed
wants to merge 13 commits into from
Closed
39 changes: 39 additions & 0 deletions doc/api/dgram.md
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,20 @@ never have reason to call this.
If `multicastInterface` is not specified, the operating system will attempt to
drop membership on all valid interfaces.

### socket.getRecvBufferSize(size)
<!-- YAML
added: REPLACEME
-->

* Returns {number} the `SO_RCVBUF` socket receive buffer size in bytes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


### socket.getSendBufferSize(size)
<!-- YAML
added: REPLACEME
-->

* Returns {number} the `SO_SNDBUF` socket send buffer size in bytes.

### socket.ref()
<!-- YAML
added: v0.9.1
Expand Down Expand Up @@ -398,6 +412,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is a good idea.

added: REPLACEME
-->

* `size` {number} Integer

Sets the `SO_RCVBUF` socket option. Sets the maximum socket receive buffer
in bytes.

### socket.setSendBufferSize(size)
<!-- YAML
added: REPLACEME
-->

* `size` {number} Integer

Sets the `SO_SNDBUF` socket option. Sets the maximum socket send buffer
in bytes.

### socket.setTTL(ttl)
<!-- YAML
added: v0.1.101
Expand Down Expand Up @@ -461,6 +495,9 @@ changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/14560
description: The `lookup` option is supported.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/13623
description: `recvBufferSize` and `sendBufferSize` options are supported now.
-->

* `options` {Object} Available options are:
Expand All @@ -469,6 +506,8 @@ changes:
* `reuseAddr` {boolean} When `true` [`socket.bind()`][] will reuse the
address, even if another process has already bound a socket on it. Optional.
Defaults to `false`.
* `recvBufferSize` {number} - Optional. Sets the `SO_RCVBUF` socket value.
* `sendBufferSize` {number} - Optional. Sets the `SO_SNDBUF` socket value.
* `lookup` {Function} Custom lookup function. Defaults to [`dns.lookup()`][].
Optional.
* `callback` {Function} Attached as a listener for `'message'` events. Optional.
Expand Down
45 changes: 45 additions & 0 deletions lib/dgram.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,15 +98,19 @@ function _createSocketHandle(address, port, addressType, fd, flags) {
return handle;
}

const kOptionSymbol = Symbol('options symbol');

function Socket(type, listener) {
EventEmitter.call(this);
var lookup;

this[kOptionSymbol] = {};
if (type !== null && typeof type === 'object') {
var options = type;
type = options.type;
lookup = options.lookup;
this[kOptionSymbol].recvBufferSize = options.recvBufferSize;
this[kOptionSymbol].sendBufferSize = options.sendBufferSize;
}

var handle = newHandle(type, lookup);
Expand Down Expand Up @@ -141,6 +145,12 @@ function startListening(socket) {
socket._bindState = BIND_STATE_BOUND;
socket.fd = -42; // compatibility hack

if (socket[kOptionSymbol].recvBufferSize)
bufferSize(socket, socket[kOptionSymbol].recvBufferSize, 'recv');

if (socket[kOptionSymbol].sendBufferSize)
bufferSize(socket, socket[kOptionSymbol].sendBufferSize, 'send');

socket.emit('listening');
}

Expand All @@ -157,6 +167,20 @@ function replaceHandle(self, newHandle) {
self._handle = newHandle;
}

function bufferSize(self, size, buffer) {
if (size >>> 0 !== size)
throw new errors.TypeError('ERR_SOCKET_BAD_BUFFER_SIZE');

try {
if (buffer === 'recv')
return self._handle.bufferSize(size, 0);
else
return self._handle.bufferSize(size, 1);
} catch (e) {
throw new errors.Error('ERR_SOCKET_BUFFER_SIZE', e);
}
}

Socket.prototype.bind = function(port_, address_ /*, callback*/) {
let port = port_;

Expand Down Expand Up @@ -636,6 +660,27 @@ Socket.prototype.unref = function() {
return this;
};


Socket.prototype.setRecvBufferSize = function(size) {
bufferSize(this, size, 'recv');
};


Socket.prototype.setSendBufferSize = function(size) {
bufferSize(this, size, 'send');
};


Socket.prototype.getRecvBufferSize = function() {
return bufferSize(this, 0, 'recv');
};


Socket.prototype.getSendBufferSize = function() {
return bufferSize(this, 0, 'send');
};


module.exports = {
_createSocketHandle,
createSocket,
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -245,11 +245,14 @@ E('ERR_SERVER_ALREADY_LISTEN',
'Listen method has been called more than once without closing.');
E('ERR_SOCKET_ALREADY_BOUND', 'Socket is already bound');
E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536');
E('ERR_SOCKET_BAD_BUFFER_SIZE', 'Buffer size must be a positive integer');
E('ERR_SOCKET_BAD_TYPE',
'Bad socket type specified. Valid types are: udp4, udp6');
E('ERR_SOCKET_CANNOT_SEND', 'Unable to send data');
E('ERR_SOCKET_CLOSED', 'Socket is closed');
E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running');
E('ERR_SOCKET_BUFFER_SIZE',
(reason) => `Could not get or set buffer size: ${reason}`);
E('ERR_STDERR_CLOSE', 'process.stderr cannot be closed');
E('ERR_STDOUT_CLOSE', 'process.stdout cannot be closed');
E('ERR_STREAM_WRAP', 'Stream has StringDecoder set or is in objectMode');
Expand Down
39 changes: 39 additions & 0 deletions src/udp_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ using v8::Object;
using v8::PropertyAttribute;
using v8::PropertyCallbackInfo;
using v8::String;
using v8::Uint32;
using v8::Undefined;
using v8::Value;

Expand Down Expand Up @@ -134,6 +135,7 @@ void UDPWrap::Initialize(Local<Object> target,
env->SetProtoMethod(t, "setMulticastLoopback", SetMulticastLoopback);
env->SetProtoMethod(t, "setBroadcast", SetBroadcast);
env->SetProtoMethod(t, "setTTL", SetTTL);
env->SetProtoMethod(t, "bufferSize", BufferSize);

env->SetProtoMethod(t, "ref", HandleWrap::Ref);
env->SetProtoMethod(t, "unref", HandleWrap::Unref);
Expand Down Expand Up @@ -222,6 +224,43 @@ void UDPWrap::Bind6(const FunctionCallbackInfo<Value>& args) {
}


void UDPWrap::BufferSize(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
UDPWrap* wrap;
ASSIGN_OR_RETURN_UNWRAP(&wrap,
args.Holder(),
args.GetReturnValue().Set(UV_EBADF));

CHECK(args[0]->IsUint32());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimothyGu you mean on the JS side, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted, do you recommend:

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');

Copy link
Contributor Author

@DamienOReilly DamienOReilly Jun 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CHECK(args[1]->IsUint32());
int size = static_cast<int>(args[0].As<Uint32>()->Value());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the Uint32 value is very large when it is cast to a signed integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you recommend just passing down a signed int from the JS layer? The libuv layer accepts signed int: https://github.com/nodejs/node/blob/master/deps/uv/src/uv-common.c#L445

I'm actually thinking if we should just let the OS deal with values if they are outside a range it supports. Linux looks to choose a max or min if input is outside a range it supports and not throw an error:
https://github.com/torvalds/linux/blob/8b4822de59d5d9919b9b045183a36c673ce20b73/net/core/sock.c#L752

I cannot find any documentation on what the behavior is on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not trivial since JS numbers are system independent, but C int is system dependent.
IMHO simplest solution is throwing if (size != args[0].As<Uint32>()->Value()) return env->ThrowUVException(FIND_A_GOOD_ERRNO, "uv_recv_buffer_size");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EINVAL should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in, throw Exception with error code EINVAL ?


if (size != args[0].As<Uint32>()->Value()) {
if (args[1].As<Uint32>()->Value() == 0)
return env->ThrowUVException(EINVAL, "uv_recv_buffer_size");
else
return env->ThrowUVException(EINVAL, "uv_send_buffer_size");
}

int err;
if (args[1].As<Uint32>()->Value() == 0) {
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_),
&size);
}

if (err != 0) {
if (args[1].As<Uint32>()->Value() == 0)
return env->ThrowUVException(err, "uv_recv_buffer_size");
else
return env->ThrowUVException(err, "uv_send_buffer_size");
}
args.GetReturnValue().Set(size);
}


#define X(name, fn) \
void UDPWrap::name(const FunctionCallbackInfo<Value>& args) { \
UDPWrap* wrap = Unwrap<UDPWrap>(args.Holder()); \
Expand Down
1 change: 1 addition & 0 deletions src/udp_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ class UDPWrap: public HandleWrap {
const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetBroadcast(const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetTTL(const v8::FunctionCallbackInfo<v8::Value>& args);
static void BufferSize(const v8::FunctionCallbackInfo<v8::Value>& args);

static v8::Local<v8::Object> Instantiate(Environment* env, AsyncWrap* parent);
uv_udp_t* UVHandle();
Expand Down
20 changes: 20 additions & 0 deletions test/parallel/test-dgram-createSocket-type.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,23 @@ validTypes.forEach((validType) => {
socket.close();
});
});

// Ensure buffer sizes can be set
{
const socket = dgram.createSocket({
type: 'udp4',
recvBufferSize: 10000,
sendBufferSize: 15000
});

socket.bind(common.mustCall(() => {
// note: linux will double the buffer size
assert.ok(socket.getRecvBufferSize() === 10000 ||
socket.getRecvBufferSize() === 20000,
'SO_RCVBUF not 1300 or 2600');
assert.ok(socket.getSendBufferSize() === 15000 ||
socket.getSendBufferSize() === 30000,
'SO_SNDBUF not 1800 or 3600');
socket.close();
}));
}
74 changes: 74 additions & 0 deletions test/parallel/test-dgram-socket-buffer-size.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const dgram = require('dgram');

{
// Should throw error if the socket is never bound.
const errorObj = {
code: 'ERR_SOCKET_BUFFER_SIZE',
type: Error,
message: /^Could not get or set buffer size:.*$/
};

const socket = dgram.createSocket('udp4');

assert.throws(() => {
socket.setRecvBufferSize(8192);
}, common.expectsError(errorObj));

assert.throws(() => {
socket.setSendBufferSize(8192);
}, common.expectsError(errorObj));

assert.throws(() => {
socket.getRecvBufferSize();
}, common.expectsError(errorObj));

assert.throws(() => {
socket.getSendBufferSize();
}, common.expectsError(errorObj));
}

{
// Should throw error if invalid buffer size is specified
const errorObj = {
code: 'ERR_SOCKET_BAD_BUFFER_SIZE',
type: TypeError,
message: /^Buffer size must be a positive integer$/
};

const badBufferSizes = [-1, Infinity, 'Doh!'];

const socket = dgram.createSocket('udp4');

socket.bind(common.mustCall(() => {
badBufferSizes.forEach((badBufferSize) => {
assert.throws(() => {
socket.setRecvBufferSize(badBufferSize);
}, common.expectsError(errorObj));

assert.throws(() => {
socket.setSendBufferSize(badBufferSize);
}, common.expectsError(errorObj));
});
socket.close();
}));
}

{
// Can set and get buffer sizes after binding the socket.
const socket = dgram.createSocket('udp4');

socket.bind(common.mustCall(() => {
socket.setRecvBufferSize(10000);
socket.setSendBufferSize(10000);

// note: linux will double the buffer size
const expectedBufferSize = common.isLinux ? 20000 : 10000;
assert.strictEqual(socket.getRecvBufferSize(), expectedBufferSize);
assert.strictEqual(socket.getSendBufferSize(), expectedBufferSize);
socket.close();
}));
}