Skip to content

src: fix cb scope bugs involved in termination

An inheritor of https://github.com/nodejs/node/pull/45422 and a fix to https://github.com/nodejs/node/issues/43084. I move the original failing test back to the test/parallel/ to increase the chance that it exercises the defect, at which this PR is targeted.

To see the bug in 43084, apply this patch and run the script

diff --git a/src/env.cc b/src/env.cc
index 2f0720d402..5a4d9b2146 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -27,6 +27,8 @@
 #include <iostream>
 #include <limits>
 #include <memory>
+#include <thread> 
+#include <chrono>
 
 namespace node {
 
@@ -907,6 +909,7 @@ void Environment::InitializeLibuv() {
 void Environment::ExitEnv() {
   set_can_call_into_js(false);
   set_stopping(true);
+  std::this_thread::sleep_for(std::chrono::milliseconds(2000));
   isolate_->TerminateExecution();
   SetImmediateThreadsafe([](Environment* env) { uv_stop(env->event_loop()); });
 }
// Flags: --expose-internals

'use strict';
const common = require('../common');
const { Worker } = require('worker_threads');

/**
 * This file is a minimal possible reproduction and an
 * explanation of https://github.com/nodejs/node/issues/43084.
 *
 * It can not trigger the bug consistently because
 * that would require a rare thread execution interleaving
 * condition.
 *
 * test-worker-http2-stream-terminate.js has a bigger chance
 * to trigger the bug.
 */

if (!process.env.HAS_STARTED_WORKER) {
  process.env.HAS_STARTED_WORKER = 1;

  const workerData = new Int32Array(new SharedArrayBuffer(4));
  const w = new Worker(__filename, { workerData });
  w.on('exit', common.mustCall());

  // Wait for the worker to enter the _write() below
  // eslint-disable-next-line
  while (Atomics.load(workerData, 0) !== 1) {}
  w.terminate(); // node::Environment::ExitEnv() is called under the hood

  return;
}

// --------- worker thread ---------

const JSStreamSocket = require('internal/js_stream_socket');
const { Duplex } = require('stream');

class DummyDuplex extends Duplex {
  _read() {
  }

  _write(chunk, encoding, callback) {
    /**
     * Now, this worker thread has a stack looks like:
     * _pthread_start
     * ...
     * node::CheckImmediate
     * ...
     * node::MakeCallback
     * ...
     * v8::internal::FunctionCallbackArguments::Call
     * ...
     * node::MakeCallback
     * ...
     */

    const { workerData } = require('worker_threads');
    Atomics.store(workerData, 0, 1);

    // Make some time for the main thread to enter node::Environment::ExitEnv()
    // eslint-disable-next-line
    for (let i = 0; i < 1000000; i++) {}

    // Just before this function returns, if the main thread
    // 1. called node::Environment::ExitEnv()/set_can_call_into_js(false)
    // 2. but did not called node::Environment::ExitEnv()/TerminateExecution() yet
    // 3. and got suspended by the OS
    // Without the fix, this bug https://github.com/nodejs/node/issues/43084 occurs
    // and emitAfter() in processImmediate() in lib/internal/timers.js complains
    // Error: async hook stack has become corrupted (actual: 6, expected: 5)
  }

  _final(callback) {
  }
}

setImmediate(() => {
  const sock = new JSStreamSocket(new DummyDuplex());
  sock.write(Buffer.alloc(10));
  // JSStreamSocket basically delegate r/w to the Duplex paased to its constructor
  // It is a net.Socket
  // lib/net.js write()
  //  lib/internal/stream_base_commons.js writeGeneric()
  //    handleWriteReq()
  //      handle.writeBuffer()
  //        node::StreamBase::WriteBuffer()
  //          node::StreamBase::Write()
  //            node::JSStream::DoWrite()
  //              call to the above DummyDuplex
});

Merge request reports

Loading