dgram: skip dns.lookup() for literal IP addresses by BridgeAR · Pull Request #64133 · nodejs/node · GitHub
Skip to content
Open
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
43 changes: 43 additions & 0 deletions benchmark/dgram/send-to-ip.js
2 changes: 2 additions & 0 deletions doc/api/dgram.md
Original file line number Diff line number Diff line change
Expand Up @@ -1039,6 +1039,8 @@ changes:
* `recvBufferSize` {number} Sets the `SO_RCVBUF` socket value.
* `sendBufferSize` {number} Sets the `SO_SNDBUF` socket value.
* `lookup` {Function} Custom lookup function. **Default:** [`dns.lookup()`][].
When the default is used, a literal IP address of the socket's family

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.

Why only when the default is used? Is it because the previous behaviour would always call custom function, and not doing so would be breaking?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have a stacked PR that does it in general, I was just concerned about it being major if we do it for custom lookup methods, so I decided to split it. I am fine to have it as one PR.

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.

Let's leave it as is to keep it non-major for this PR 👍 .

resolves to itself without calling [`dns.lookup()`][].
* `signal` {AbortSignal} An AbortSignal that may be used to close a socket.
* `receiveBlockList` {net.BlockList} `receiveBlockList` can be used for discarding
inbound datagram to specific IP addresses, IP ranges, or IP subnets. This does not
Expand Down
20 changes: 15 additions & 5 deletions lib/internal/dgram.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const {
const { codes: {
ERR_SOCKET_BAD_TYPE,
} } = require('internal/errors');
const { isIP } = require('internal/net');
const { UDP } = internalBinding('udp_wrap');
const { guessHandleType } = require('internal/util');
const {
Expand All @@ -28,13 +29,22 @@ function lookup6(lookup, address, callback) {
return lookup(address || '::1', 6, callback);
}

// A literal IP of the socket's family resolves to itself, so skip dns.lookup().
// Defer with nextTick to keep the callback async (e.g. bind()'s 'listening').
function defaultLookup(address, family, callback) {
if (isIP(address) === family) {
process.nextTick(callback, null, address, family);
return;
}
if (dns === undefined) {
dns = require('dns');
}
return dns.lookup(address, family, callback);
}

function newHandle(type, lookup) {
if (lookup === undefined) {
if (dns === undefined) {
dns = require('dns');
}

lookup = dns.lookup;
lookup = defaultLookup;
} else {
validateFunction(lookup, 'lookup');
}
Expand Down
14 changes: 8 additions & 6 deletions test/parallel/test-dgram-custom-lookup.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ const assert = require('assert');
const dgram = require('dgram');
const dns = require('dns');

const originalLookup = dns.lookup;

{
// Verify that the provided lookup function is called.
const lookup = common.mustCall((host, family, callback) => {
dns.lookup(host, family, callback);
originalLookup(host, family, callback);
});

const socket = dgram.createSocket({ type: 'udp4', lookup });
Expand All @@ -18,17 +20,17 @@ const dns = require('dns');
}

{
// Verify that lookup defaults to dns.lookup().
const originalLookup = dns.lookup;

// Verify that the default lookup forwards host names to dns.lookup().
dns.lookup = common.mustCall((host, family, callback) => {
dns.lookup = originalLookup;
originalLookup(host, family, callback);
assert.strictEqual(host, 'example.invalid');
assert.strictEqual(family, 4);
callback(null, '127.0.0.1', 4);
});

const socket = dgram.createSocket({ type: 'udp4' });

socket.bind(common.mustCall(() => {
socket.bind(0, 'example.invalid', common.mustCall(() => {
socket.close();
}));
}
Expand Down
80 changes: 80 additions & 0 deletions test/parallel/test-dgram-default-lookup-ip.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
'use strict';

// The default dgram lookup resolves a literal IP address of the socket's own
// family to itself, without calling dns.lookup(). Each case below stubs the
// process-global dns.lookup(), so they run sequentially to keep one case from
// observing another's stub.

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

const originalLookup = dns.lookup;

function ipv4SendSkipsLookup(next) {
dns.lookup = common.mustNotCall('dns.lookup() ran for an IPv4 literal');

const receiver = dgram.createSocket('udp4');
const sender = dgram.createSocket('udp4');

receiver.on('message', common.mustCall((msg) => {
assert.strictEqual(msg.toString(), 'payload');
dns.lookup = originalLookup;
receiver.close();
sender.close();
next();
}));

receiver.bind(0, '127.0.0.1', common.mustCall(() => {
sender.send('payload', receiver.address().port, '127.0.0.1', common.mustCall());
}));
}

function ipv6BindSkipsLookup(next) {
if (!common.hasIPv6) {
next();
return;
}

dns.lookup = common.mustNotCall('dns.lookup() ran for an IPv6 literal');

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

socket.bind(0, '::1', common.mustCall(() => {
dns.lookup = originalLookup;
socket.close();
next();
}));
}

function mismatchedFamilyFallsThrough(next) {
// '::1' is not an IPv4 literal, so a udp4 socket still resolves it via
// dns.lookup() rather than short-circuiting.
dns.lookup = common.mustCall((host, family, callback) => {
dns.lookup = originalLookup;
assert.strictEqual(host, '::1');
assert.strictEqual(family, 4);
callback(null, '127.0.0.1', 4);
});

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

socket.bind(0, '::1', common.mustCall(() => {
socket.close();
next();
}));
}

const cases = [
ipv4SendSkipsLookup,
ipv6BindSkipsLookup,
mismatchedFamilyFallsThrough,
];

(function runNext() {
const testCase = cases.shift();
if (testCase !== undefined) {
testCase(runNext);
}
})();
13 changes: 7 additions & 6 deletions test/sequential/test-dgram-implicit-bind-failure.js
Loading