From bd891dae3819b25fb938c1d73067b83d276c87e9 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sat, 22 Oct 2022 20:13:50 +0900 Subject: [PATCH] src: simplify exit code accesses This simplifies getting the exit code which is set through `process.exitCode` by removing manually reading the JS property from the native side. Signed-off-by: Daeyeon Jeong --- lib/internal/bootstrap/node.js | 7 +++++++ src/api/hooks.cc | 38 ++++++++++++---------------------- src/env-inl.h | 8 +++++++ src/env.h | 5 +++++ src/env_properties.h | 4 ++-- src/node_errors.cc | 12 +++-------- src/node_process_object.cc | 37 +++++++++++++++++++++++++++++++++ 7 files changed, 75 insertions(+), 36 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 571888ebc8d6cbc..7c47d927290bae2 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -83,6 +83,9 @@ const { exiting_aliased_Uint32Array, getHiddenValue, } = internalBinding('util'); +const { + exit_code_symbol: kExitCode, +} = internalBinding('symbols'); setupProcessObject(); @@ -123,6 +126,10 @@ process._exiting = false; value = code; } validateInteger(value, 'code'); + process[kExitCode] = value; + } else { + // unset exit code + process[kExitCode] = code; } exitCode = code; }, diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 04fc00c394404b4..5083312411c9bc0 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -36,18 +36,14 @@ Maybe EmitProcessBeforeExit(Environment* env) { if (!env->destroy_async_id_list()->empty()) AsyncWrap::DestroyAsyncIdsCallback(env); - HandleScope handle_scope(env->isolate()); + Isolate* isolate = env->isolate(); + HandleScope handle_scope(isolate); Local context = env->context(); Context::Scope context_scope(context); - Local exit_code_v; - if (!env->process_object()->Get(context, env->exit_code_string()) - .ToLocal(&exit_code_v)) return Nothing(); - - Local exit_code; - if (!exit_code_v->ToInteger(context).ToLocal(&exit_code)) { - return Nothing(); - } + Local exit_code = v8::Integer::New( + isolate, + env->exit_code().value_or(static_cast(ExitCode::kNoFailure))); return ProcessEmit(env, "beforeExit", exit_code).IsEmpty() ? Nothing() : Just(true); @@ -67,27 +63,19 @@ Maybe EmitProcessExitInternal(Environment* env) { HandleScope handle_scope(isolate); Local context = env->context(); Context::Scope context_scope(context); - Local process_object = env->process_object(); - // TODO(addaleax): It might be nice to share process.exitCode via - // getter/setter pairs that pass data directly to the native side, so that we - // don't manually have to read and write JS properties here. These getters - // could use e.g. a typed array for performance. env->set_exiting(true); - Local exit_code = env->exit_code_string(); - Local code_v; - int code; - if (!process_object->Get(context, exit_code).ToLocal(&code_v) || - !code_v->Int32Value(context).To(&code) || - ProcessEmit(env, "exit", Integer::New(isolate, code)).IsEmpty() || - // Reload exit code, it may be changed by `emit('exit')` - !process_object->Get(context, exit_code).ToLocal(&code_v) || - !code_v->Int32Value(context).To(&code)) { + const std::optional& exit_code = env->exit_code(); + const int no_failure = static_cast(ExitCode::kNoFailure); + + if (ProcessEmit( + env, "exit", Integer::New(isolate, exit_code.value_or(no_failure))) + .IsEmpty()) { return Nothing(); } - - return Just(static_cast(code)); + // Reload exit code, it may be changed by `emit('exit')` + return Just(static_cast(exit_code.value_or(no_failure))); } Maybe EmitProcessExit(Environment* env) { diff --git a/src/env-inl.h b/src/env-inl.h index 4446b6057d5977d..f212a891f675ef4 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -371,6 +371,14 @@ inline AliasedUint32Array& Environment::exiting() { return exiting_; } +inline void Environment::set_exit_code(const std::optional value) { + exit_code_ = value; +} + +inline const std::optional& Environment::exit_code() const { + return exit_code_; +} + inline void Environment::set_abort_on_uncaught_exception(bool value) { options_->abort_on_uncaught_exception = value; } diff --git a/src/env.h b/src/env.h index a5b15863574a56a..db70e8d33f2557f 100644 --- a/src/env.h +++ b/src/env.h @@ -54,6 +54,7 @@ #include #include #include +#include #include #include #include @@ -746,6 +747,9 @@ class Environment : public MemoryRetainer { inline void set_exiting(bool value); inline AliasedUint32Array& exiting(); + inline void set_exit_code(const std::optional value); + inline const std::optional& exit_code() const; + // This stores whether the --abort-on-uncaught-exception flag was passed // to Node. inline bool abort_on_uncaught_exception() const; @@ -1102,6 +1106,7 @@ class Environment : public MemoryRetainer { uint32_t function_id_counter_ = 0; AliasedUint32Array exiting_; + std::optional exit_code_; AliasedUint32Array should_abort_on_uncaught_toggle_; int should_not_abort_scope_counter_ = 0; diff --git a/src/env_properties.h b/src/env_properties.h index c708518efa293fe..9732d7189b24d65 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -41,7 +41,8 @@ V(owner_symbol, "owner_symbol") \ V(onpskexchange_symbol, "onpskexchange") \ V(resource_symbol, "resource_symbol") \ - V(trigger_async_id_symbol, "trigger_async_id_symbol") + V(trigger_async_id_symbol, "trigger_async_id_symbol") \ + V(exit_code_symbol, "exit_code_symbol") // Strings are per-isolate primitives but Environment proxies them // for the sake of convenience. Strings should be ASCII-only. @@ -114,7 +115,6 @@ V(errno_string, "errno") \ V(error_string, "error") \ V(exchange_string, "exchange") \ - V(exit_code_string, "exitCode") \ V(expire_string, "expire") \ V(exponent_string, "exponent") \ V(exports_string, "exports") \ diff --git a/src/node_errors.cc b/src/node_errors.cc index c400d74d23bbbd7..57783b6ddc1fd05 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -1151,15 +1151,9 @@ void TriggerUncaughtException(Isolate* isolate, RunAtExit(env); // If the global uncaught exception handler sets process.exitCode, - // exit with that code. Otherwise, exit with 1. - Local exit_code = env->exit_code_string(); - Local code; - if (process_object->Get(env->context(), exit_code).ToLocal(&code) && - code->IsInt32()) { - env->Exit(static_cast(code.As()->Value())); - } else { - env->Exit(ExitCode::kGenericUserError); - } + // exit with that code. Otherwise, exit with `ExitCode::kGenericUserError`. + env->Exit(static_cast(env->exit_code().value_or( + static_cast(ExitCode::kGenericUserError)))); } void TriggerUncaughtException(Isolate* isolate, const v8::TryCatch& try_catch) { diff --git a/src/node_process_object.cc b/src/node_process_object.cc index 6822601a47d0bed..3b7d40e34417af3 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -14,6 +14,8 @@ namespace node { using v8::Context; using v8::DEFAULT; +using v8::DontDelete; +using v8::DontEnum; using v8::EscapableHandleScope; using v8::Function; using v8::FunctionCallbackInfo; @@ -26,6 +28,7 @@ using v8::Name; using v8::NewStringType; using v8::None; using v8::Object; +using v8::PropertyAttribute; using v8::PropertyCallbackInfo; using v8::SideEffectType; using v8::String; @@ -73,6 +76,27 @@ static void DebugPortSetter(Local property, host_port->set_port(static_cast(port)); } +static void ExitCodeGetter(Local property, + const PropertyCallbackInfo& info) { + Environment* env = Environment::GetCurrent(info); + std::optional maybe_exit_code = env->exit_code(); + if (maybe_exit_code) { + info.GetReturnValue().Set(*maybe_exit_code); + } +} + +static void ExitCodeSetter(Local property, + Local value, + const PropertyCallbackInfo& info) { + Environment* env = Environment::GetCurrent(info); + if (value->IsNullOrUndefined()) { + return env->set_exit_code(std::nullopt); + } + env->set_exit_code( + value->Int32Value(env->context()) + .FromMaybe(static_cast(ExitCode::kNoFailure))); +} + static void GetParentProcessId(Local property, const PropertyCallbackInfo& info) { info.GetReturnValue().Set(uv_os_getppid()); @@ -216,6 +240,17 @@ void PatchProcessObject(const FunctionCallbackInfo& args) { env->owns_process_state() ? DebugPortSetter : nullptr, Local()) .FromJust()); + + // process[exit_code_symbol] + CHECK(process + ->SetAccessor(context, + env->exit_code_symbol(), + ExitCodeGetter, + ExitCodeSetter, + Local(), + DEFAULT, + DontEnum) + .FromJust()); } void RegisterProcessExternalReferences(ExternalReferenceRegistry* registry) { @@ -225,6 +260,8 @@ void RegisterProcessExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(DebugPortGetter); registry->Register(ProcessTitleSetter); registry->Register(ProcessTitleGetter); + registry->Register(ExitCodeSetter); + registry->Register(ExitCodeGetter); } } // namespace node