test: handle EUNATCH by abmusse · Pull Request #48050 · nodejs/node · GitHub

When IPv6 is disabled IBM i returns EUNATCH (errno 42)
instead of EADDRNOTAVAIL.

libuv 1.46.0 adds EUNATCH errno

We can now use error.code to refer to EUNATCH
in node versions that use libuv 1.46.0.

CC @richardlau
CC @nodejs/platform-ibmi

LGTM

This is a bandaid and I believe you're aware of that. The right way to fix it is to make libuv handle it.

I can't test it but the change would look something like this:

diff --git a/src/unix/stream.c b/src/unix/stream.c
index 03f92b50..1d01eac5 100644
--- a/src/unix/stream.c
+++ b/src/unix/stream.c
@@ -60,6 +60,10 @@ struct uv__stream_select_s {
 };
 #endif /* defined(__APPLE__) */

+#ifndef EUNATCH
+#define EUNATCH 0
+#undef
+
 union uv__cmsg {
   struct cmsghdr hdr;
   /* This cannot be larger because of the IBMi PASE limitation that
@@ -1267,6 +1271,11 @@ static void uv__stream_connect(uv_stream_t* stream) {
   if (error == UV__ERR(EINPROGRESS))
     return;

+  /* Work around ibmi quirk. */
+  if (EUNATCH)
+    if (error == UV__ERR(EUNATCH))
+      error = UV__ERR(ECONNREFUSED);
+
   stream->connect_req = NULL;
   uv__req_unregister(stream->loop, req);

@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 include: add EOVERFLOW status code mapping libuv/libuv#3145
Once EUNATCH is available, I don't know if we want to remap EUNATCH -> ECONNREFUSED though.

@bnoordhuis

I've landed a PR to add EUNATCH support in libuv.

When IPv6 is disabled IBM i returns EUNATCH (errno 42) instead of EADDRNOTAVAIL. libuv 1.46.0 adds EUNATCH errno We can now use error.code to refer to EUNATCH in node versions that use libuv 1.46.0.

@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

LGTM

Ping @bnoordhuis

Can you please re-review the latest changes?

This PR was updated after libuv was updated to 1.46.0. @bnoordhuis does your objection still stand?

CI: https://ci.nodejs.org/job/node-test-pull-request/53323/

@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:

libuv/libuv#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/

no strong objections

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).

This doesn't match existing code behavior. Should add a separate else if leg like the EAFNOSUPPORT above.

Suggested change

      
                    } else if (error.code === 'EADDRNOTAVAIL' || error.code === 'EUNATCH') {
          
                      assert.strictEqual(error.message, `connect ${error.code} ::1:${port} - Local (:::0)`);
          
                    } else if (error.code === 'EUNATCH') {
          
                      assert.strictEqual(error.message, `connect EUNATCH ::1:${port} - Local (:::0)`);
          
                    } else {
          
                      assert.strictEqual(error.code, 'EADDRNOTAVAIL');
          
                      assert.strictEqual(error.message, `connect EADDRNOTAVAIL ::1:${port} - Local (:::0)`);

Either that or merge the EAFNOSUPPORT else block in to this one. The other tests have this same issue.

I like the option of adding a separate else leg. Will fix this up!

Added else if legs for EUNATCH in f83b102

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 f83b102

CI: https://ci.nodejs.org/job/node-test-pull-request/53495/

CI: https://ci.nodejs.org/job/node-test-pull-request/53523/

When IPv6 is disabled IBM i returns EUNATCH (errno 42) instead of EADDRNOTAVAIL. libuv 1.46.0 adds EUNATCH errno We can now use error.code to refer to EUNATCH in node versions that use libuv 1.46.0. PR-URL: #48050 Refs: #48049 Refs: #46546 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Landed in ee1f609

When IPv6 is disabled IBM i returns EUNATCH (errno 42) instead of EADDRNOTAVAIL. libuv 1.46.0 adds EUNATCH errno We can now use error.code to refer to EUNATCH in node versions that use libuv 1.46.0. PR-URL: #48050 Refs: #48049 Refs: #46546 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels May 17, 2023

richardlau approved these changes May 17, 2023

View reviewed changes

richardlau added the ibm i Issues and PRs related to the IBM i platform. label May 17, 2023

abmusse changed the title test: Handle EUNATCH errno on IBM i test: handle EUNATCH on IBM i May 17, 2023

mhdawson approved these changes May 17, 2023

View reviewed changes

tniessen approved these changes May 18, 2023

View reviewed changes

Comment thread test/parallel/test-net-autoselectfamily-commandline-option.js Outdated

bnoordhuis previously requested changes May 18, 2023

View reviewed changes

abmusse mentioned this pull request Jun 13, 2023

include: add EUNATCH errno mapping libuv/libuv#4047

Merged

test: handle EUNATCH

09207b8

When IPv6 is disabled IBM i returns EUNATCH (errno 42) instead of EADDRNOTAVAIL. libuv 1.46.0 adds EUNATCH errno We can now use error.code to refer to EUNATCH in node versions that use libuv 1.46.0.

abmusse changed the title test: handle EUNATCH on IBM i test: handle EUNATCH Jul 6, 2023

mhdawson approved these changes Jul 12, 2023

View reviewed changes

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023

kadler reviewed Aug 18, 2023

View reviewed changes

fixup! test: handle EUNATCH

f83b102

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 22, 2023

github-actions Bot mentioned this pull request Aug 23, 2023

CI Reliability 2023-08-23 nodejs/reliability#643

Open

27 tasks

mhdawson closed this Aug 23, 2023

UlisesGascon mentioned this pull request Sep 10, 2023

v20.7.0 proposal #49592

Merged

		} else if (error.code === 'EADDRNOTAVAIL' \|\| error.code === 'EUNATCH') {
		assert.strictEqual(error.message, `connect ${error.code} ::1:${port} - Local (:::0)`);

test: handle EUNATCH by abmusse · Pull Request #48050 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: handle EUNATCH - #48050

test: handle EUNATCH#48050abmusse wants to merge 2 commits intonodejs:mainnodejs/node:mainfrom abmusse:ibmi-fix-test-parallel-test-net-autoselectfamily-commandline-optionCopy head branch name to clipboard

Conversation

abmusse commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

abmusse commented May 18, 2023

Uh oh!

abmusse commented Jun 19, 2023

Uh oh!

abmusse commented Jul 6, 2023

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

abmusse commented Jul 18, 2023

Uh oh!

richardlau commented Aug 8, 2023

Uh oh!

nodejs-github-bot commented Aug 14, 2023

Uh oh!

mhdawson commented Aug 15, 2023

Uh oh!

bnoordhuis commented Aug 17, 2023

Uh oh!

abmusse commented Aug 17, 2023

Uh oh!

bnoordhuis commented Aug 17, 2023

Uh oh!

ThePrez commented Aug 17, 2023

Uh oh!

kadler commented Aug 18, 2023

Uh oh!

kadler Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

abmusse Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

abmusse Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Aug 19, 2023

Uh oh!

mhdawson commented Aug 22, 2023

Uh oh!

abmusse commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 22, 2023

Uh oh!

nodejs-github-bot commented Aug 23, 2023

Uh oh!

mhdawson commented Aug 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

test: handle EUNATCH#48050
abmusse wants to merge 2 commits into
nodejs:mainfrom
abmusse:ibmi-fix-test-parallel-test-net-autoselectfamily-commandline-option

abmusse commented May 17, 2023 •
edited
Loading

abmusse Aug 22, 2023 •
edited
Loading

abmusse commented Aug 22, 2023 •
edited
Loading