src,worker: display remaining handles if `uv_loop_close` fails #21238

addaleax · Jun 9, 2018

This is refactoring our native stack trace code, and adding support for a better error message when a uv_loop_close() for a worker fails because there are still active handles.

Example output in that case (taken from test failures on the current canary branch while looking into nodejs/node-v8#66):

uv loop at [0x5623f5b32950] has active handles
[0x5623f5b13360] async
	Close callback: (nil) 
	Data: 0x5623f5b343e0 
	(First field): 0x5623f4dbf000 vtable for node::PerIsolatePlatformData [out/Release/node]
out/Release/node[28598]: ../src/debug_utils.cc:217:void node::CheckedUvLoopClose(uv_loop_t*): Assertion `0 && "uv_loop_close() while having open handles"' failed.
 1: node::Abort() [out/Release/node]
[ ... stack trace ... ]

As opposed to the previous error message:

out/Release/node[31550]: ../src/node_worker.cc:307:virtual node::worker::Worker::~Worker(): Assertion `(uv_loop_close(&loop_)) == (0)' failed.
 1: node::Abort() [out/Release/node]
[ ... stack trace ... ]

Checklist

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

nodejs-github-bot · Jun 9, 2018

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

addaleax · Jun 10, 2018

Full CI: https://ci.nodejs.org/job/node-test-commit/19128/

TimothyGu · Jun 10, 2018

TimothyGu approved these changes Jun 10, 2018

View changes

TimothyGu · Jun 10, 2018

LGTM when the testing issues get ironed out.

jasnell · Jun 10, 2018

jasnell approved these changes Jun 10, 2018

View changes

devsnek · Jun 10, 2018

devsnek approved these changes Jun 10, 2018

View changes

addaleax · Jun 11, 2018

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

Trott · Jun 11, 2018

Some failures in Alpine Linux on CI.

Re-run: https://ci.nodejs.org/job/node-test-pull-request/15385/

Trott · Jun 11, 2018

Same failures again on Alpine.

20:55:58 not ok 2114 abort/test-addon-uv-handle-leak
20:55:58   ---
20:55:58   duration_ms: 0.955
20:55:58   severity: fail
20:55:58   exitcode: 1
20:55:58   stack: |-
20:55:58     assert.js:270
20:55:58         throw err;
20:55:58         ^
20:55:58     
20:55:58     AssertionError [ERR_ASSERTION]: uv loop at [0x562f2861e130] has active handles
20:55:58     [0x562f286a6080] timer
20:55:58     	Close callback: 0x7f99924f80f0 
20:55:58     	Data: 0x7f99926f9060 
20:55:58     [0x562f286ba640] timer
20:55:58     	Close callback: 0x7f99924f80f0 
20:55:58     	Data: 0 
20:55:58     [0x562f286ba700] timer
20:55:58     	Close callback: 0x7f99924f80f0 
20:55:58     	Data: 0x42 
20:55:58     /home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-latest-x64/out/Release/node[4614]: ../src/debug_utils.cc:219:void node::CheckedUvLoopClose(uv_loop_t*): Assertion `0 && "uv_loop_close() while having open handles"' failed.
20:55:58     
20:55:58         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/alpine-latest-x64/test/abort/test-addon-uv-handle-leak.js:53:5)
20:55:58         at Module._compile (internal/modules/cjs/loader.js:702:30)
20:55:58         at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
20:55:58         at Module.load (internal/modules/cjs/loader.js:612:32)
20:55:58         at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
20:55:58         at Function.Module._load (internal/modules/cjs/loader.js:543:3)
20:55:58         at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)
20:55:58         at startup (internal/bootstrap/node.js:267:19)
20:55:58         at bootstrapNodeJSCore (internal/bootstrap/node.js:600:3)
20:55:58   ...

addaleax · Jun 11, 2018

Yeah, this is still in progress … do we have any way of detecting musl libc? It would be a shame to skip that test part for all of Linux…

bnoordhuis · Jun 11, 2018

bnoordhuis approved these changes Jun 11, 2018

View changes

Apropos checking for musl libc, it's complicated...

You can check ldd node and see if there's a line about libc.so:

If not, it's probably musl, statically linked.
If yes, check if nm /path/to/libc.so prints gnu_get_libc_version. If not, it's probably musl.

It's easier to check if os.hostname() is one of the alpine buildbots, or if /etc/alpine-release exists.

That's not strictly correct (all alpine is musl but not all musl is alpine) but it's much less brittle.

bnoordhuis · Jun 11, 2018

src/debug_utils.cc

+ public:
+  Win32SymbolDebuggingContext() {
+    current_process_ = GetCurrentProcess();
+    (void)SymInitialize(current_process_, nullptr, true);


bnoordhuis · Jun 11, 2018

src/debug_utils.cc

+  bool IsMapped(void* address) override {
+    void* page_aligned = reinterpret_cast<void*>(
+        // NOLINTNEXTLINE(whitespace/operators)
+        reinterpret_cast<uintptr_t>(address) &~ (pagesize_ - 1));


Why not ... & ~(...)? Then you don't have to suppress the linter.

I’m getting a déjà vu with that review comment … 😄 I’ve switched it for now, but I really prefer &~ for masking operations

bnoordhuis · Jun 11, 2018

src/debug_utils.cc

+    void* page_aligned = reinterpret_cast<void*>(
+        // NOLINTNEXTLINE(whitespace/operators)
+        reinterpret_cast<uintptr_t>(address) &~ (pagesize_ - 1));
+    return msync(page_aligned, pagesize_, 0) == 0;


I'm surprised this works; passing in no flags is technically an error (they're non-zero on linux and macos.)

@bnoordhuis I guess this could be made MS_ASYNC? Reading the Linux man page, it seems like that’s how Linux interprets 0 anyway …

I’m switching and seeing how that fares in CI.

addaleax · Jun 11, 2018

I’ve tried the ldd + nm suggestion – let’s see how that goes, if not, we can just go for one of the simpler options.

CI: https://ci.nodejs.org/job/node-test-commit/19160/

(Edit: 15 green ✔️ in a row, not even any flaky tests … 😍)

benjamingr · Jun 12, 2018

benjamingr approved these changes Jun 12, 2018

View changes

cjihrig · Jun 12, 2018

cjihrig approved these changes Jun 12, 2018

View changes

bnoordhuis · Jun 12, 2018

bnoordhuis approved these changes Jun 12, 2018

View changes

addaleax · Jun 13, 2018

Landed in 8e2d33f, aa2304b

Releasers: The first commit can be backported to LTS.

addaleax added the worker label Jun 9, 2018

addaleax requested a review from danbev Jun 9, 2018

nodejs-github-bot added C++ lib / src labels Jun 9, 2018

addaleax added the work in progress (WIP) label Jun 10, 2018

addaleax force-pushed the addaleax:worker-handle-leaks branch from 197b8c3 to 542e4a4 Jun 10, 2018

addaleax force-pushed the addaleax:worker-handle-leaks branch from 42efda4 to 24e7701 Jun 11, 2018

addaleax added author ready and removed work in progress (WIP) labels Jun 11, 2018

addaleax closed this Jun 13, 2018

addaleax deleted the addaleax:worker-handle-leaks branch Jun 13, 2018

TimothyGu removed the author ready label Jun 15, 2018

targos referenced this pull request Jun 19, 2018
Merged
v10.5.0 proposal #21400

This was referenced Jun 20, 2018

Closed

2018-06-20 Version 10.5.0 (Current) @targos beevelop/docker-android-nodejs#216

Closed

2018-06-20 Version 10.5.0 (Current) @targos beevelop/docker-nodejs#201

alexeykuzmin added a commit to electron/node that referenced this pull request Sep 3, 2018

fixup! chore: add GN build files
- nodejs/node#21238 - nodejs/node#20987

28428d3

nodejs/node

Join GitHub today

src,worker: display remaining handles if `uv_loop_close` fails #21238

Conversation

addaleax commented Jun 9, 2018

Checklist

addaleax added the worker label Jun 9, 2018

addaleax requested a review from danbev Jun 9, 2018

This comment has been minimized.

nodejs-github-bot commented Jun 9, 2018

nodejs-github-bot added C++ lib / src labels Jun 9, 2018

This comment has been minimized.

addaleax commented Jun 10, 2018

addaleax added the work in progress (WIP) label Jun 10, 2018

addaleax force-pushed the addaleax:worker-handle-leaks branch from 197b8c3 to 542e4a4 Jun 10, 2018

TimothyGu approved these changes Jun 10, 2018 View changes

This comment has been minimized.

TimothyGu commented Jun 10, 2018

jasnell approved these changes Jun 10, 2018 View changes

devsnek approved these changes Jun 10, 2018 View changes

This comment has been minimized.

addaleax commented Jun 11, 2018

This comment has been minimized.

Trott commented Jun 11, 2018

This comment has been minimized.

Trott commented Jun 11, 2018

This comment has been minimized.

addaleax commented Jun 11, 2018

bnoordhuis approved these changes Jun 11, 2018 View changes

bnoordhuis left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

addaleax added some commits Jun 7, 2018

addaleax force-pushed the addaleax:worker-handle-leaks branch from 42efda4 to 24e7701 Jun 11, 2018

This comment has been minimized.

addaleax commented Jun 11, 2018 • edited

addaleax added author ready and removed work in progress (WIP) labels Jun 11, 2018

benjamingr approved these changes Jun 12, 2018 View changes

cjihrig approved these changes Jun 12, 2018 View changes

bnoordhuis approved these changes Jun 12, 2018 View changes

This comment has been minimized.

addaleax commented Jun 13, 2018

addaleax closed this Jun 13, 2018

addaleax deleted the addaleax:worker-handle-leaks branch Jun 13, 2018

addaleax added a commit that referenced this pull request Jun 13, 2018

addaleax added a commit that referenced this pull request Jun 13, 2018

targos added a commit that referenced this pull request Jun 13, 2018

targos added a commit that referenced this pull request Jun 13, 2018

dna2github added a commit to dna2fork/node that referenced this pull request Jun 14, 2018

dna2github added a commit to dna2fork/node that referenced this pull request Jun 14, 2018

TimothyGu removed the author ready label Jun 15, 2018

targos referenced this pull request Jun 19, 2018

This was referenced Jun 20, 2018

calebeby added a commit to Pigmice2733/scouting-frontend that referenced this pull request Jun 20, 2018

This was referenced Jun 20, 2018

alexeykuzmin added a commit to electron/node that referenced this pull request Sep 3, 2018

addaleax force-pushed the addaleax:worker-handle-leaks branch from `197b8c3` to `542e4a4` Jun 10, 2018

TimothyGu approved these changes Jun 10, 2018

View changes

jasnell approved these changes Jun 10, 2018

View changes

devsnek approved these changes Jun 10, 2018

View changes

bnoordhuis approved these changes Jun 11, 2018

View changes

addaleax force-pushed the addaleax:worker-handle-leaks branch from `42efda4` to `24e7701` Jun 11, 2018

addaleax commented Jun 11, 2018 •

edited

benjamingr approved these changes Jun 12, 2018

View changes

cjihrig approved these changes Jun 12, 2018

View changes

bnoordhuis approved these changes Jun 12, 2018

View changes