test-dns-ipv6.js is broken

[tniessen]

Both in GitHub actions and locally, for the last three weeks as far as I can tell:

=== release test-dns-ipv6 ===
Path: internet/test-dns-ipv6
test_resolve6
test_reverse_ipv6
node:internal/errors:490
    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/runner/work/node/node/test/internet/test-dns-ipv6.js:72:36)
    at next (/home/runner/work/node/node/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'
}

Node.js v21.0.0-pre
Command: out/Release/node /home/runner/work/node/node/test/internet/test-dns-ipv6.js

[tniessen]

git bisect blames ead4079 / #47339. cc @anonrig

[anonrig]

I'm looking into it.

[anonrig]

It works with Node 18 (Ada v1.0.4)

> node -v
v18.16.0

> node test/internet/test-dns-ipv6.js
test_resolve6
test_reverse_ipv6
test_lookup_ipv6_explicit
test_lookup_ipv6_explicit_object
test_lookup_ipv6_hint
test_lookup_ip_ipv6
test_lookup_all_ipv6
test_lookupservice_ip_ipv6

[anonrig]

lib/internal/dns/promises.js line 277 calls toASCII which is:

• const err = resolver._handle[bindingName](req, toASCII(hostname));

[anonrig]

I found the bug: 2001:4860:4860::8888 passed to domainToASCII previously returned the same payload, but with ada::idna, it returns an empty string (detecting invalid). cc @lemire

[anonrig]

@tniessen the following diff fixes the issue, although, we need to fix this and release a new version of Ada...

diff --git a/lib/internal/idna.js b/lib/internal/idna.js
index 566f8590d8..8591226d10 100644
--- a/lib/internal/idna.js
+++ b/lib/internal/idna.js
@@ -1,4 +1,9 @@
 'use strict';

-const { domainToASCII, domainToUnicode } = require('internal/url');
-module.exports = { toASCII: domainToASCII, toUnicode: domainToUnicode };
+if (internalBinding('config').hasIntl) {
+  const { toASCII, toUnicode } = internalBinding('icu');
+  module.exports = { toASCII, toUnicode };
+} else {
+  const { domainToASCII, domainToUnicode } = require('internal/url');
+  module.exports = { toASCII: domainToASCII, toUnicode: domainToUnicode };
+}

[tniessen]

So pretty much reverting ead4079, or at least the lib/ changes of that? I assume it's still going to fail if !hasIntl. Then again, I don't think we care about non-ICU builds.

[anonrig]

I think the idna.js is flawed. icu.toASCII does not equal to domainToASCII. I think our approach is correct, but the previous code was flawed, and our pull request on removing the ICU from idna.js exposed the bug.

[anonrig]

No need to revert, I'll add a new C++ function that directly calls ada::idna::to_ascii and expose them through internalBinding.