FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

worker,src: display remaining handles if `uv_loop_close` fails · nodejs/node@4a54ebc · GitHub

/ node Public

Commit 4a54ebc

Browse files
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

11 files changed

Lines changed: 262 additions & 15 deletions

File tree

‎src/debug_utils.cc‎

Lines 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+
}

‎src/debug_utils.h‎

Lines 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

‎src/inspector_io.cc‎

Lines 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>

‎src/node_watchdog.cc‎

Lines 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
}

‎src/node_worker.cc‎

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

‎src/spawn_sync.cc‎

Lines 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

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

Lines 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+
}

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

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

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

Lines 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+
}

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

Lines 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)
Back | FazBrowse Home | New Git URL