Skip to content

Commit

Permalink
dgram: handle default address case when offset and length are specified
Browse files Browse the repository at this point in the history
Fixes a regression introduced by: #4374.
Adds a new test to avoid similar issue in the future.
The test is disabled on windows, because this feature never worked
there.

Fixes: #5398
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Evan Lucas <evanlucas@me.com>
  • Loading branch information
mcollina committed Feb 28, 2016
1 parent ffdc046 commit 725ffdb
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 2 deletions.
10 changes: 8 additions & 2 deletions lib/dgram.js
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,13 @@ function enqueue(self, toEnqueue) {
}


// valid combinations
// send(buffer, offset, length, port, address, callback)
// send(buffer, offset, length, port, address)
// send(buffer, offset, length, port)
// send(bufferOrList, port, address, callback)
// send(bufferOrList, port, address)
// send(bufferOrList, port)
Socket.prototype.send = function(buffer,
offset,
length,
Expand All @@ -294,8 +301,7 @@ Socket.prototype.send = function(buffer,
callback) {
var self = this;

// same as arguments.length === 5 || arguments.length === 6
if (address) {
if (address || (port && typeof port !== 'function')) {
buffer = sliceBuffer(buffer, offset, length);
} else {
callback = port;
Expand Down
43 changes: 43 additions & 0 deletions test/parallel/test-dgram-send-default-host.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';

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

if (common.isWindows) {
// on Windows this test will fail
// see https://github.com/nodejs/node/pull/5407
console.log('1..0 # Skipped: This test does not apply on Windows.');
return;
}

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

const timer = setTimeout(function() {
throw new Error('Timeout');
}, common.platformTimeout(2000));

const toSend = [new Buffer(256), new Buffer(256), new Buffer(256), 'hello'];

toSend[0].fill('x');
toSend[1].fill('y');
toSend[2].fill('z');

client.on('listening', function() {
client.send(toSend[0], 0, toSend[0].length, common.PORT);
client.send(toSend[1], common.PORT);
client.send([toSend[2]], common.PORT);
client.send(toSend[3], 0, toSend[3].length, common.PORT);
});

client.on('message', function(buf, info) {
const expected = toSend.shift().toString();
assert.ok(buf.toString() === expected, 'message was received correctly');

if (toSend.length === 0) {
client.close();
clearTimeout(timer);
}
});

client.bind(common.PORT);

6 comments on commit 725ffdb

@MylesBorins
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to have landed without a PR-URL 😭

@jasnell
Copy link
Member

Choose a reason for hiding this comment

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

Do you happen to know the PR?

@MylesBorins
Copy link
Contributor

Choose a reason for hiding this comment

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

@MylesBorins
Copy link
Contributor

Choose a reason for hiding this comment

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

I am now seeing that this is don't land... adjusting audit thread

@mcollina
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sorry :/ My bad. Have you fixed this? May I help anyhow?

@jasnell
Copy link
Member

Choose a reason for hiding this comment

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

@mcollina .. no worries, we've all done it :-) at this point there's nothing to do.

Please sign in to comment.