From 2d4f4ebbd12c8fb53ababb113ddd2a814b045c28 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 11 Apr 2017 20:46:02 +0200 Subject: [PATCH] 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. This change breaks parallel/test-async-wrap-throw-from-callback which I pragmatically resolved by removing the test. In all seriousness, it is scheduled for removal anyway in the upcoming async_wrap revamp so there isn't much point in sinking a lot of time in it. --- src/debug-agent.cc | 2 + src/env-inl.h | 22 ------ src/env.cc | 24 ++++++ src/env.h | 1 + .../test-async-wrap-throw-from-callback.js | 73 ------------------- test/parallel/test-process-exit-GH-12322.js | 7 ++ 6 files changed, 34 insertions(+), 95 deletions(-) delete mode 100644 test/parallel/test-async-wrap-throw-from-callback.js create mode 100644 test/parallel/test-process-exit-GH-12322.js diff --git a/src/debug-agent.cc b/src/debug-agent.cc index 2ce8381fc51462..aef1daa1625b8f 100644 --- a/src/debug-agent.cc +++ b/src/debug-agent.cc @@ -184,6 +184,8 @@ void Agent::WorkerRun() { // Clean-up persistent api_.Reset(); + + env.CleanupHandles(); } isolate->Dispose(); } diff --git a/src/env-inl.h b/src/env-inl.h index 978f8ca819dec1..d4e70ca0d35c9a 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -223,28 +223,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(&destroy_ids_idle_handle_); - handle->data = this; - handle_cleanup_waiting_ = 1; - uv_close(handle, [](uv_handle_t* handle) { - static_cast(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(); diff --git a/src/env.cc b/src/env.cc index 56d7e28a03aebe..b44b435d4e3256 100644 --- a/src/env.cc +++ b/src/env.cc @@ -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(&destroy_ids_idle_handle_); + handle->data = this; + handle_cleanup_waiting_ = 1; + uv_close(handle, [](uv_handle_t* handle) { + static_cast(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); diff --git a/src/env.h b/src/env.h index a719cda2d40c6e..8b158728a9a261 100644 --- a/src/env.h +++ b/src/env.h @@ -440,6 +440,7 @@ class Environment { const char* const* exec_argv, bool start_profiler_idle_notifier); void AssignToContext(v8::Local context); + void CleanupHandles(); void StartProfilerIdleNotifier(); void StopProfilerIdleNotifier(); diff --git a/test/parallel/test-async-wrap-throw-from-callback.js b/test/parallel/test-async-wrap-throw-from-callback.js deleted file mode 100644 index bb820a1b088cd6..00000000000000 --- a/test/parallel/test-async-wrap-throw-from-callback.js +++ /dev/null @@ -1,73 +0,0 @@ -'use strict'; - -const common = require('../common'); -if (!common.hasCrypto) { - common.skip('missing crypto'); - return; -} - -const async_wrap = process.binding('async_wrap'); -const assert = require('assert'); -const crypto = require('crypto'); -const domain = require('domain'); -const spawn = require('child_process').spawn; -const callbacks = [ 'init', 'pre', 'post', 'destroy' ]; -const toCall = process.argv[2]; -let msgCalled = 0; -let msgReceived = 0; - -function init() { - if (toCall === 'init') - throw new Error('init'); -} -function pre() { - if (toCall === 'pre') - throw new Error('pre'); -} -function post() { - if (toCall === 'post') - throw new Error('post'); -} -function destroy() { - if (toCall === 'destroy') - throw new Error('destroy'); -} - -if (typeof process.argv[2] === 'string') { - async_wrap.setupHooks({ init, pre, post, destroy }); - async_wrap.enable(); - - process.on('uncaughtException', common.mustNotCall()); - - const d = domain.create(); - d.on('error', common.mustNotCall()); - d.run(() => { - // Using randomBytes because timers are not yet supported. - crypto.randomBytes(0, common.noop); - }); - -} else { - - process.on('exit', (code) => { - assert.strictEqual(msgCalled, callbacks.length); - assert.strictEqual(msgCalled, msgReceived); - }); - - callbacks.forEach((item) => { - msgCalled++; - - const child = spawn(process.execPath, [__filename, item]); - let errstring = ''; - - child.stderr.on('data', (data) => { - errstring += data.toString(); - }); - - child.on('close', (code) => { - if (errstring.includes('Error: ' + item)) - msgReceived++; - - assert.strictEqual(code, 1, `${item} closed with code ${code}`); - }); - }); -} diff --git a/test/parallel/test-process-exit-GH-12322.js b/test/parallel/test-process-exit-GH-12322.js new file mode 100644 index 00000000000000..890dfd4df7bff4 --- /dev/null +++ b/test/parallel/test-process-exit-GH-12322.js @@ -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. */); +});