From 56600962e80ad0b7d6f9315e8667922620b743a8 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 16 May 2020 11:39:53 +0200 Subject: [PATCH 1/2] src: remove BeforeExit callback list It obscures the fact that there is only a single BeforeExit action. Just call that statically from `EmitBeforeExit()`. --- src/api/hooks.cc | 5 ++++- src/env.cc | 20 -------------------- src/env.h | 3 --- 3 files changed, 4 insertions(+), 24 deletions(-) diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 2dd0cc994ea7f2..d0113670457401 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -30,7 +30,10 @@ void AtExit(Environment* env, void (*cb)(void* arg), void* arg) { } void EmitBeforeExit(Environment* env) { - env->RunBeforeExitCallbacks(); + TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment), + "BeforeExit", env); + if (!env->destroy_async_id_list()->empty()) + AsyncWrap::DestroyAsyncIdsCallback(env); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); diff --git a/src/env.cc b/src/env.cc index b83bd62835128c..2a7166cb233fc9 100644 --- a/src/env.cc +++ b/src/env.cc @@ -373,13 +373,6 @@ Environment::Environment(IsolateData* isolate_data, } destroy_async_id_list_.reserve(512); - BeforeExit( - [](void* arg) { - Environment* env = static_cast(arg); - if (!env->destroy_async_id_list()->empty()) - AsyncWrap::DestroyAsyncIdsCallback(env); - }, - this); performance_state_ = std::make_unique(isolate()); @@ -692,19 +685,6 @@ void Environment::RunCleanup() { } } -void Environment::RunBeforeExitCallbacks() { - TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment), - "BeforeExit", this); - for (ExitCallback before_exit : before_exit_functions_) { - before_exit.cb_(before_exit.arg_); - } - before_exit_functions_.clear(); -} - -void Environment::BeforeExit(void (*cb)(void* arg), void* arg) { - before_exit_functions_.push_back(ExitCallback{cb, arg}); -} - void Environment::RunAtExitCallbacks() { TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment), "AtExit", this); diff --git a/src/env.h b/src/env.h index f4456228849d8b..91adfd0983d75d 100644 --- a/src/env.h +++ b/src/env.h @@ -1102,8 +1102,6 @@ class Environment : public MemoryRetainer { const char* name, v8::FunctionCallback callback); - void BeforeExit(void (*cb)(void* arg), void* arg); - void RunBeforeExitCallbacks(); void AtExit(void (*cb)(void* arg), void* arg); void RunAtExitCallbacks(); @@ -1364,7 +1362,6 @@ class Environment : public MemoryRetainer { void (*cb_)(void* arg); void* arg_; }; - std::list before_exit_functions_; std::list at_exit_functions_; From c46bbdd4f3c3541c7a6ab3904f26c1386dae2f2e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 16 May 2020 11:39:53 +0200 Subject: [PATCH 2/2] wasi,worker: handle termination exception Be careful when emitting the 'beforeExit' event. It's not allowed to call into the runtime when a termination exception is pending. Fixes: https://github.com/nodejs/node/issues/33377 --- src/api/hooks.cc | 13 +++++--- src/node_process_events.cc | 8 +++-- test/wasi/test-wasi-worker-terminate.js | 44 +++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 7 deletions(-) create mode 100644 test/wasi/test-wasi-worker-terminate.js diff --git a/src/api/hooks.cc b/src/api/hooks.cc index d0113670457401..037bdda6f41c82 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -37,11 +37,14 @@ void EmitBeforeExit(Environment* env) { HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); - Local exit_code = env->process_object() - ->Get(env->context(), env->exit_code_string()) - .ToLocalChecked() - ->ToInteger(env->context()) - .ToLocalChecked(); + + Local exit_code_v; + if (!env->process_object()->Get(env->context(), env->exit_code_string()) + .ToLocal(&exit_code_v)) return; + + Local exit_code; + if (!exit_code_v->ToInteger(env->context()).ToLocal(&exit_code)) return; + ProcessEmit(env, "beforeExit", exit_code).ToLocalChecked(); } diff --git a/src/node_process_events.cc b/src/node_process_events.cc index d192ef19b7abad..1b902949e264e0 100644 --- a/src/node_process_events.cc +++ b/src/node_process_events.cc @@ -24,10 +24,14 @@ MaybeLocal ProcessEmit(Environment* env, const char* event, Local message) { // Send message to enable debug in cluster workers - Local process = env->process_object(); Isolate* isolate = env->isolate(); - Local argv[] = {OneByteString(isolate, event), message}; + Local event_string; + if (!String::NewFromOneByte(isolate, reinterpret_cast(event)) + .ToLocal(&event_string)) return MaybeLocal(); + + Local process = env->process_object(); + Local argv[] = {event_string, message}; return MakeCallback(isolate, process, "emit", arraysize(argv), argv, {0, 0}); } diff --git a/test/wasi/test-wasi-worker-terminate.js b/test/wasi/test-wasi-worker-terminate.js new file mode 100644 index 00000000000000..118c2a9d47e411 --- /dev/null +++ b/test/wasi/test-wasi-worker-terminate.js @@ -0,0 +1,44 @@ +// Flags: --experimental-wasi-unstable-preview1 +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { WASI } = require('wasi'); +const { Worker, parentPort } = require('worker_threads'); + +// void _start(void) { for (;;); } +const bytecode = new Uint8Array([ + 0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00, 0x01, 0x04, 0x01, 0x60, + 0x00, 0x00, 0x03, 0x02, 0x01, 0x00, 0x04, 0x05, 0x01, 0x70, 0x01, 0x01, + 0x01, 0x05, 0x03, 0x01, 0x00, 0x02, 0x06, 0x08, 0x01, 0x7f, 0x01, 0x41, + 0x80, 0x88, 0x04, 0x0b, 0x07, 0x13, 0x02, 0x06, 0x6d, 0x65, 0x6d, 0x6f, + 0x72, 0x79, 0x02, 0x00, 0x06, 0x5f, 0x73, 0x74, 0x61, 0x72, 0x74, 0x00, + 0x00, 0x0a, 0x09, 0x01, 0x07, 0x00, 0x03, 0x40, 0x0c, 0x00, 0x0b, 0x0b, + 0x00, 0x10, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x01, 0x09, 0x01, 0x00, 0x06, + 0x5f, 0x73, 0x74, 0x61, 0x72, 0x74, 0x00, 0x2f, 0x09, 0x70, 0x72, 0x6f, + 0x64, 0x75, 0x63, 0x65, 0x72, 0x73, 0x01, 0x0c, 0x70, 0x72, 0x6f, 0x63, + 0x65, 0x73, 0x73, 0x65, 0x64, 0x2d, 0x62, 0x79, 0x01, 0x05, 0x63, 0x6c, + 0x61, 0x6e, 0x67, 0x0f, 0x31, 0x30, 0x2e, 0x30, 0x2e, 0x30, 0x2d, 0x34, + 0x75, 0x62, 0x75, 0x6e, 0x74, 0x75, 0x31 +]); + +// Do not use isMainThread so that this test itself can be run inside a Worker. +if (!process.env.HAS_STARTED_WORKER) { + process.env.HAS_STARTED_WORKER = 1; + const worker = new Worker(__filename); + worker.once('message', (message) => { + assert.strictEqual(message, 'start'); + setTimeout(() => worker.terminate(), common.platformTimeout(50)); + }); +} else { + go(); +} + +async function go() { + const wasi = new WASI({ returnOnExit: true }); + const imports = { wasi_snapshot_preview1: wasi.wasiImport }; + const module = await WebAssembly.compile(bytecode); + const instance = await WebAssembly.instantiate(module, imports); + parentPort.postMessage('start'); + wasi.start(instance); +}