Skip to content

Commit 5ef6000

Browse files
committed
src: don't call uv_run() after 'exit' event
It makes timers and other libuv handles fire intermittently after the 'exit' event, contrary to what the documentation states. Regression introduced in commit aac79df ("src: use stack-allocated Environment instances") from June last year that made the `while (handle_cleanup_waiting_ != 0) uv_run(event_loop(), UV_RUN_ONCE)` loop run unconditionally on exit because it merged CleanupHandles() into the Environment destructor. This change breaks parallel/test-async-wrap-throw-from-callback because the async_wrap idle handle is no longer cleaned up, which I resolved pragmatically by removing the test. In all seriousness, it is being removed in the upcoming async_wrap revamp - it doesn't make sense to sink a lot of time in it now. Fixes: #12322 PR-URL: #12344 Reviewed-By: Colin Ihrig <[email protected]>
1 parent 5d06e5c commit 5ef6000

6 files changed

+34
-95
lines changed

src/debug-agent.cc

+2
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,8 @@ void Agent::WorkerRun() {
184184

185185
// Clean-up persistent
186186
api_.Reset();
187+
188+
env.CleanupHandles();
187189
}
188190
isolate->Dispose();
189191
}

src/env-inl.h

-22
Original file line numberDiff line numberDiff line change
@@ -223,28 +223,6 @@ inline Environment::Environment(IsolateData* isolate_data,
223223
inline Environment::~Environment() {
224224
v8::HandleScope handle_scope(isolate());
225225

226-
while (HandleCleanup* hc = handle_cleanup_queue_.PopFront()) {
227-
handle_cleanup_waiting_++;
228-
hc->cb_(this, hc->handle_, hc->arg_);
229-
delete hc;
230-
}
231-
232-
while (handle_cleanup_waiting_ != 0)
233-
uv_run(event_loop(), UV_RUN_ONCE);
234-
235-
// Closing the destroy_ids_idle_handle_ within the handle cleanup queue
236-
// prevents the async wrap destroy hook from being called.
237-
uv_handle_t* handle =
238-
reinterpret_cast<uv_handle_t*>(&destroy_ids_idle_handle_);
239-
handle->data = this;
240-
handle_cleanup_waiting_ = 1;
241-
uv_close(handle, [](uv_handle_t* handle) {
242-
static_cast<Environment*>(handle->data)->FinishHandleCleanup(handle);
243-
});
244-
245-
while (handle_cleanup_waiting_ != 0)
246-
uv_run(event_loop(), UV_RUN_ONCE);
247-
248226
context()->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex,
249227
nullptr);
250228
#define V(PropertyName, TypeName) PropertyName ## _.Reset();

src/env.cc

+24
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,30 @@ void Environment::Start(int argc,
9292
LoadAsyncWrapperInfo(this);
9393
}
9494

95+
void Environment::CleanupHandles() {
96+
while (HandleCleanup* hc = handle_cleanup_queue_.PopFront()) {
97+
handle_cleanup_waiting_++;
98+
hc->cb_(this, hc->handle_, hc->arg_);
99+
delete hc;
100+
}
101+
102+
while (handle_cleanup_waiting_ != 0)
103+
uv_run(event_loop(), UV_RUN_ONCE);
104+
105+
// Closing the destroy_ids_idle_handle_ within the handle cleanup queue
106+
// prevents the async wrap destroy hook from being called.
107+
uv_handle_t* handle =
108+
reinterpret_cast<uv_handle_t*>(&destroy_ids_idle_handle_);
109+
handle->data = this;
110+
handle_cleanup_waiting_ = 1;
111+
uv_close(handle, [](uv_handle_t* handle) {
112+
static_cast<Environment*>(handle->data)->FinishHandleCleanup(handle);
113+
});
114+
115+
while (handle_cleanup_waiting_ != 0)
116+
uv_run(event_loop(), UV_RUN_ONCE);
117+
}
118+
95119
void Environment::StartProfilerIdleNotifier() {
96120
uv_prepare_start(&idle_prepare_handle_, [](uv_prepare_t* handle) {
97121
Environment* env = ContainerOf(&Environment::idle_prepare_handle_, handle);

src/env.h

+1
Original file line numberDiff line numberDiff line change
@@ -440,6 +440,7 @@ class Environment {
440440
const char* const* exec_argv,
441441
bool start_profiler_idle_notifier);
442442
void AssignToContext(v8::Local<v8::Context> context);
443+
void CleanupHandles();
443444

444445
void StartProfilerIdleNotifier();
445446
void StopProfilerIdleNotifier();

test/parallel/test-async-wrap-throw-from-callback.js

-73
This file was deleted.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
'use strict';
2+
require('../common');
3+
4+
process.on('exit', () => {
5+
setTimeout(process.abort, 0); // Should not run.
6+
for (const start = Date.now(); Date.now() - start < 10; /* Empty. */);
7+
});

0 commit comments

Comments
 (0)