nodejs
test: handle EUNATCH on IBM i
Refs: https://github.com/nodejs/node/issues/48049
When IPv6 is disabled IBM i returns EUNATCH (errno 42). For now, I've added an errno check in test case. libuv needs to get updated to handle EUNATCH so that the error code can be addressed as EUNATCH.
This change is related to changes done in https://github.com/nodejs/node/pull/46546
At the time this test case (test-net-autoselectfamily-commandline-option) was not enabled
CC @richardlau CC @nodejs/platform-ibmi
@bnoordhuis
I agree the right way is to make libuv handle it.
-
To my understanding libuv needs to get updated to handle EUNATCH so that the error code can be addressed as
EUNATCH. Similar to https://github.com/libuv/libuv/pull/3145 -
Once EUNATCH is available, I don't know if we want to remap EUNATCH -> ECONNREFUSED though.
@bnoordhuis @mhdawson @richardlau
We can now use error.code to refer to EUNATCH in node versions that use libuv 1.46.0.
I've updated the checks to look for EADDRNOTAVAIL or EUNATCH
CC @nodejs/platform-ibmi
This PR was updated after libuv was updated to 1.46.0. @bnoordhuis does your objection still stand?
@bnoordhuis not sure if you are away, but I think your concerns/comments have been addressed and this is ready to land. If we don't hear back from you by the end of the week I'll assume it's ok to remove your block and proceed.
I'm indeed away. "Gone rock climbing" is intended quite literally but of course I'll take a break from on-siting 7a's for Node.js.
So, I don't believe it's necessary to merge this PR as libuv should be handling it transparently now. Is that not the case?
Hey @bnoordhuis 👋🏿
This is PR is still needed as libuv now has support for the EUNATCH errno.
The libuv changes:
https://github.com/libuv/libuv/pull/4047
We are not remapping EUNATCH -> ECONNREFUSED.
Before we had test cases simply checking the raw errno e.g.
assert.strictEqual(error.errno, -42);
Now we addresses the issue by checking the error code e.g.
error.code === 'EUNATCH'
This PR adds checks for EUNATCH error code in the spots its raised (when ipv6 is disabled).
We have a failing test case on the IBM i CI that is waiting on the changes from this PR:
https://ci.nodejs.org/job/node-test-commit-ibmi/nodes=ibmi73-ppc64/1256/testReport/(root)/parallel/test_net_autoselectfamily_commandline_option/
I've dismissed my review because shrug but I wonder why you didn't opt to handle this inside libuv. I assume IBM wants porting to its platforms to be as frictionless as possible.
I've dismissed my review because shrug but I wonder why you didn't opt to handle this inside libuv. I assume IBM wants porting to its platforms to be as frictionless as possible.
Yes, but it's a hard balance sometimes. EUNATCH represents something different from ECONNREFUSED (or any other errno), and some software behaves appropriately differently when EUNATCH is surfaced.
In this particular case, differentiation of behavior is not needed, but I don't think we want to mask EUNATCH in libuv
ECONNREFUSED is not the correct errno to compare with, since that's used in the case that IPv6 is enabled. The real comparison is with EAFNOSUPPORT and EADDRNOTAVAIL. If we wanted to remap this for "consistency" then, which should we choose? POSIX errnos are already kind of a mess of inconsistencies and libuv returning it directly basically pushes that on to the consumer. I'm not sure why we should remap EUNATACH for this one case when libuv doesn't do that for EAFNOSUPPORT or EADDRNOTAVAIL already (and likely in other circumstances).
ECONNREFUSED is not the correct errno to compare with [..] not sure why we should remap EUNATACH for this one case when libuv doesn't do that for EAFNOSUPPORT or EADDRNOTAVAIL already
My point wasn't that ECONNREFUSED is the best error to report, I just picked it as an example.
EUNATACH is an exotic error (not widely available and rare even on systems that report it) and therefore it's better to map it to something more mundane like EADDRNOTAVAIL.
There's a decent chance a libuv user has logic for handling EADDRNOTAVAIL but EUNATACH? No way.
@abmusse let me know once you have addressed @kadler's comments and the PR is ready to land.
@mhdawson
Yes I've addressed Kevin's comments in https://github.com/nodejs/node/pull/48050/commits/f83b1021341d5e9ddc3ea5e590d947e4e85d5703