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.

Sign up for GitHub

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
Closed

report: add fallback for uv_getnameinfo() failures #26140

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b49fc72
report: add fallback for uv_getnameinfo() failures
richardlau Feb 15, 2019
f9b4388
fixup! report: add fallback for uv_getnameinfo() failures
richardlau Feb 27, 2019
93e3a3b
fixup! report: add fallback for uv_getnameinfo() failures
richardlau Feb 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 33 additions & 27 deletions src/node_report_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,42 @@ using node::MallocedBuffer;

static constexpr auto null = JSONWriter::Null{};

// Utility function to format socket information.
static bool ReportEndpoint(uv_handle_t* h,
struct sockaddr* addr,
const char* name,
JSONWriter* writer) {
uv_getnameinfo_t endpoint;
if (uv_getnameinfo(h->loop, &endpoint, nullptr, addr, NI_NUMERICSERV) == 0) {
writer->json_objectstart(name);
writer->json_keyvalue("host", endpoint.host);
writer->json_keyvalue("port", endpoint.service);
writer->json_objectend();
return true;
} else {
char host[INET6_ADDRSTRLEN];
int family = addr->sa_family;
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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

writer->json_objectstart(name);
writer->json_keyvalue("host", host);
writer->json_keyvalue("port", port);
writer->json_objectend();
return true;
}
return false;
}
}

