test: handle EUNATCH #48050

abmusse · 2023-05-17T20:19:49Z

Refs: #48049
Refs: #46546

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

mhdawson

LGTM

test/parallel/test-net-autoselectfamily-commandline-option.js

bnoordhuis

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

abmusse · 2023-05-18T12:45:09Z

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

abmusse · 2023-06-19T16:54:11Z

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

abmusse · 2023-07-06T21:38:49Z

@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

mhdawson

LGTM

abmusse · 2023-07-18T20:38:12Z

Ping @bnoordhuis

Can you please re-review the latest changes?

richardlau · 2023-08-08T14:10:33Z

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

nodejs-github-bot · 2023-08-14T18:22:54Z

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

mhdawson · 2023-08-15T20:49:53Z

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

bnoordhuis · 2023-08-17T10:42:44Z

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?

abmusse · 2023-08-17T15:32:53Z

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

bnoordhuis · 2023-08-17T20:58:25Z

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.

ThePrez · 2023-08-17T22:57:35Z

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

kadler · 2023-08-18T16:33:48Z

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

kadler · 2023-08-18T16:39:06Z

test/parallel/test-net-autoselectfamily-commandline-option.js

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


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

bnoordhuis · 2023-08-19T10:27:34Z

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.

mhdawson · 2023-08-22T14:39:18Z

@abmusse let me know once you have addressed @kadler's comments and the PR is ready to land.

abmusse · 2023-08-22T14:54:31Z

@mhdawson

Yes I've addressed Kevin's comments in f83b102

nodejs-github-bot · 2023-08-22T15:00:26Z

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

nodejs-github-bot · 2023-08-23T14:40:33Z

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>

mhdawson · 2023-08-23T18:01:24Z

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>

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: nodejs#48050 Refs: nodejs#48049 Refs: nodejs#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

abmusse force-pushed the ibmi-fix-test-parallel-test-net-autoselectfamily-commandline-option branch from 29a4028 to 6ec3fe0 Compare May 17, 2023 20:22

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

abmusse force-pushed the ibmi-fix-test-parallel-test-net-autoselectfamily-commandline-option branch from 6ec3fe0 to 7b86dc9 Compare May 17, 2023 20:24

mhdawson approved these changes May 17, 2023

View reviewed changes

tniessen approved these changes May 18, 2023

View reviewed changes

test/parallel/test-net-autoselectfamily-commandline-option.js Outdated Show resolved Hide resolved

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 force-pushed the ibmi-fix-test-parallel-test-net-autoselectfamily-commandline-option branch from 7b86dc9 to 09207b8 Compare July 6, 2023 21:33

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: handle EUNATCH #48050

test: handle EUNATCH #48050

abmusse commented May 17, 2023 •

edited

Loading

mhdawson left a comment

bnoordhuis left a comment

abmusse commented May 18, 2023

abmusse commented Jun 19, 2023

abmusse commented Jul 6, 2023

mhdawson left a comment

abmusse commented Jul 18, 2023

richardlau commented Aug 8, 2023

nodejs-github-bot commented Aug 14, 2023

mhdawson commented Aug 15, 2023

bnoordhuis commented Aug 17, 2023

abmusse commented Aug 17, 2023

bnoordhuis commented Aug 17, 2023

ThePrez commented Aug 17, 2023

kadler commented Aug 18, 2023

kadler Aug 18, 2023

abmusse Aug 18, 2023

abmusse Aug 22, 2023 •

edited

Loading

bnoordhuis commented Aug 19, 2023

mhdawson commented Aug 22, 2023

abmusse commented Aug 22, 2023 •

edited

Loading

nodejs-github-bot commented Aug 22, 2023

nodejs-github-bot commented Aug 23, 2023

mhdawson commented Aug 23, 2023

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

-        } 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)`);

test: handle EUNATCH #48050

test: handle EUNATCH #48050

Conversation

abmusse commented May 17, 2023 • edited Loading

mhdawson left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

abmusse commented May 18, 2023

abmusse commented Jun 19, 2023

abmusse commented Jul 6, 2023

mhdawson left a comment

Choose a reason for hiding this comment

abmusse commented Jul 18, 2023

richardlau commented Aug 8, 2023

nodejs-github-bot commented Aug 14, 2023

mhdawson commented Aug 15, 2023

bnoordhuis commented Aug 17, 2023

abmusse commented Aug 17, 2023

bnoordhuis commented Aug 17, 2023

ThePrez commented Aug 17, 2023

kadler commented Aug 18, 2023

kadler Aug 18, 2023

Choose a reason for hiding this comment

abmusse Aug 18, 2023

Choose a reason for hiding this comment

abmusse Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

bnoordhuis commented Aug 19, 2023

mhdawson commented Aug 22, 2023

abmusse commented Aug 22, 2023 • edited Loading

nodejs-github-bot commented Aug 22, 2023

nodejs-github-bot commented Aug 23, 2023

mhdawson commented Aug 23, 2023

abmusse commented May 17, 2023 •

edited

Loading

abmusse Aug 22, 2023 •

edited

Loading

abmusse commented Aug 22, 2023 •

edited

Loading