report: add fallback for uv_getnameinfo() failures by richardlau · Pull Request #26140 · nodejs/node · GitHub

I started on the test changes first (to test the information reported in the libuv section) and uncovered this bug (the parsed object had null for all remoteEndpoint keys).

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

@richardlau build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2627/pipeline

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

Failue on ubuntu1804 is also seen in the CI for the standalone module when run through CITGM: nodejs/node-report#120 (comment)

14:45:23 not ok 39 node-report/test-api-uvhandles
14:45:23   ---
14:45:23   duration_ms: 0.263
14:45:23   severity: fail
14:45:23   exitcode: 1
14:45:23   stack: |-
14:45:23     /home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-64/test/node-report/test-api-uvhandles.js:139
14:45:23             assert.strictEqual(handle.localEndpoint.host,
14:45:23                                                     ^
14:45:23     
14:45:23     TypeError: Cannot read property 'host' of null
14:45:23         at Object.udp_validator (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-64/test/node-report/test-api-uvhandles.js:139:49)
14:45:23         at Object.udp (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-64/test/common/index.js:367:15)
14:45:23         at ChildProcess.child.on.common.mustCall (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-64/test/node-report/test-api-uvhandles.js:147:57)
14:45:23         at ChildProcess.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1804-64/test/common/index.js:367:15)
14:45:23         at ChildProcess.emit (events.js:197:13)
14:45:23         at Process.ChildProcess._handle.onexit (internal/child_process.js:254:12)
14:45:23   ...

Windows failures are due to the filename being prefixed with \\?\.

14:57:33 not ok 24 node-report/test-api-uvhandles
14:57:33   ---
14:57:33   duration_ms: 0.804
14:57:33   severity: fail
14:57:33   exitcode: 1
14:57:33   stack: |-
14:57:33     assert.js:86
14:57:33       throw new AssertionError(obj);
14:57:33       ^
14:57:33     
14:57:33     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
14:57:33     + actual - expected
14:57:33     
14:57:33     + '\\\\?\\c:\\workspace\\node-test-binary-windows\\test\\node-report\\test-api-uvhandles.js'
14:57:33     - 'c:\\workspace\\node-test-binary-windows\\test\\node-report\\test-api-uvhandles.js'
14:57:33         at Object.fs_event_validator (c:\workspace\node-test-binary-windows\test\node-report\test-api-uvhandles.js:102:18)
14:57:33         at Object.fs_event (c:\workspace\node-test-binary-windows\test\common\index.js:367:15)
14:57:33         at ChildProcess.child.on.common.mustCall (c:\workspace\node-test-binary-windows\test\node-report\test-api-uvhandles.js:147:57)
14:57:33         at ChildProcess.<anonymous> (c:\workspace\node-test-binary-windows\test\common\index.js:367:15)
14:57:33         at ChildProcess.emit (events.js:197:13)
14:57:33         at Process.ChildProcess._handle.onexit (internal/child_process.js:254:12)
14:57:33   ...

Sticking a WIP label on for the moment while I fix the test for Windows. The ubuntu1804 also needs investigating.

Re. ubuntu1804 failures.

I've stuck some temporary debug code and get this on the CI:

05:56:34     { localEndpointError:
05:56:34        { code: 'EAI_AGAIN',
05:56:34          error: 'temporary failure',
05:56:34          host: '0.0.0.0',
05:56:34          port: 47765 },
05:56:34       localEndpoint: null,
05:56:34       remoteEndpoint: null,
05:56:34       sendBufferSize: 212992,
05:56:34       recvBufferSize: 212992,
05:56:34       fd: 23,
05:56:34       type: 'udp',
05:56:34       is_active: true,
05:56:34       is_referenced: true,
05:56:34       address: '0x000055a9cca00e50' }

So

