Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

report: add fallback for uv_getnameinfo() failures #26140

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
5 participants
@richardlau
Copy link
Member

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

@richardlau

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

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

@richardlau

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2019

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?
report: add fallback for uv_getnameinfo() failures
Attempt to report the host and port in the case that uv_getnameinfo()
fails.

@richardlau richardlau force-pushed the richardlau:forkhandles branch from 06936cd to b49fc72 Feb 19, 2019

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

@richardlau

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

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.

@richardlau richardlau dismissed stale reviews from jasnell and addaleax Feb 19, 2019

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

@richardlau

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

@richardlau

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

@richardlau

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

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

@richardlau

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2019

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

@richardlau richardlau referenced this pull request Feb 24, 2019

Merged

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

3 of 3 tasks complete

@richardlau richardlau requested review from addaleax and cjihrig Feb 27, 2019

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

This comment has been minimized.

Copy link
@addaleax

addaleax Feb 27, 2019

Member

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?

This comment has been minimized.

Copy link
@richardlau

richardlau Feb 27, 2019

Author Member

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

This comment has been minimized.

Copy link
@cjihrig

cjihrig Feb 27, 2019

Contributor

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

@cjihrig
Copy link
Contributor

left a comment

LGTM with a couple suggestions.

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

This comment has been minimized.

Copy link
@cjihrig

cjihrig Feb 27, 2019

Contributor

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

This comment has been minimized.

Copy link
@cjihrig

cjihrig Feb 27, 2019

Contributor

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

@richardlau

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

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

@richardlau richardlau added the needs-ci label Feb 27, 2019

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

This comment has been minimized.

Copy link
@richardlau

richardlau Feb 27, 2019

Author Member

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

@richardlau

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

@richardlau richardlau added author ready and removed needs-ci labels Feb 28, 2019

@richardlau

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

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

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

@addaleax

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Landed in fc4c0de

@addaleax addaleax closed this Mar 1, 2019

addaleax added a commit that referenced this pull request Mar 1, 2019

report: add fallback for uv_getnameinfo() failures
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>

addaleax added a commit that referenced this pull request Mar 2, 2019

report: add fallback for uv_getnameinfo() failures
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>

@BridgeAR BridgeAR referenced this pull request Mar 4, 2019

Merged

v11.11.0 proposal #26322

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

Fix CI failures on ubuntu1804
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
Ports fixes from core:
nodejs/node#25962
nodejs/node#26140

PR-URL: #123
Fixes: #120
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.