Dgram buffers by DamienOReilly · Pull Request #13623 · nodejs/node · GitHub
Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions doc/api/dgram.md
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
Copy Markdown
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
Copy Markdown
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');

@DamienOReilly DamienOReilly Jun 12, 2017

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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