The uv_getnameinfo call failed with EAI_AGAIN. At the moment we're just ignoring errors (i.e. localEndpoint is null).
We can fall back to uv_ip4_name/uv_ip6_name and reading the port from the sockaddr struct. Or maybe we should do that in all cases, instead of calling uv_getnameinfo, for efficiency (i.e. avoid the reverse lookup) at the cost of only have numeric hosts?

Attempt to report the host and port in the case that uv_getnameinfo() fails.

I've reworked the endpoint reporting by factoring out to another function and adding a fallback for the case that uv_getnameinfo() fails. Since the changes to src/node_report_utils.cc are now different, I'm dismissing the existing reviews.

Outdated - Changes to src/node_report_utils.cc are now different.

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

alpine failure is nodejs/build#1682

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/20890/ (✔️)

Ports fixes from core: nodejs/node#25962 nodejs/node#26140 Fixes: nodejs#120

I've also ported this over to the standalone module (nodejs/node-report#123) to fix CITGM failures seen on ubuntu1804 on our CI (nodejs/node-report#120).

Does it make sense to represent the port as an integer? That would allow skipping the stringifying step, but I imagine we’d also need to do that in the uv_getnameinfo() case?

Yes, it probably does make sense to be an integer. I've stringified it here for consistency with the existing uv_getnameinfo() case.

If you just assign the host and port in the conditionals, then you can DRY the writer code a bit.

LGTM with a couple suggestions.

Could you move this into ReportEndpoint()? Then, this logic could just be:

if (rc != 0)
  ReportEndpoint(h, addr, "localEndpoint", writer);

Same for the remote endpoint code below.

If you just assign the host and port in the conditionals, then you can DRY the writer code a bit.

Refactored ReportEndpoint() based on suggestions. port is now written as an integer. PTAL.

I believe it's extremely unlikely for host to be nullptr (both uv_getnameinfo() and uv_inet_ntop() would have to fail) here but in the case that it is I've opted to omit the host key. Let me know if you'd prefer to emit host: null instead (i.e. the host key is always written), or something else (host: '<unable to determine>'?).

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

AIX failure is infra (disk space issues): nodejs/build#1708

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/21036/ (✔️ )

Landed in fc4c0de

Attempt to report the host and port in the case that uv_getnameinfo() fails. PR-URL: #26140 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Ports fixes from core: nodejs/node#25962 nodejs/node#26140 PR-URL: nodejs#123 Fixes: nodejs#120 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Ports fixes from core: nodejs/node#25962 nodejs/node#26140 PR-URL: #123 Fixes: #120 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 15, 2019

addaleax previously approved these changes Feb 15, 2019

View reviewed changes

richardlau added the wip Issues and PRs that are still a work in progress. label Feb 15, 2019

jasnell previously approved these changes Feb 18, 2019

View reviewed changes

report: add fallback for uv_getnameinfo() failures

b49fc72

Attempt to report the host and port in the case that uv_getnameinfo() fails.

richardlau force-pushed the forkhandles branch from 06936cd to b49fc72 Compare February 19, 2019 16:45

richardlau changed the title report: avoid writing second null remote endpoint report: add fallback for uv_getnameinfo() failures Feb 19, 2019

richardlau added report Issues and PRs related to process.report. and removed wip Issues and PRs that are still a work in progress. labels Feb 19, 2019

richardlau added a commit to richardlau/nodereport that referenced this pull request Feb 21, 2019

Fix CI failures on ubuntu1804

78d03d3

Ports fixes from core: nodejs/node#25962 nodejs/node#26140 Fixes: nodejs#120

This was referenced Feb 21, 2019

Fix CI failures on ubuntu1804 nodejs/node-report#123

Merged

Test suite fails on ubuntu1804 nodejs/node-report#120

Closed

richardlau mentioned this pull request Feb 24, 2019

test: simplify node-report/test-exception.js #26277

Merged

3 tasks

richardlau requested review from addaleax and cjihrig February 27, 2019 00:39

addaleax approved these changes Feb 27, 2019

View reviewed changes

cjihrig approved these changes Feb 27, 2019