// Utility function to format libuv socket information.
static void ReportEndpoints(uv_handle_t* h, JSONWriter* writer) {
struct sockaddr_storage addr_storage;
struct sockaddr* addr = reinterpret_cast<sockaddr*>(&addr_storage);
uv_any_handle* handle = reinterpret_cast<uv_any_handle*>(h);
int addr_size = sizeof(addr_storage);
int rc = -1;
bool wrote_local_endpoint = false;
bool wrote_remote_endpoint = false;

switch (h->type) {
case UV_UDP:
Expand All @@ -27,38 +54,17 @@ static void ReportEndpoints(uv_handle_t* h, JSONWriter* writer) {
default:
break;
}
if (rc == 0) {
// uv_getnameinfo will format host and port and handle IPv4/IPv6.
uv_getnameinfo_t local;
rc = uv_getnameinfo(h->loop, &local, nullptr, addr, NI_NUMERICSERV);

if (rc == 0) {
writer->json_objectstart("localEndpoint");
writer->json_keyvalue("host", local.host);
writer->json_keyvalue("port", local.service);
writer->json_objectend();
wrote_local_endpoint = true;
}
if (rc != 0 || !ReportEndpoint(h, addr, "localEndpoint", writer)) {
writer->json_keyvalue("localEndpoint", null);
Copy link
Contributor

@cjihrig cjihrig Feb 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (!wrote_local_endpoint) writer->json_keyvalue("localEndpoint", null);

if (h->type == UV_TCP) {
// Get the remote end of the connection.
rc = uv_tcp_getpeername(&handle->tcp, addr, &addr_size);
if (rc == 0) {
uv_getnameinfo_t remote;
rc = uv_getnameinfo(h->loop, &remote, nullptr, addr, NI_NUMERICSERV);

if (rc == 0) {
writer->json_objectstart("remoteEndpoint");
writer->json_keyvalue("host", remote.host);
writer->json_keyvalue("port", remote.service);
writer->json_objectend();
wrote_local_endpoint = true;
}
if (rc != 0 || !ReportEndpoint(h, addr, "remoteEndpoint", writer)) {
writer->json_keyvalue("remoteEndpoint", null);
}
}
if (!wrote_remote_endpoint) writer->json_keyvalue("remoteEndpoint", null);
}

// Utility function to format libuv path information.
Expand Down
89 changes: 57 additions & 32 deletions test/node-report/test-api-uvhandles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,7 @@ if (process.argv[2] === 'child') {
const data = { pid: child_process.pid,
tcp_address: server.address(),
udp_address: udp_socket.address(),
skip_fs_watch: (watcher === undefined ?
'fs.watch() unavailable' :
false) };
skip_fs_watch: (watcher === undefined) };
process.send(data);
http.get({ port: server.address().port });
});
Expand All @@ -74,6 +72,8 @@ if (process.argv[2] === 'child') {
tmpdir.refresh();
const options = { encoding: 'utf8', silent: true, cwd: tmpdir.path };
const child = fork('--experimental-report', [__filename, 'child'], options);
let child_data;
child.on('message', (data) => { child_data = data; });
let stderr = '';
child.stderr.on('data', (chunk) => { stderr += chunk; });
let stdout = '';
Expand All @@ -94,36 +94,61 @@ if (process.argv[2] === 'child') {
assert.deepStrictEqual(reports, [], report_msg, reports);

const report = JSON.parse(stdout);
let fs = 0;
let poll = 0;
let process = 0;
let timer = 0;
let pipe = 0;
let tcp = 0;
let udp = 0;
const fs_msg = 'fs_event not found';
const poll_msg = 'poll_event not found';
const process_msg = 'process event not found';
const timer_msg = 'timer event not found';
const pipe_msg = 'pipe event not found';
const tcp_msg = 'tcp event not found';
const udp_msg = 'udp event not found';
for (const entry in report.libuv) {
if (report.libuv[entry].type === 'fs_event') fs = 1;
else if (report.libuv[entry].type === 'fs_poll') poll = 1;
else if (report.libuv[entry].type === 'process') process = 1;
else if (report.libuv[entry].type === 'timer') timer = 1;
else if (report.libuv[entry].type === 'pipe') pipe = 1;
else if (report.libuv[entry].type === 'tcp') tcp = 1;
else if (report.libuv[entry].type === 'udp') udp = 1;
const prefix = common.isWindows ? '\\\\?\\' : '';
const expected_filename = `${prefix}${__filename}`;
const found_tcp = [];
// Functions are named to aid debugging when they are not called.
const validators = {
fs_event: common.mustCall(function fs_event_validator(handle) {
if (!child_data.skip_fs_watch) {
assert.strictEqual(handle.filename, expected_filename);
assert(handle.is_referenced);
}
}),
fs_poll: common.mustCall(function fs_poll_validator(handle) {
assert.strictEqual(handle.filename, expected_filename);
assert(handle.is_referenced);
}),
pipe: common.mustCallAtLeast(function pipe_validator(handle) {
assert(handle.is_referenced);
}),
process: common.mustCall(function process_validator(handle) {
assert.strictEqual(handle.pid, child_data.pid);
assert(handle.is_referenced);
}),
tcp: common.mustCall(function tcp_validator(handle) {
// TCP handles. The report should contain three sockets:
// 1. The server's listening socket.
// 2. The inbound socket making the request.
// 3. The outbound socket sending the response.
const port = child_data.tcp_address.port.toString();
if (handle.localEndpoint.port === port) {
if (handle.remoteEndpoint === null) {
found_tcp.push('listening');
} else {
found_tcp.push('inbound');
}
} else if (handle.remoteEndpoint.port === port) {
found_tcp.push('outbound');
}
assert(handle.is_referenced);
}, 3),
timer: common.mustCall(function timer_validator(handle) {
assert(!handle.is_referenced);
assert.strictEqual(handle.repeat, 0);
}),
udp: common.mustCall(function udp_validator(handle) {
assert.strictEqual(handle.localEndpoint.port,
child_data.udp_address.port.toString());
assert(handle.is_referenced);
}),
};
for (const entry of report.libuv) {
if (validators[entry.type]) validators[entry.type](entry);
}
for (const socket of ['listening', 'inbound', 'outbound']) {
assert(found_tcp.includes(socket), `${socket} TCP socket was not found`);
}
assert.deepStrictEqual(fs, 1, fs_msg);
assert.deepStrictEqual(poll, 1, poll_msg);
assert.deepStrictEqual(process, 1, process_msg);
assert.deepStrictEqual(timer, 1, timer_msg);
assert.deepStrictEqual(pipe, 1, pipe_msg);
assert.deepStrictEqual(tcp, 1, tcp_msg);
assert.deepStrictEqual(udp, 1, udp_msg);

// Common report tests.
helper.validateContent(stdout);
Expand Down