Skip to content

Commit

Permalink
src: don't call uv_run() after 'exit' event
Browse files Browse the repository at this point in the history
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: nodejs#12322
PR-URL: nodejs#12344
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
bnoordhuis authored and danbev committed May 2, 2017
1 parent d7ea036 commit 3fb8537
Show file tree
Hide file tree
Showing 6 changed files with 34 additions and 95 deletions.
2 changes: 2 additions & 0 deletions src/debug-agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ void Agent::WorkerRun() {

// Clean-up persistent
api_.Reset();

env.CleanupHandles();
}
isolate->Dispose();
}
Expand Down
22 changes: 0 additions & 22 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -201,28 +201,6 @@ inline Environment::Environment(IsolateData* isolate_data,
inline Environment::~Environment() {
v8::HandleScope handle_scope(isolate());

while (HandleCleanup* hc = handle_cleanup_queue_.PopFront()) {
handle_cleanup_waiting_++;
hc->cb_(this, hc->handle_, hc->arg_);
delete hc;
}

while (handle_cleanup_waiting_ != 0)
uv_run(event_loop(), UV_RUN_ONCE);

// Closing the destroy_ids_idle_handle_ within the handle cleanup queue
// prevents the async wrap destroy hook from being called.
uv_handle_t* handle =
reinterpret_cast<uv_handle_t*>(&destroy_ids_idle_handle_);
handle->data = this;
handle_cleanup_waiting_ = 1;
uv_close(handle, [](uv_handle_t* handle) {
static_cast<Environment*>(handle->data)->FinishHandleCleanup(handle);
});

while (handle_cleanup_waiting_ != 0)
uv_run(event_loop(), UV_RUN_ONCE);

context()->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex,
nullptr);
#define V(PropertyName, TypeName) PropertyName ## _.Reset();
Expand Down
24 changes: 24 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,30 @@ void Environment::Start(int argc,
LoadAsyncWrapperInfo(this);
}

void Environment::CleanupHandles() {
while (HandleCleanup* hc = handle_cleanup_queue_.PopFront()) {
handle_cleanup_waiting_++;
hc->cb_(this, hc->handle_, hc->arg_);
delete hc;
}

while (handle_cleanup_waiting_ != 0)
uv_run(event_loop(), UV_RUN_ONCE);

// Closing the destroy_ids_idle_handle_ within the handle cleanup queue
// prevents the async wrap destroy hook from being called.
uv_handle_t* handle =
reinterpret_cast<uv_handle_t*>(&destroy_ids_idle_handle_);
handle->data = this;
handle_cleanup_waiting_ = 1;
uv_close(handle, [](uv_handle_t* handle) {
static_cast<Environment*>(handle->data)->FinishHandleCleanup(handle);
});

while (handle_cleanup_waiting_ != 0)
uv_run(event_loop(), UV_RUN_ONCE);
}

void Environment::StartProfilerIdleNotifier() {
uv_prepare_start(&idle_prepare_handle_, [](uv_prepare_t* handle) {
Environment* env = ContainerOf(&Environment::idle_prepare_handle_, handle);
Expand Down
1 change: 1 addition & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,7 @@ class Environment {
const char* const* exec_argv,
bool start_profiler_idle_notifier);
void AssignToContext(v8::Local<v8::Context> context);
void CleanupHandles();

void StartProfilerIdleNotifier();
void StopProfilerIdleNotifier();
Expand Down
73 changes: 0 additions & 73 deletions test/parallel/test-async-wrap-throw-from-callback.js

This file was deleted.

7 changes: 7 additions & 0 deletions test/parallel/test-process-exit-GH-12322.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';
require('../common');

process.on('exit', () => {
setTimeout(process.abort, 0); // Should not run.
for (const start = Date.now(); Date.now() - start < 10; /* Empty. */);
});

0 comments on commit 3fb8537

Please sign in to comment.