View reviewed changes

fixup! report: add fallback for uv_getnameinfo() failures

f9b4388

richardlau added the needs-ci PRs that need a full CI run. label Feb 27, 2019

fixup! report: add fallback for uv_getnameinfo() failures

93e3a3b

richardlau commented Feb 27, 2019

View reviewed changes

richardlau added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed needs-ci PRs that need a full CI run. labels Feb 28, 2019

addaleax closed this Mar 1, 2019

BridgeAR mentioned this pull request Mar 4, 2019

v11.11.0 proposal #26322

Merged

richardlau added a commit to richardlau/nodereport that referenced this pull request Mar 5, 2019

Fix CI failures on ubuntu1804

0bbe2a4

Ports fixes from core: nodejs/node#25962 nodejs/node#26140 PR-URL: nodejs#123 Fixes: nodejs#120 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

richardlau added a commit to nodejs/node-report that referenced this pull request Mar 5, 2019

Fix CI failures on ubuntu1804

d28201c

Ports fixes from core: nodejs/node#25962 nodejs/node#26140 PR-URL: #123 Fixes: #120 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

+                  if (uv_inet_ntop(family, addr, host, sizeof(host)) == 0) {
+                    std::string port = std::to_string(ntohs(family == AF_INET ?
+                                       reinterpret_cast<sockaddr_in*>(addr)->sin_port :
+                                       reinterpret_cast<sockaddr_in6*>(addr)->sin6_port));

+                  if (uv_inet_ntop(family, addr, host, sizeof(host)) == 0) {
+                    std::string port = std::to_string(ntohs(family == AF_INET ?
+                                       reinterpret_cast<sockaddr_in*>(addr)->sin_port :
+                                       reinterpret_cast<sockaddr_in6*>(addr)->sin6_port));

-                    wrote_local_endpoint = true;
-                  }
+                if (rc != 0 || !ReportEndpoint(h, addr, "localEndpoint", writer)) {
+                  writer->json_keyvalue("localEndpoint", null);

+                }
+                writer->json_objectstart(name);
+                if (host != nullptr) {
+                  writer->json_keyvalue("host", host);

report: add fallback for uv_getnameinfo() failures by richardlau · Pull Request #26140 · nodejs/node · GitHub

Uh oh!

Uh oh!

report: add fallback for uv_getnameinfo() failures - #26140

report: add fallback for uv_getnameinfo() failures#26140richardlau wants to merge 3 commits intonodejs:masternodejs/node:masterfrom richardlau:forkhandlesrichardlau/node-1:forkhandlesCopy head branch name to clipboard

Conversation

richardlau commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Feb 15, 2019

Uh oh!

richardlau commented Feb 15, 2019

Uh oh!

richardlau commented Feb 15, 2019

Uh oh!

richardlau commented Feb 15, 2019

Uh oh!

richardlau commented Feb 15, 2019

Uh oh!

richardlau commented Feb 16, 2019

Uh oh!

richardlau commented Feb 19, 2019

Uh oh!

richardlau commented Feb 19, 2019

Uh oh!

richardlau commented Feb 19, 2019

Uh oh!

richardlau commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Feb 21, 2019

Uh oh!

addaleax Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

richardlau Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

cjihrig Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Feb 27, 2019 • 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!

cjihrig Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

richardlau commented Feb 27, 2019

Uh oh!

richardlau Feb 27, 2019

Choose a reason for hiding this comment

Uh oh!

richardlau commented Feb 28, 2019

Uh oh!

richardlau commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Mar 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

report: add fallback for uv_getnameinfo() failures#26140
richardlau wants to merge 3 commits into
nodejs:masterfrom
richardlau:forkhandles

richardlau commented Feb 15, 2019 •
edited
Loading

richardlau commented Feb 19, 2019 •
edited
Loading

cjihrig Feb 27, 2019 •
edited
Loading

richardlau commented Feb 28, 2019 •
edited
Loading