Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 4a54ebc

Browse filesBrowse files
addaleaxtargos
addaleax
authored and
targos
committed
worker,src: display remaining handles if uv_loop_close fails
Right now, we crash the process if there are handles remaining on the event loop when we exit (except for the main thread). This does not provide a lot of information about causes, though; in particular, we don’t show which handles are pending and who own them. This patch adds debug output to these cases to help with the situation. PR-URL: #21238 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
1 parent aa468ab commit 4a54ebc
Copy full SHA for 4a54ebc

File tree

Expand file treeCollapse file tree

11 files changed

+262
-15
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

11 files changed

+262
-15
lines changed
Open diff view settings
Collapse file

‎src/debug_utils.cc‎

Copy file name to clipboardExpand all lines: src/debug_utils.cc
+52-7Lines changed: 52 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -97,11 +97,11 @@ class Win32SymbolDebuggingContext final : public NativeSymbolDebuggingContext {
9797
public:
9898
Win32SymbolDebuggingContext() {
9999
current_process_ = GetCurrentProcess();
100-
USE(SymInitialize(process, nullptr, true));
100+
USE(SymInitialize(current_process_, nullptr, true));
101101
}
102102

103103
~Win32SymbolDebuggingContext() {
104-
USE(SymCleanup(process));
104+
USE(SymCleanup(current_process_));
105105
}
106106

107107
SymbolInfo LookupSymbol(void* address) override {
@@ -114,16 +114,16 @@ class Win32SymbolDebuggingContext final : public NativeSymbolDebuggingContext {
114114
info->SizeOfStruct = sizeof(SYMBOL_INFO);
115115

116116
SymbolInfo ret;
117-
const bool have_info = SymFromAddr(process,
117+
const bool have_info = SymFromAddr(current_process_,
118118
reinterpret_cast<DWORD64>(address),
119119
nullptr,
120120
info);
121121
if (have_info && strlen(info->Name) == 0) {
122122
if (UnDecorateSymbolName(info->Name,
123-
demangled_,
124-
sizeof(demangled_),
123+
demangled,
124+
sizeof(demangled),
125125
UNDNAME_COMPLETE)) {
126-
ret.name = demangled_;
126+
ret.name = demangled;
127127
} else {
128128
ret.name = info->Name;
129129
}
@@ -135,7 +135,7 @@ class Win32SymbolDebuggingContext final : public NativeSymbolDebuggingContext {
135135
bool IsMapped(void* address) override {
136136
MEMORY_BASIC_INFORMATION info;
137137

138-
if (VirtualQuery(address, &info, sizeof(info)) != info)
138+
if (VirtualQuery(address, &info, sizeof(info)) != sizeof(info))
139139
return false;
140140

141141
return info.State == MEM_COMMIT && info.Protect != 0;
@@ -149,6 +149,7 @@ class Win32SymbolDebuggingContext final : public NativeSymbolDebuggingContext {
149149
HANDLE current_process_;
150150
};
151151

152+
std::unique_ptr<NativeSymbolDebuggingContext>
152153
NativeSymbolDebuggingContext::New() {
153154
return std::unique_ptr<NativeSymbolDebuggingContext>(
154155
new Win32SymbolDebuggingContext());
@@ -177,4 +178,48 @@ void DumpBacktrace(FILE* fp) {
177178
}
178179
}
179180

181+
void CheckedUvLoopClose(uv_loop_t* loop) {
182+
if (uv_loop_close(loop) == 0) return;
183+
184+
auto sym_ctx = NativeSymbolDebuggingContext::New();
185+
186+
fprintf(stderr, "uv loop at [%p] has active handles\n", loop);
187+
188+
uv_walk(loop, [](uv_handle_t* handle, void* arg) {
189+
auto sym_ctx = static_cast<NativeSymbolDebuggingContext*>(arg);
190+
191+
fprintf(stderr, "[%p] %s\n", handle, uv_handle_type_name(handle->type));
192+
193+
void* close_cb = reinterpret_cast<void*>(handle->close_cb);
194+
fprintf(stderr, "\tClose callback: %p %s\n",
195+
close_cb, sym_ctx->LookupSymbol(close_cb).Display().c_str());
196+
197+
fprintf(stderr, "\tData: %p %s\n",
198+
handle->data, sym_ctx->LookupSymbol(handle->data).Display().c_str());
199+
200+
// We are also interested in the first field of what `handle->data`
201+
// points to, because for C++ code that is usually the virtual table pointer
202+
// and gives us information about the exact kind of object we're looking at.
203+
void* first_field = nullptr;
204+
// `handle->data` might be any value, including `nullptr`, or something
205+
// cast from a completely different type; therefore, check that it’s
206+
// dereferencable first.
207+
if (sym_ctx->IsMapped(handle->data))
208+
first_field = *reinterpret_cast<void**>(handle->data);
209+
210+
if (first_field != nullptr) {
211+
fprintf(stderr, "\t(First field): %p %s\n",
212+
first_field, sym_ctx->LookupSymbol(first_field).Display().c_str());
213+
}
214+
}, sym_ctx.get());
215+
216+
fflush(stderr);
217+
// Finally, abort.
218+
CHECK(0 && "uv_loop_close() while having open handles");
219+
}
220+
180221
} // namespace node
222+
223+
extern "C" void __DumpBacktrace(FILE* fp) {
224+
node::DumpBacktrace(fp);
225+
}
Collapse file

‎src/debug_utils.h‎

Copy file name to clipboardExpand all lines: src/debug_utils.h
+5Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,11 @@ class NativeSymbolDebuggingContext {
103103
virtual int GetStackTrace(void** frames, int count) { return 0; }
104104
};
105105

106+
// Variant of `uv_loop_close` that tries to be as helpful as possible
107+
// about giving information on currently existing handles, if there are any,
108+
// but still aborts the process.
109+
void CheckedUvLoopClose(uv_loop_t* loop);
110+
106111
} // namespace node
107112

108113
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Collapse file

‎src/inspector_io.cc‎

Copy file name to clipboardExpand all lines: src/inspector_io.cc
+5-4Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#include "inspector_socket_server.h"
44
#include "inspector/node_string.h"
55
#include "env-inl.h"
6+
#include "debug_utils.h"
67
#include "node.h"
78
#include "node_crypto.h"
89
#include "node_mutex.h"
@@ -67,14 +68,14 @@ void HandleSyncCloseCb(uv_handle_t* handle) {
6768
*static_cast<bool*>(handle->data) = true;
6869
}
6970

70-
int CloseAsyncAndLoop(uv_async_t* async) {
71+
void CloseAsyncAndLoop(uv_async_t* async) {
7172
bool is_closed = false;
7273
async->data = &is_closed;
7374
uv_close(reinterpret_cast<uv_handle_t*>(async), HandleSyncCloseCb);
7475
while (!is_closed)
7576
uv_run(async->loop, UV_RUN_ONCE);
7677
async->data = nullptr;
77-
return uv_loop_close(async->loop);
78+
CheckedUvLoopClose(async->loop);
7879
}
7980

8081
// Delete main_thread_req_ on async handle close
@@ -281,7 +282,7 @@ void InspectorIo::ThreadMain() {
281282
thread_req_.data = &queue_transport;
282283
if (!server.Start()) {
283284
state_ = State::kError; // Safe, main thread is waiting on semaphore
284-
CHECK_EQ(0, CloseAsyncAndLoop(&thread_req_));
285+
CloseAsyncAndLoop(&thread_req_);
285286
uv_sem_post(&thread_start_sem_);
286287
return;
287288
}
@@ -291,7 +292,7 @@ void InspectorIo::ThreadMain() {
291292
}
292293
uv_run(&loop, UV_RUN_DEFAULT);
293294
thread_req_.data = nullptr;
294-
CHECK_EQ(uv_loop_close(&loop), 0);
295+
CheckedUvLoopClose(&loop);
295296
}
296297

297298
template <typename ActionType>
Collapse file

‎src/node_watchdog.cc‎

Copy file name to clipboardExpand all lines: src/node_watchdog.cc
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
#include "node_watchdog.h"
2323
#include "node_internals.h"
24+
#include "debug_utils.h"
2425
#include <algorithm>
2526

2627
namespace node {
@@ -60,8 +61,7 @@ Watchdog::~Watchdog() {
6061
// UV_RUN_DEFAULT so that libuv has a chance to clean up.
6162
uv_run(loop_, UV_RUN_DEFAULT);
6263

63-
int rc = uv_loop_close(loop_);
64-
CHECK_EQ(0, rc);
64+
CheckedUvLoopClose(loop_);
6565
delete loop_;
6666
loop_ = nullptr;
6767
}
Collapse file

‎src/node_worker.cc‎

Copy file name to clipboardExpand all lines: src/node_worker.cc
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#include "node_worker.h"
2+
#include "debug_utils.h"
23
#include "node_errors.h"
34
#include "node_internals.h"
45
#include "node_buffer.h"
@@ -304,7 +305,7 @@ Worker::~Worker() {
304305
CHECK(stopped_);
305306
CHECK(thread_joined_);
306307
CHECK_EQ(child_port_, nullptr);
307-
CHECK_EQ(uv_loop_close(&loop_), 0);
308+
CheckedUvLoopClose(&loop_);
308309

309310
// This has most likely already happened within the worker thread -- this
310311
// is just in case Worker creation failed early.
Collapse file

‎src/spawn_sync.cc‎

Copy file name to clipboardExpand all lines: src/spawn_sync.cc
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
// USE OR OTHER DEALINGS IN THE SOFTWARE.
2121

2222
#include "spawn_sync.h"
23+
#include "debug_utils.h"
2324
#include "env-inl.h"
2425
#include "string_bytes.h"
2526
#include "util.h"
@@ -528,7 +529,7 @@ void SyncProcessRunner::CloseHandlesAndDeleteLoop() {
528529
if (r < 0)
529530
ABORT();
530531

531-
CHECK_EQ(uv_loop_close(uv_loop_), 0);
532+
CheckedUvLoopClose(uv_loop_);
532533
delete uv_loop_;
533534
uv_loop_ = nullptr;
534535

Collapse file

‎test/abort/test-addon-uv-handle-leak.js‎

Copy file name to clipboard
+97Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
// Flags: --experimental-worker
2+
'use strict';
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const fs = require('fs');
6+
const path = require('path');
7+
const cp = require('child_process');
8+
const { Worker } = require('worker_threads');
9+
10+
// This is a sibling test to test/addons/uv-handle-leak.
11+
12+
const bindingPath = path.resolve(
13+
__dirname, '..', 'addons', 'uv-handle-leak', 'build',
14+
`${common.buildType}/binding.node`);
15+
16+
if (!fs.existsSync(bindingPath))
17+
common.skip('binding not built yet');
18+
19+
if (process.argv[2] === 'child') {
20+
new Worker(`
21+
const binding = require(${JSON.stringify(bindingPath)});
22+
23+
binding.leakHandle();
24+
binding.leakHandle(0);
25+
binding.leakHandle(0x42);
26+
`, { eval: true });
27+
} else {
28+
const child = cp.spawnSync(process.execPath,
29+
['--experimental-worker', __filename, 'child']);
30+
const stderr = child.stderr.toString();
31+
32+
assert.strictEqual(child.stdout.toString(), '');
33+
34+
const lines = stderr.split('\n');
35+
36+
let state = 'initial';
37+
38+
// parse output that is formatted like this:
39+
40+
// uv loop at [0x559b65ed5770] has active handles
41+
// [0x7f2de0018430] timer
42+
// Close callback: 0x7f2df31de220 CloseCallback(uv_handle_s*) [...]
43+
// Data: 0x7f2df33df140 example_instance [...]
44+
// (First field): 0x7f2df33dedc0 vtable for ExampleOwnerClass [...]
45+
// [0x7f2de000b870] timer
46+
// Close callback: 0x7f2df31de220 CloseCallback(uv_handle_s*) [...]
47+
// Data: (nil)
48+
// [0x7f2de000b910] timer
49+
// Close callback: 0x7f2df31de220 CloseCallback(uv_handle_s*) [...]
50+
// Data: 0x42
51+
52+
if (!(common.isFreeBSD ||
53+
common.isAIX ||
54+
(common.isLinux && !common.isGlibc()) ||
55+
common.isWindows)) {
56+
assert(stderr.includes('ExampleOwnerClass'), stderr);
57+
assert(stderr.includes('CloseCallback'), stderr);
58+
assert(stderr.includes('example_instance'), stderr);
59+
}
60+
61+
while (lines.length > 0) {
62+
const line = lines.shift().trim();
63+
64+
switch (state) {
65+
case 'initial':
66+
assert(/^uv loop at \[.+\] has active handles$/.test(line), line);
67+
state = 'handle-start';
68+
break;
69+
case 'handle-start':
70+
if (/Assertion .+ failed/.test(line)) {
71+
state = 'done';
72+
break;
73+
}
74+
assert(/^\[.+\] timer$/.test(line), line);
75+
state = 'close-callback';
76+
break;
77+
case 'close-callback':
78+
assert(/^Close callback:/.test(line), line);
79+
state = 'data';
80+
break;
81+
case 'data':
82+
assert(/^Data: .+$/.test(line), line);
83+
state = 'maybe-first-field';
84+
break;
85+
case 'maybe-first-field':
86+
if (/^\(First field\)$/.test(line)) {
87+
lines.unshift(line);
88+
state = 'handle-start';
89+
break;
90+
}
91+
state = 'maybe-first-field';
92+
break;
93+
case 'done':
94+
break;
95+
}
96+
}
97+
}
Collapse file

‎test/addons/uv-handle-leak/binding.cc‎

Copy file name to clipboard
+48Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
#include <node.h>
2+
#include <v8.h>
3+
#include <uv.h>
4+
5+
using v8::Context;
6+
using v8::FunctionCallbackInfo;
7+
using v8::Isolate;
8+
using v8::Local;
9+
using v8::Object;
10+
using v8::Value;
11+
12+
// Give these things names in the public namespace so that we can see
13+
// them show up in symbol dumps.
14+
void CloseCallback(uv_handle_t* handle) {}
15+
16+
class ExampleOwnerClass {
17+
public:
18+
virtual ~ExampleOwnerClass() {}
19+
};
20+
21+
ExampleOwnerClass example_instance;
22+
23+
void LeakHandle(const FunctionCallbackInfo<Value>& args) {
24+
Isolate* isolate = args.GetIsolate();
25+
Local<Context> context = isolate->GetCurrentContext();
26+
uv_loop_t* loop = node::GetCurrentEventLoop(isolate);
27+
assert(loop != nullptr);
28+
29+
uv_timer_t* leaked_timer = new uv_timer_t;
30+
leaked_timer->close_cb = CloseCallback;
31+
32+
if (args[0]->IsNumber()) {
33+
leaked_timer->data =
34+
reinterpret_cast<void*>(args[0]->IntegerValue(context).FromJust());
35+
} else {
36+
leaked_timer->data = &example_instance;
37+
}
38+
39+
uv_timer_init(loop, leaked_timer);
40+
uv_timer_start(leaked_timer, [](uv_timer_t*){}, 1000, 1000);
41+
uv_unref(reinterpret_cast<uv_handle_t*>(leaked_timer));
42+
}
43+
44+
void Initialize(v8::Local<v8::Object> exports) {
45+
NODE_SET_METHOD(exports, "leakHandle", LeakHandle);
46+
}
47+
48+
NODE_MODULE(NODE_GYP_MODULE_NAME, Initialize)
Collapse file

‎test/addons/uv-handle-leak/binding.gyp‎

Copy file name to clipboard
+9Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
'targets': [
3+
{
4+
'target_name': 'binding',
5+
'defines': [ 'V8_DEPRECATION_WARNINGS=1' ],
6+
'sources': [ 'binding.cc' ]
7+
}
8+
]
9+
}
Collapse file

‎test/addons/uv-handle-leak/test.js‎

Copy file name to clipboard
+23Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
'use strict';
2+
const common = require('../../common');
3+
const bindingPath = require.resolve(`./build/${common.buildType}/binding`);
4+
const binding = require(bindingPath);
5+
6+
// This tests checks that addons may leak libuv handles until process exit.
7+
// It’s really not a good idea to do so, but it tests existing behaviour
8+
// that likely can never be removed for backwards compatibility.
9+
10+
// This has a sibling test in test/abort/ which checks output for failures
11+
// from workers.
12+
13+
try {
14+
// We don’t want to run this in Workers because they do actually enforce
15+
// a clean-exit policy.
16+
const { isMainThread } = require('worker_threads');
17+
if (!isMainThread)
18+
common.skip('Cannot run test in environment with clean-exit policy');
19+
} catch {}
20+
21+
binding.leakHandle();
22+
binding.leakHandle(0);
23+
binding.leakHandle(1);

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.