UDP/dgram performs unnecessary DNS lookups

[schmod]

Version

14.x

Platform

All

Subsystem

dgram

What steps will reproduce the bug?

It appears that UDP/dgram connections perform unnecessary DNS lookups when an IP address is provided as the address to connect to.

The issue is easily demonstrated via this snippet:

const dns = require("dns");

const _lookup = dns.lookup;
dns.lookup = function (...args) {
  console.log("Called lookup", args);
  return _lookup(...arguments);
};

const dgram = require("dgram");

const socket = dgram.createSocket({ type: "udp4" });
console.log("Socket created");

socket.connect(1234, "192.168.1.1", (err) => {
  console.log("Socket connected", { err });
});

Output:

$ node index.js

Socket created
Called lookup [ '0.0.0.0', 4, [Function (anonymous)] ]
Called lookup [ '192.168.1.1', 4, [Function: afterDns] ]
Socket connected { err: undefined }

How often does it reproduce? Is there a required condition?

These lookups appear to happen 100% of the time a UDP socket connects to an address that is a raw IP address.

What is the expected behavior?

DNS lookups only occur when the address is a hostname.

What do you see instead?

No response

Additional information

Note that the net module contains code that bypasses dns.lookup() when the target address is a valid IP address. It seems like we could improve efficiency by adding a similar check to dgram without breaking compatibility.

[renatomariscal]

I suspect there is a misconception here. The dns.lookup is itself the place where NodeJS has implemented the code to prevent actual DNS lookup on IP address:

node/lib/dns.js

Lines 155 to 164 in b3723fa

	const matchedFamily = isIP(hostname);
	if (matchedFamily) {
	if (all) {
	process.nextTick(
	callback, null, [{ address: hostname, family: matchedFamily }]);
	} else {
	process.nextTick(callback, null, hostname, matchedFamily);
	}
	return {};
	}

So, the fact that dns.lookup is called doesn't imply on IO.

[renatomariscal]

Trying to come up with a test, the easiest I could find is to get an invalid DNS server, and if it is true that for ip it does lookup, it would fail the same way it fails for a non-ip name.

NodeJS dns.setServers explicitly don't work for dns.lookup, and there is no alternative.

We can use docker with an invalid DNS server:

docker run --dns 127.0.0.1 -it --rm node:12.18.4

Test 1 - call a non-ip name:

Calling foobar:

require("dgram").
    createSocket({ type: "udp4" }).
    connect(1234, "foobar", (err) => {
        console.log("Socket connected", { err });
    });

Result

Socket connected {
  err: Error: getaddrinfo EAI_AGAIN foobar
      at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:66:26)
      at GetAddrInfoReqWrap.callbackTrampoline (internal/async_hooks.js:120:14) {
    errno: 'EAI_AGAIN',
    code: 'EAI_AGAIN',
    syscall: 'getaddrinfo',
    hostname: 'foobar'
  }
}

Test 2 - call an ip address:

Calling 192.168.0.1:

require("dgram").
    createSocket({ type: "udp4" }).
    connect(1234, "192.168.0.1", (err) => {
        console.log("Socket connected", { err });
    });

Result

 Socket connected { err: undefined }

---

So, I am quite convinced it is working properly.

[schmod]

To test conclusively, I think you would want to actually run a real DNS server with logging, just in case there's something going on in the Docker image and/or c-ares. However, having looked at the code, I do think that you might be right.

That being said, I think we should still move forward with the change proposed in this ticket.

• There is still a (minor) performance benefit
• It would improve consistency between dgram and the net module, and create a more consistent contract for custom resolver functions (ie. for both TCP and UDP, Node will avoid calling the DNS resolver when there's nothing to resolve)
• It would greatly reduce the number of real-world cases where dns.lookup ends up being a no-op. Many folks (including myself) instrument dns.lookup to monitor performance, and are getting false signals because of the way that drgram invokes it.