Review requested:

• @nodejs/net

Codecov Report

Attention: Patch coverage is 71.42857% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.10%. Comparing base (e0634f5) to head (1400cbd).
Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main   #54249      +/-   ##
==========================================
- Coverage   87.10%   87.10%   -0.01%     
==========================================
  Files         647      647              
  Lines      181739   181758      +19     
  Branches    34887    34889       +2     
==========================================
+ Hits       158310   158318       +8     
- Misses      16738    16741       +3     
- Partials     6691     6699       +8     

... and 28 files with indirect coverage changes

How is the new API different from
the old one?

How is the new API different from the old one?

if the result of the single lookup is a loopback address:

before:

const response = await dnsPromises.lookup('ipv6_loopback', { verbatim: true });
console.log(response); // { address: 'fe80::1', family: 6 }

util.promisify(dns.lookup)('ipv6_loopback').then(response => {
    console.log(response); // { address: 'fe80::1', family: 6 }
});

after:

const response = await dnsPromises.lookup('ipv6_loopback', { verbatim: true });
console.log(response); // { }

util.promisify(dns.lookup)('ipv6_loopback').then(response => {
    console.log(response); // { }
});

to be honest, considering we are completely filtering out any loopback address in this case, and the result being empty, I would be more in favor of returning a ENOTFOUND , like the address cannot be resolved at all

there's no other consequences. Changing the server listen logic is the cl

Hi @pimterry ! Thanks for the super detailed explaination 😃
I aree with you on pretty much everything, but I have 2 points to make:

1. you said that changing the server.listen is probably not a breaking change, while I think it is, we might have people out there relying on this exact behavior right now, we just don't know
2. if I manage to change server.listen's API (this is my second contribution to Node, I am still getting lost in the humongous codebase) it will not directly solve the reported issue. We actually don't know what the OP wanted to do and he directly reported that dns.lookup was returning a loopback address, we are not sure he needed that to be used with server.listen

All this said, I agree with both your domain analysis and implementation specs 😄
I will start working on it ASAP!

Thanks again 😃

you said that changing the server.listen is probably not a breaking change, while I think it is, we might have people out there relying on this exact behavior right now, we just don't know

Yes, it's not totally clear, it does need some thought & testing.

If server.listen on a link-local address does always fail, then I think fixing this is probably not a breaking change. We don't formally define the hostname preference order anywhere anyway (AFAICT) and I would hope that nobody's code is depending on intentionally starting servers that will intentionally fail to start, but only because the hostname has two IPs but the first one is link-local. In practice, I think anything that currently listens to a hostname like this is broken right now (i.e. the failure is a bug, and shouldn't happen) and so this is just a bugfix.

That's just if listening on link-local addresses does indeed always fail. I haven't intentionally ever used them in practice myself though, so that needs confirmation. If there are practical cases in Node where it does work today, then we can still make the change if it's clearly what most people would expect (for a hostname with a public + link-local IP, I think that's true) but in that case it's definitely a breaking change.

We actually don't know what the OP wanted to do and he directly reported that dns.lookup was returning a loopback address, we are not sure he needed that to be used with server.listen

True - I just don't think that DNS returning that address is necessarily wrong. In some use cases it's weird and won't work, but it's not entirely invalid, and the world of computers contains plenty of weird software that might want to actually use these addresses 😄.

The server.listen case however seems to be the underlying motivation, and that case does seem relatively clear cut - if you specify a hostname to listen on, and it has both a public and a link-local address, then I think you do almost certainly want to listen on the public address (@vbraun feel free to chime in with more context if you have thoughts on this).

@pimterry amazing! thanks for being so thorough with your answers :)

The PR is almost ready, the code is working and new (and old) automated tests are passing, just refining a couple of things and then I will sumbmit it. I will close this one in favor of the other one because the scope (and how it is achieved) is completely different.

Btw, about your

If server.listen on a link-local address does always fail, then I think fixing this is probably not a breaking change.

It looks like it is the case, and this new test confirms it:

closing in favor of #54264