nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.46k stars 29.01k forks source link

v18.x - test-dns-ipv6 fails in 18.17.0 and later #49203

Closed mhdawson closed 11 months ago

mhdawson commented 1 year ago

Version

v18.17.0

Platform

at least linux

Subsystem

net

What steps will reproduce the bug?

tools/test.py test/internet/test-dns-ipv6

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

100%

What is the expected behavior? Why is that the expected behavior?

Test passes

What do you see instead?

Test passes

midawson@drx-hemera io.js]$ tools/test.py test/internet/test-dns-ipv6
=== release test-dns-ipv6 ===                   
Path: internet/test-dns-ipv6
test_resolve6
test_reverse_ipv6
node:internal/errors:496
    ErrorCaptureStackTrace(err);
    ^

Error: getHostByAddr EINVAL 2001:4860:4860::8888
    at node:internal/dns/promises:280:14
    at new Promise (<anonymous>)
    at createResolverPromise (node:internal/dns/promises:267:10)
    at ResolverBase.getHostByAddr (node:internal/dns/promises:299:12)
    at test_reverse_ipv6 (/home/midawson/newpull/io.js/backports/io.js/test/internet/test-dns-ipv6.js:72:36)
    at next (/home/midawson/newpull/io.js/backports/io.js/test/internet/test-dns-ipv6.js:22:7)
    at process.processTicksAndRejections (node:internal/process/task_queues:77:11) {
  errno: -22,
  code: 'EINVAL',
  syscall: 'getHostByAddr',
  hostname: '2001:4860:4860::8888'
}

Additional information

No response

mhdawson commented 1 year ago

I did some bisection as part of https://github.com/nodejs/node/pull/49183 which was what led me to open this issue.

mhdawson commented 1 year ago

Using git bisect it seems to indicate that it was this PR that causes the failure:

0dc485eb28fb05eafad4dc78c646d917d8d4024a is the first bad commit
commit 0dc485eb28fb05eafad4dc78c646d917d8d4024a
Author: Yagiz Nizipli <yagiz@nizipli.com>
Date:   Fri Mar 31 09:04:03 2023 -0400

    url: drop ICU requirement for parsing hostnames

    PR-URL: https://github.com/nodejs/node/pull/47339
    Backport-PR-URL: https://github.com/nodejs/node/pull/48345
    Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
    Reviewed-By: Rich Trott <rtrott@gmail.com>

 deps/ada/ada.gyp                     | 19 ++-----------------
 doc/api/url.md                       | 25 +++++++++++++++----------
 lib/internal/idna.js                 |  9 ++-------
 test/benchmark/test-benchmark-url.js | 14 +-------------
 test/wpt/status/url.json             | 21 ---------------------
 5 files changed, 20 insertions(+), 68 deletions(-)
mhdawson commented 1 year ago

@anonrig can you take a look at the backport and see if you can spot why it might be causing that test to fail?

anonrig commented 1 year ago

We had a similar issue in the past: https://github.com/nodejs/node/issues/48262. Might be related to it. I'll take a look at it soon.

I think we should backport these 2 pull requests as well:

anonrig commented 1 year ago

I think this is caused by calling domainToASCII instead of ada::idna::to_ascii.

https://github.com/nodejs/node/blob/v18.x-staging/lib/internal/idna.js#L3

anonrig commented 1 year ago

Does this issue persist with the v18 proposal?

richardlau commented 11 months ago

This was fixed in Node.js 18.18.0.