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: improve "address" parameter behavior in Socket.prototype.send #10473

Closed
wants to merge 2 commits into from
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
14 changes: 8 additions & 6 deletions doc/api/dgram.md
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ never have reason to call this.
If `multicastInterface` is not specified, the operating system will attempt to
drop membership on all valid interfaces.

### socket.send(msg, [offset, length,] port, address[, callback])
### socket.send(msg, [offset, length,] port [, address] [, callback])
<!-- YAML
added: v0.1.99
-->
Expand All @@ -234,7 +234,7 @@ added: v0.1.99
* `offset` {Number} Integer. Optional. Offset in the buffer where the message starts.
* `length` {Number} Integer. Optional. Number of bytes in the message.
* `port` {Number} Integer. Destination port.
* `address` {String} Destination hostname or IP address.
* `address` {String} Destination hostname or IP address. Optional.
* `callback` {Function} Called when the message has been sent. Optional.

Broadcasts a datagram on the socket. The destination `port` and `address` must
Expand All @@ -251,8 +251,9 @@ respect to [byte length][] and not the character position.
If `msg` is an array, `offset` and `length` must not be specified.

The `address` argument is a string. If the value of `address` is a host name,
DNS will be used to resolve the address of the host. If the `address` is not
specified or is an empty string, `'127.0.0.1'` or `'::1'` will be used instead.
DNS will be used to resolve the address of the host. If `address` is not
provided or otherwise falsy, `'127.0.0.1'` (for `udp4` sockets) or `'::1'`
(for `udp6` sockets) will be used by default.

If the socket has not been previously bound with a call to `bind`, the socket
is assigned a random port number and is bound to the "all interfaces" address
Expand Down Expand Up @@ -283,14 +284,15 @@ client.send(message, 41234, 'localhost', (err) => {
});
```

Example of sending a UDP packet composed of multiple buffers to a random port on `localhost`;
Example of sending a UDP packet composed of multiple buffers to a random port
on `127.0.0.1`;

```js
const dgram = require('dgram');
const buf1 = Buffer.from('Some ');
const buf2 = Buffer.from('bytes');
const client = dgram.createSocket('udp4');
client.send([buf1, buf2], 41234, 'localhost', (err) => {
client.send([buf1, buf2], 41234, (err) => {
client.close();
});
```
Expand Down
10 changes: 10 additions & 0 deletions lib/dgram.js
Original file line number Diff line number Diff line change
Expand Up @@ -314,9 +314,11 @@ function clearQueue() {
// valid combinations
// send(buffer, offset, length, port, address, callback)
// send(buffer, offset, length, port, address)
// send(buffer, offset, length, port, callback)
// send(buffer, offset, length, port)
// send(bufferOrList, port, address, callback)
// send(bufferOrList, port, address)
// send(bufferOrList, port, callback)
// send(bufferOrList, port)
Socket.prototype.send = function(buffer,
offset,
Expand Down Expand Up @@ -355,6 +357,14 @@ Socket.prototype.send = function(buffer,
if (typeof callback !== 'function')
callback = undefined;

if (typeof address === 'function') {
callback = address;
address = undefined;
} else if (address && typeof address !== 'string') {
throw new TypeError('Invalid arguments: address must be a nonempty ' +
'string or falsy');
}

this._healthCheck();

if (this._bindState === BIND_STATE_UNBOUND)
Expand Down
51 changes: 51 additions & 0 deletions test/parallel/test-dgram-send-address-types.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const dgram = require('dgram');

const port = common.PORT;
const buf = Buffer.from('test');
const client = dgram.createSocket('udp4');

const onMessage = common.mustCall((err, bytes) => {
assert.ifError(err);
assert.strictEqual(bytes, buf.length);
}, 6);

// valid address: false
client.send(buf, port, false, onMessage);

// valid address: empty string
client.send(buf, port, '', onMessage);

// valid address: null
client.send(buf, port, null, onMessage);

// valid address: 0
client.send(buf, port, 0, onMessage);

// valid address: undefined
client.send(buf, port, undefined, onMessage);

// valid address: not provided
client.send(buf, port, onMessage);

const expectedError = new RegExp('^TypeError: Invalid arguments: address ' +
'must be a nonempty string or falsy$');

// invalid address: object
assert.throws(() => {
client.send(buf, port, []);
}, expectedError);

// invalid address: nonzero number
assert.throws(() => {
client.send(buf, port, 1);
}, expectedError);

// invalid address: true
assert.throws(() => {
client.send(buf, port, true);
}, expectedError);

client.unref();
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed this. Is there any chance that the socket would be unref'ed and the process exit before all six messages are received. I am able to make the test fail artificially by adding a short timeout around the last successful send. It doesn't seem to be a problem on the CI, but wouldn't want it to be a source of future flakiness either.

17 changes: 17 additions & 0 deletions test/parallel/test-dgram-send-callback-buffer-empty-address.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';

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

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

const buf = Buffer.alloc(256, 'x');

const onMessage = common.mustCall(function(err, bytes) {
assert.ifError(err);
assert.strictEqual(bytes, buf.length);
client.close();
});

client.send(buf, common.PORT, onMessage);
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use strict';

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

const dgram = require('dgram');
const client = dgram.createSocket('udp4');

const buf = Buffer.alloc(256, 'x');
const offset = 20;
const len = buf.length - offset;

const onMessage = common.mustCall(function messageSent(err, bytes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

err has to be asserted first.

Copy link
Contributor Author

@boneskull boneskull Feb 14, 2017

Choose a reason for hiding this comment

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

FWIW, these two files are copypasta from test-dgram-send-callback-buffer-length (etc) (some of) which have the same issues.

assert.ifError(err);
assert.notStrictEqual(bytes, buf.length);
assert.strictEqual(bytes, buf.length - offset);
client.close();
});

client.send(buf, offset, len, common.PORT, onMessage);
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
'use strict';

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

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

const messageSent = common.mustCall(function messageSent(err, bytes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

assert.ifError(err)

assert.ifError(err);
assert.strictEqual(bytes, buf1.length + buf2.length);
});

const buf1 = Buffer.alloc(256, 'x');
const buf2 = Buffer.alloc(256, 'y');

client.on('listening', function() {
const port = this.address().port;
client.send([buf1, buf2], port, messageSent);
});

client.on('message', common.mustCall(function onMessage(buf) {
const expected = Buffer.concat([buf1, buf2]);
assert.ok(buf.equals(expected), 'message was received correctly');
client.close();
}));

client.bind(0);