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: https://github.com/nodejs/node/issues/12322
PR-URL: https://github.com/nodejs/node/pull/12344
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

src/debug-agent.cc

@@ -184,6 +184,8 @@ void Agent::WorkerRun() {
 
     // Clean-up persistent
     api_.Reset();
+
+    env.CleanupHandles();
   }
   isolate->Dispose();
 }

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<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();

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

src/env.h

@@ -440,6 +440,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();

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

@@ -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}`);
-    });
-  });
-}

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. */);
+});