From eef1c86130c7b311e464b14499a9ebeafed124e9 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sat, 22 Oct 2022 20:13:50 +0900 Subject: [PATCH 1/8] 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 | 39 ++++++++++++---------------------- src/env-inl.h | 8 +++++++ src/env.h | 5 +++++ src/env_properties.h | 4 ++-- src/node_errors.cc | 12 +++-------- src/node_process_object.cc | 35 ++++++++++++++++++++++++++++++ 7 files changed, 73 insertions(+), 37 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 571888ebc8d6cb..7c47d927290bae 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 04fc00c394404b..88c1f258bcff1b 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -16,7 +16,6 @@ using v8::NewStringType; using v8::Nothing; using v8::Object; using v8::String; -using v8::Value; void RunAtExit(Environment* env) { env->RunAtExitCallbacks(); @@ -36,18 +35,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 +62,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 4446b6057d5977..f212a891f675ef 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 a5b15863574a56..db70e8d33f2557 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 c708518efa293f..9732d7189b24d6 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 c400d74d23bbbd..57783b6ddc1fd0 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 6822601a47d0be..6a7e6ef1b540da 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -14,6 +14,7 @@ namespace node { using v8::Context; using v8::DEFAULT; +using v8::DontEnum; using v8::EscapableHandleScope; using v8::Function; using v8::FunctionCallbackInfo; @@ -73,6 +74,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 +238,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 +258,8 @@ void RegisterProcessExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(DebugPortGetter); registry->Register(ProcessTitleSetter); registry->Register(ProcessTitleGetter); + registry->Register(ExitCodeSetter); + registry->Register(ExitCodeGetter); } } // namespace node From 13ef5a2e3def8660f6e4f3145bc01c81776118d5 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sat, 29 Oct 2022 14:57:11 +0900 Subject: [PATCH 2/8] fixup: use typed array Signed-off-by: Daeyeon Jeong --- lib/internal/bootstrap/node.js | 15 +++++++----- src/api/hooks.cc | 13 ++++------ src/env-inl.h | 28 +++++++++++++++------- src/env.cc | 27 +++++++++++++++++++++ src/env.h | 42 ++++++++++++++++++++++++++++---- src/env_properties.h | 6 ++--- src/node_errors.cc | 4 ++-- src/node_internals.h | 2 ++ src/node_process_object.cc | 44 +++++++--------------------------- src/node_snapshotable.cc | 33 +++++++++++++++++++++++++ 10 files changed, 147 insertions(+), 67 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 7c47d927290bae..bf16a514d1db90 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -81,11 +81,9 @@ const { } = require('internal/validators'); const { exiting_aliased_Uint32Array, + exit_info_private_symbol, getHiddenValue, } = internalBinding('util'); -const { - exit_code_symbol: kExitCode, -} = internalBinding('symbols'); setupProcessObject(); @@ -111,6 +109,11 @@ process.domain = null; process._exiting = false; { + // Must match `Environment::ExitInfo::Fields` in `src/env.h`. + const kExitCode = 0; + const kHasExitCode = 1; + const fields = getHiddenValue(process, exit_info_private_symbol); + let exitCode; ObjectDefineProperty(process, 'exitCode', { @@ -126,10 +129,10 @@ process._exiting = false; value = code; } validateInteger(value, 'code'); - process[kExitCode] = value; + fields[kExitCode] = value; + fields[kHasExitCode] = 1; } else { - // unset exit code - process[kExitCode] = code; + fields[kHasExitCode] = 0; } exitCode = code; }, diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 88c1f258bcff1b..bfc5de53e228a0 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -37,12 +37,11 @@ Maybe EmitProcessBeforeExit(Environment* env) { Isolate* isolate = env->isolate(); HandleScope handle_scope(isolate); - Local context = env->context(); - Context::Scope context_scope(context); + Context::Scope context_scope(env->context()); Local exit_code = v8::Integer::New( isolate, - env->exit_code().value_or(static_cast(ExitCode::kNoFailure))); + env->maybe_exit_code(static_cast(ExitCode::kNoFailure))); return ProcessEmit(env, "beforeExit", exit_code).IsEmpty() ? Nothing() : Just(true); @@ -60,21 +59,19 @@ Maybe EmitProcessExitInternal(Environment* env) { // process.emit('exit') Isolate* isolate = env->isolate(); HandleScope handle_scope(isolate); - Local context = env->context(); - Context::Scope context_scope(context); + Context::Scope context_scope(env->context()); env->set_exiting(true); - 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))) + env, "exit", Integer::New(isolate, env->maybe_exit_code(no_failure))) .IsEmpty()) { return Nothing(); } // Reload exit code, it may be changed by `emit('exit')` - return Just(static_cast(exit_code.value_or(no_failure))); + return Just(static_cast(env->maybe_exit_code(no_failure))); } Maybe EmitProcessExit(Environment* env) { diff --git a/src/env-inl.h b/src/env-inl.h index f212a891f675ef..0aa09f55d8422c 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -171,6 +171,18 @@ inline bool TickInfo::has_rejection_to_warn() const { return fields_[kHasRejectionToWarn] == 1; } +inline const AliasedInt32Array& ExitInfo::fields() { + return fields_; +} + +inline bool ExitInfo::has_exit_code() const { + return fields_[kHasExitCode] == 1; +} + +inline int32_t ExitInfo::exit_code() const { + return fields_[kExitCode]; +} + inline Environment* Environment::GetCurrent(v8::Isolate* isolate) { if (UNLIKELY(!isolate->InContext())) return nullptr; v8::HandleScope handle_scope(isolate); @@ -327,6 +339,14 @@ inline TickInfo* Environment::tick_info() { return &tick_info_; } +inline ExitInfo* Environment::exit_info() { + return &exit_info_; +} + +inline int32_t Environment::maybe_exit_code(const int32_t default_code) const { + return exit_info_.has_exit_code() ? exit_info_.exit_code() : default_code; +} + inline uint64_t Environment::timer_base() const { return timer_base_; } @@ -371,14 +391,6 @@ 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.cc b/src/env.cc index d5552ce9133519..dd617397e6831d 100644 --- a/src/env.cc +++ b/src/env.cc @@ -640,6 +640,7 @@ Environment::Environment(IsolateData* isolate_data, async_hooks_(isolate, MAYBE_FIELD_PTR(env_info, async_hooks)), immediate_info_(isolate, MAYBE_FIELD_PTR(env_info, immediate_info)), tick_info_(isolate, MAYBE_FIELD_PTR(env_info, tick_info)), + exit_info_(isolate, MAYBE_FIELD_PTR(env_info, exit_info)), timer_base_(uv_now(isolate_data->event_loop())), exec_argv_(exec_args), argv_(args), @@ -1507,6 +1508,29 @@ void AsyncHooks::FailWithCorruptedAsyncStack(double expected_async_id) { ABORT_NO_BACKTRACE(); } +ExitInfo::SerializeInfo ExitInfo::Serialize(Local context, + SnapshotCreator* creator) { + return {fields_.Serialize(context, creator)}; +} + +void ExitInfo::Deserialize(Local context) { + fields_.Deserialize(context); +} + +std::ostream& operator<<(std::ostream& output, + const ExitInfo::SerializeInfo& i) { + output << "{ " << i.fields << " }"; + return output; +} + +void ExitInfo::MemoryInfo(MemoryTracker* tracker) const { + tracker->TrackField("fields", fields_); +} + +ExitInfo::ExitInfo(Isolate* isolate, const SerializeInfo* info) + : fields_( + isolate, kFieldsCount, info == nullptr ? nullptr : &(info->fields)) {} + void Environment::Exit(ExitCode exit_code) { if (options()->trace_exit) { HandleScope handle_scope(isolate()); @@ -1595,6 +1619,7 @@ EnvSerializeInfo Environment::Serialize(SnapshotCreator* creator) { info.async_hooks = async_hooks_.Serialize(ctx, creator); info.immediate_info = immediate_info_.Serialize(ctx, creator); info.tick_info = tick_info_.Serialize(ctx, creator); + info.exit_info = exit_info_.Serialize(ctx, creator); info.performance_state = performance_state_->Serialize(ctx, creator); info.exiting = exiting_.Serialize(ctx, creator); info.stream_base_state = stream_base_state_.Serialize(ctx, creator); @@ -1637,6 +1662,7 @@ void Environment::DeserializeProperties(const EnvSerializeInfo* info) { async_hooks_.Deserialize(ctx); immediate_info_.Deserialize(ctx); tick_info_.Deserialize(ctx); + exit_info_.Deserialize(ctx); performance_state_->Deserialize(ctx); exiting_.Deserialize(ctx); stream_base_state_.Deserialize(ctx); @@ -1833,6 +1859,7 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("async_hooks", async_hooks_); tracker->TrackField("immediate_info", immediate_info_); tracker->TrackField("tick_info", tick_info_); + tracker->TrackField("exit_info", exit_info_); tracker->TrackField("principal_realm", principal_realm_); // FIXME(joyeecheung): track other fields in Environment. diff --git a/src/env.h b/src/env.h index db70e8d33f2557..4a24085bd84891 100644 --- a/src/env.h +++ b/src/env.h @@ -54,7 +54,6 @@ #include #include #include -#include #include #include #include @@ -460,6 +459,38 @@ class TickInfo : public MemoryRetainer { AliasedUint8Array fields_; }; +class ExitInfo : public MemoryRetainer { + public: + inline const AliasedInt32Array& fields(); + inline bool has_exit_code() const; + inline int32_t exit_code() const; + + SET_MEMORY_INFO_NAME(ExitInfo) + SET_SELF_SIZE(ExitInfo) + void MemoryInfo(MemoryTracker* tracker) const override; + + ExitInfo(const ExitInfo&) = delete; + ExitInfo& operator=(const ExitInfo&) = delete; + ExitInfo(ExitInfo&&) = delete; + ExitInfo& operator=(ExitInfo&&) = delete; + ~ExitInfo() = default; + + struct SerializeInfo { + AliasedBufferIndex fields; + }; + SerializeInfo Serialize(v8::Local context, + v8::SnapshotCreator* creator); + void Deserialize(v8::Local context); + + enum Fields { kExitCode = 0, kHasExitCode, kFieldsCount }; + + private: + friend class Environment; // So we can call the constructor. + explicit ExitInfo(v8::Isolate* isolate, const SerializeInfo* info); + + AliasedInt32Array fields_; +}; + class TrackingTraceStateObserver : public v8::TracingController::TraceStateObserver { public: @@ -515,6 +546,7 @@ struct EnvSerializeInfo { TickInfo::SerializeInfo tick_info; ImmediateInfo::SerializeInfo immediate_info; performance::PerformanceState::SerializeInfo performance_state; + ExitInfo::SerializeInfo exit_info; AliasedBufferIndex exiting; AliasedBufferIndex stream_base_state; AliasedBufferIndex should_abort_on_uncaught_toggle; @@ -727,6 +759,8 @@ class Environment : public MemoryRetainer { inline AsyncHooks* async_hooks(); inline ImmediateInfo* immediate_info(); inline TickInfo* tick_info(); + inline ExitInfo* exit_info(); + inline int32_t maybe_exit_code(const int32_t default_code) const; inline uint64_t timer_base() const; inline std::shared_ptr env_vars(); inline void set_env_vars(std::shared_ptr env_vars); @@ -747,9 +781,6 @@ 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; @@ -1058,6 +1089,7 @@ class Environment : public MemoryRetainer { AsyncHooks async_hooks_; ImmediateInfo immediate_info_; TickInfo tick_info_; + ExitInfo exit_info_; const uint64_t timer_base_; std::shared_ptr env_vars_; bool printed_error_ = false; @@ -1105,8 +1137,8 @@ class Environment : public MemoryRetainer { uint32_t script_id_counter_ = 0; uint32_t function_id_counter_ = 0; + // TODO(daeyeon): merge into `exit_info_` 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 9732d7189b24d6..56d084024f9fc2 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -25,7 +25,8 @@ V(napi_type_tag, "node:napi:type_tag") \ V(napi_wrapper, "node:napi:wrapper") \ V(untransferable_object_private_symbol, "node:untransferableObject") \ - V(exiting_aliased_Uint32Array, "node:exiting_aliased_Uint32Array") + V(exiting_aliased_Uint32Array, "node:exiting_aliased_Uint32Array") \ + V(exit_info_private_symbol, "node:exit_info_private_symbol") // Symbols are per-isolate primitives but Environment proxies them // for the sake of convenience. @@ -41,8 +42,7 @@ V(owner_symbol, "owner_symbol") \ V(onpskexchange_symbol, "onpskexchange") \ V(resource_symbol, "resource_symbol") \ - V(trigger_async_id_symbol, "trigger_async_id_symbol") \ - V(exit_code_symbol, "exit_code_symbol") + V(trigger_async_id_symbol, "trigger_async_id_symbol") // Strings are per-isolate primitives but Environment proxies them // for the sake of convenience. Strings should be ASCII-only. diff --git a/src/node_errors.cc b/src/node_errors.cc index 57783b6ddc1fd0..344ba7a351b894 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -1152,8 +1152,8 @@ void TriggerUncaughtException(Isolate* isolate, // If the global uncaught exception handler sets process.exitCode, // exit with that code. Otherwise, exit with `ExitCode::kGenericUserError`. - env->Exit(static_cast(env->exit_code().value_or( - static_cast(ExitCode::kGenericUserError)))); + env->Exit(static_cast( + env->maybe_exit_code(static_cast(ExitCode::kGenericUserError)))); } void TriggerUncaughtException(Isolate* isolate, const v8::TryCatch& try_catch) { diff --git a/src/node_internals.h b/src/node_internals.h index 9f15a807d02e09..ddffa060960c2b 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -414,6 +414,8 @@ std::ostream& operator<<(std::ostream& output, std::ostream& operator<<(std::ostream& output, const AsyncHooks::SerializeInfo& d); std::ostream& operator<<(std::ostream& output, const SnapshotMetadata& d); +std::ostream& operator<<(std::ostream& output, + const ExitInfo::SerializeInfo& d); namespace performance { std::ostream& operator<<(std::ostream& output, diff --git a/src/node_process_object.cc b/src/node_process_object.cc index 6a7e6ef1b540da..5b64226a936ec3 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -14,7 +14,6 @@ namespace node { using v8::Context; using v8::DEFAULT; -using v8::DontEnum; using v8::EscapableHandleScope; using v8::Function; using v8::FunctionCallbackInfo; @@ -74,27 +73,6 @@ 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()); @@ -123,6 +101,15 @@ MaybeLocal CreateProcessObject(Realm* realm) { return {}; } + // process[exit_info_private_symbol] + if (process + ->SetPrivate(context, + realm->env()->exit_info_private_symbol(), + realm->env()->exit_info()->fields().GetJSArray()) + .IsNothing()) { + return {}; + } + // process.version READONLY_PROPERTY( process, "version", FIXED_ONE_BYTE_STRING(isolate, NODE_VERSION)); @@ -238,17 +225,6 @@ 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) { @@ -258,8 +234,6 @@ void RegisterProcessExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(DebugPortGetter); registry->Register(ProcessTitleSetter); registry->Register(ProcessTitleGetter); - registry->Register(ExitCodeSetter); - registry->Register(ExitCodeGetter); } } // namespace node diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index a5fda9aa5a3833..b893bed11a345d 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -123,6 +123,7 @@ std::ostream& operator<<(std::ostream& output, const EnvSerializeInfo& i) { << "// -- performance_state begins --\n" << i.performance_state << ",\n" << "// -- performance_state ends --\n" + << i.exit_info << ", // exit_info\n" << i.exiting << ", // exiting\n" << i.stream_base_state << ", // stream_base_state\n" << i.should_abort_on_uncaught_toggle @@ -666,6 +667,36 @@ size_t FileWriter::Write( return written_total; } +// Layout of ExitInfo::SerializeInfo +// [ 4/8 bytes ] snapshot index of fields +template <> +ExitInfo::SerializeInfo FileReader::Read() { + Debug("Read()\n"); + + ExitInfo::SerializeInfo result; + result.fields = Read(); + + if (is_debug) { + std::string str = ToStr(result); + Debug("Read() %s\n", str.c_str()); + } + + return result; +} + +template <> +size_t FileWriter::Write(const ExitInfo::SerializeInfo& data) { + if (is_debug) { + std::string str = ToStr(data); + Debug("Write() %s\n", str.c_str()); + } + + size_t written_total = Write(data.fields); + + Debug("Write() wrote %d bytes\n", written_total); + return written_total; +} + // Layout of IsolateDataSerializeInfo // [ 4/8 bytes ] length of primitive_values vector // [ ... ] |length| of primitive_values indices @@ -736,6 +767,7 @@ EnvSerializeInfo FileReader::Read() { result.immediate_info = Read(); result.performance_state = Read(); + result.exit_info = Read(); result.exiting = Read(); result.stream_base_state = Read(); result.should_abort_on_uncaught_toggle = Read(); @@ -757,6 +789,7 @@ size_t FileWriter::Write(const EnvSerializeInfo& data) { written_total += Write(data.immediate_info); written_total += Write( data.performance_state); + written_total += Write(data.exit_info); written_total += Write(data.exiting); written_total += Write(data.stream_base_state); written_total += From 62819b1e4970fa3d6796fe0f268652707112db58 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Fri, 4 Nov 2022 01:19:43 +0900 Subject: [PATCH 3/8] fixup: use inlined typed array Signed-off-by: Daeyeon Jeong --- lib/internal/bootstrap/node.js | 25 +++++++--------- src/api/hooks.cc | 2 +- src/env-inl.h | 30 +++++-------------- src/env.cc | 35 +++------------------- src/env.h | 54 +++++++--------------------------- src/env_properties.h | 1 - src/node_internals.h | 2 -- src/node_process_object.cc | 11 +------ src/node_snapshotable.cc | 37 ++--------------------- 9 files changed, 36 insertions(+), 161 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index bf16a514d1db90..c2ca367dc40d13 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -80,7 +80,6 @@ const { validateInteger, } = require('internal/validators'); const { - exiting_aliased_Uint32Array, exit_info_private_symbol, getHiddenValue, } = internalBinding('util'); @@ -91,31 +90,28 @@ setupGlobalProxy(); setupBuffer(); process.domain = null; + +// process._exiting and process.exitCode { - const exitingAliasedUint32Array = - getHiddenValue(process, exiting_aliased_Uint32Array); + // Must match `Environment::ExitInfoFields` in `src/env.h`. + const kExiting = 0; + const kExitCode = 1; + const kHasExitCode = 2; + const fields = getHiddenValue(process, exit_info_private_symbol); + ObjectDefineProperty(process, '_exiting', { __proto__: null, get() { - return exitingAliasedUint32Array[0] === 1; + return fields[kExiting] === 1; }, set(value) { - exitingAliasedUint32Array[0] = value ? 1 : 0; + fields[kExiting] = value ? 1 : 0; }, enumerable: true, configurable: true, }); -} -process._exiting = false; - -{ - // Must match `Environment::ExitInfo::Fields` in `src/env.h`. - const kExitCode = 0; - const kHasExitCode = 1; - const fields = getHiddenValue(process, exit_info_private_symbol); let exitCode; - ObjectDefineProperty(process, 'exitCode', { __proto__: null, get() { @@ -140,6 +136,7 @@ process._exiting = false; configurable: false, }); } +process._exiting = false; // process.config is serialized config.gypi const nativeModule = internalBinding('builtins'); diff --git a/src/api/hooks.cc b/src/api/hooks.cc index bfc5de53e228a0..236b62c1050e22 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -39,7 +39,7 @@ Maybe EmitProcessBeforeExit(Environment* env) { HandleScope handle_scope(isolate); Context::Scope context_scope(env->context()); - Local exit_code = v8::Integer::New( + Local exit_code = Integer::New( isolate, env->maybe_exit_code(static_cast(ExitCode::kNoFailure))); diff --git a/src/env-inl.h b/src/env-inl.h index 0aa09f55d8422c..e205ab59161093 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -171,18 +171,6 @@ inline bool TickInfo::has_rejection_to_warn() const { return fields_[kHasRejectionToWarn] == 1; } -inline const AliasedInt32Array& ExitInfo::fields() { - return fields_; -} - -inline bool ExitInfo::has_exit_code() const { - return fields_[kHasExitCode] == 1; -} - -inline int32_t ExitInfo::exit_code() const { - return fields_[kExitCode]; -} - inline Environment* Environment::GetCurrent(v8::Isolate* isolate) { if (UNLIKELY(!isolate->InContext())) return nullptr; v8::HandleScope handle_scope(isolate); @@ -339,14 +327,6 @@ inline TickInfo* Environment::tick_info() { return &tick_info_; } -inline ExitInfo* Environment::exit_info() { - return &exit_info_; -} - -inline int32_t Environment::maybe_exit_code(const int32_t default_code) const { - return exit_info_.has_exit_code() ? exit_info_.exit_code() : default_code; -} - inline uint64_t Environment::timer_base() const { return timer_base_; } @@ -384,11 +364,15 @@ inline bool Environment::force_context_aware() const { } inline void Environment::set_exiting(bool value) { - exiting_[0] = value ? 1 : 0; + exit_info_[kExiting] = value ? 1 : 0; +} + +inline int32_t Environment::maybe_exit_code(const int32_t default_code) const { + return exit_info_[kHasExitCode] == 0 ? default_code : exit_info_[kExitCode]; } -inline AliasedUint32Array& Environment::exiting() { - return exiting_; +inline AliasedInt32Array& Environment::exit_info() { + return exit_info_; } inline void Environment::set_abort_on_uncaught_exception(bool value) { diff --git a/src/env.cc b/src/env.cc index dd617397e6831d..6c56d1a5597e0d 100644 --- a/src/env.cc +++ b/src/env.cc @@ -640,12 +640,11 @@ Environment::Environment(IsolateData* isolate_data, async_hooks_(isolate, MAYBE_FIELD_PTR(env_info, async_hooks)), immediate_info_(isolate, MAYBE_FIELD_PTR(env_info, immediate_info)), tick_info_(isolate, MAYBE_FIELD_PTR(env_info, tick_info)), - exit_info_(isolate, MAYBE_FIELD_PTR(env_info, exit_info)), timer_base_(uv_now(isolate_data->event_loop())), exec_argv_(exec_args), argv_(args), exec_path_(GetExecPath(args)), - exiting_(isolate_, 1, MAYBE_FIELD_PTR(env_info, exiting)), + exit_info_(isolate_, 3, MAYBE_FIELD_PTR(env_info, exit_info)), should_abort_on_uncaught_toggle_( isolate_, 1, @@ -1508,29 +1507,6 @@ void AsyncHooks::FailWithCorruptedAsyncStack(double expected_async_id) { ABORT_NO_BACKTRACE(); } -ExitInfo::SerializeInfo ExitInfo::Serialize(Local context, - SnapshotCreator* creator) { - return {fields_.Serialize(context, creator)}; -} - -void ExitInfo::Deserialize(Local context) { - fields_.Deserialize(context); -} - -std::ostream& operator<<(std::ostream& output, - const ExitInfo::SerializeInfo& i) { - output << "{ " << i.fields << " }"; - return output; -} - -void ExitInfo::MemoryInfo(MemoryTracker* tracker) const { - tracker->TrackField("fields", fields_); -} - -ExitInfo::ExitInfo(Isolate* isolate, const SerializeInfo* info) - : fields_( - isolate, kFieldsCount, info == nullptr ? nullptr : &(info->fields)) {} - void Environment::Exit(ExitCode exit_code) { if (options()->trace_exit) { HandleScope handle_scope(isolate()); @@ -1619,9 +1595,8 @@ EnvSerializeInfo Environment::Serialize(SnapshotCreator* creator) { info.async_hooks = async_hooks_.Serialize(ctx, creator); info.immediate_info = immediate_info_.Serialize(ctx, creator); info.tick_info = tick_info_.Serialize(ctx, creator); - info.exit_info = exit_info_.Serialize(ctx, creator); info.performance_state = performance_state_->Serialize(ctx, creator); - info.exiting = exiting_.Serialize(ctx, creator); + info.exit_info = exit_info_.Serialize(ctx, creator); info.stream_base_state = stream_base_state_.Serialize(ctx, creator); info.should_abort_on_uncaught_toggle = should_abort_on_uncaught_toggle_.Serialize(ctx, creator); @@ -1662,9 +1637,8 @@ void Environment::DeserializeProperties(const EnvSerializeInfo* info) { async_hooks_.Deserialize(ctx); immediate_info_.Deserialize(ctx); tick_info_.Deserialize(ctx); - exit_info_.Deserialize(ctx); performance_state_->Deserialize(ctx); - exiting_.Deserialize(ctx); + exit_info_.Deserialize(ctx); stream_base_state_.Deserialize(ctx); should_abort_on_uncaught_toggle_.Deserialize(ctx); @@ -1851,7 +1825,7 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("builtins_without_cache", builtins_without_cache); tracker->TrackField("destroy_async_id_list", destroy_async_id_list_); tracker->TrackField("exec_argv", exec_argv_); - tracker->TrackField("exiting", exiting_); + tracker->TrackField("exit_info", exit_info_); tracker->TrackField("should_abort_on_uncaught_toggle", should_abort_on_uncaught_toggle_); tracker->TrackField("stream_base_state", stream_base_state_); @@ -1859,7 +1833,6 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("async_hooks", async_hooks_); tracker->TrackField("immediate_info", immediate_info_); tracker->TrackField("tick_info", tick_info_); - tracker->TrackField("exit_info", exit_info_); tracker->TrackField("principal_realm", principal_realm_); // FIXME(joyeecheung): track other fields in Environment. diff --git a/src/env.h b/src/env.h index 4a24085bd84891..be8557046d8042 100644 --- a/src/env.h +++ b/src/env.h @@ -459,40 +459,8 @@ class TickInfo : public MemoryRetainer { AliasedUint8Array fields_; }; -class ExitInfo : public MemoryRetainer { - public: - inline const AliasedInt32Array& fields(); - inline bool has_exit_code() const; - inline int32_t exit_code() const; - - SET_MEMORY_INFO_NAME(ExitInfo) - SET_SELF_SIZE(ExitInfo) - void MemoryInfo(MemoryTracker* tracker) const override; - - ExitInfo(const ExitInfo&) = delete; - ExitInfo& operator=(const ExitInfo&) = delete; - ExitInfo(ExitInfo&&) = delete; - ExitInfo& operator=(ExitInfo&&) = delete; - ~ExitInfo() = default; - - struct SerializeInfo { - AliasedBufferIndex fields; - }; - SerializeInfo Serialize(v8::Local context, - v8::SnapshotCreator* creator); - void Deserialize(v8::Local context); - - enum Fields { kExitCode = 0, kHasExitCode, kFieldsCount }; - - private: - friend class Environment; // So we can call the constructor. - explicit ExitInfo(v8::Isolate* isolate, const SerializeInfo* info); - - AliasedInt32Array fields_; -}; - -class TrackingTraceStateObserver : - public v8::TracingController::TraceStateObserver { +class TrackingTraceStateObserver + : public v8::TracingController::TraceStateObserver { public: explicit TrackingTraceStateObserver(Environment* env) : env_(env) {} @@ -546,8 +514,7 @@ struct EnvSerializeInfo { TickInfo::SerializeInfo tick_info; ImmediateInfo::SerializeInfo immediate_info; performance::PerformanceState::SerializeInfo performance_state; - ExitInfo::SerializeInfo exit_info; - AliasedBufferIndex exiting; + AliasedBufferIndex exit_info; AliasedBufferIndex stream_base_state; AliasedBufferIndex should_abort_on_uncaught_toggle; @@ -759,8 +726,6 @@ class Environment : public MemoryRetainer { inline AsyncHooks* async_hooks(); inline ImmediateInfo* immediate_info(); inline TickInfo* tick_info(); - inline ExitInfo* exit_info(); - inline int32_t maybe_exit_code(const int32_t default_code) const; inline uint64_t timer_base() const; inline std::shared_ptr env_vars(); inline void set_env_vars(std::shared_ptr env_vars); @@ -776,10 +741,12 @@ class Environment : public MemoryRetainer { inline void set_force_context_aware(bool value); inline bool force_context_aware() const; - // This is a pseudo-boolean that keeps track of whether the process is - // exiting. + // This contains fields that are a pseudo-boolean that keeps track of whether + // the process is exiting, an integer representing the process exit code, and + // a pseudo-boolean to indicate whether the exit code is undefined. + inline AliasedInt32Array& exit_info(); inline void set_exiting(bool value); - inline AliasedUint32Array& exiting(); + inline int32_t maybe_exit_code(const int32_t default_code) const; // This stores whether the --abort-on-uncaught-exception flag was passed // to Node. @@ -1089,7 +1056,6 @@ class Environment : public MemoryRetainer { AsyncHooks async_hooks_; ImmediateInfo immediate_info_; TickInfo tick_info_; - ExitInfo exit_info_; const uint64_t timer_base_; std::shared_ptr env_vars_; bool printed_error_ = false; @@ -1137,8 +1103,8 @@ class Environment : public MemoryRetainer { uint32_t script_id_counter_ = 0; uint32_t function_id_counter_ = 0; - // TODO(daeyeon): merge into `exit_info_` - AliasedUint32Array exiting_; + enum ExitInfoFields { kExiting = 0, kExitCode, kHasExitCode }; + AliasedInt32Array exit_info_; 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 56d084024f9fc2..02715c166b0f3e 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -25,7 +25,6 @@ V(napi_type_tag, "node:napi:type_tag") \ V(napi_wrapper, "node:napi:wrapper") \ V(untransferable_object_private_symbol, "node:untransferableObject") \ - V(exiting_aliased_Uint32Array, "node:exiting_aliased_Uint32Array") \ V(exit_info_private_symbol, "node:exit_info_private_symbol") // Symbols are per-isolate primitives but Environment proxies them diff --git a/src/node_internals.h b/src/node_internals.h index ddffa060960c2b..9f15a807d02e09 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -414,8 +414,6 @@ std::ostream& operator<<(std::ostream& output, std::ostream& operator<<(std::ostream& output, const AsyncHooks::SerializeInfo& d); std::ostream& operator<<(std::ostream& output, const SnapshotMetadata& d); -std::ostream& operator<<(std::ostream& output, - const ExitInfo::SerializeInfo& d); namespace performance { std::ostream& operator<<(std::ostream& output, diff --git a/src/node_process_object.cc b/src/node_process_object.cc index 5b64226a936ec3..decb41f82c35b1 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -92,20 +92,11 @@ MaybeLocal CreateProcessObject(Realm* realm) { return MaybeLocal(); } - // process[exiting_aliased_Uint32Array] - if (process - ->SetPrivate(context, - realm->env()->exiting_aliased_Uint32Array(), - realm->env()->exiting().GetJSArray()) - .IsNothing()) { - return {}; - } - // process[exit_info_private_symbol] if (process ->SetPrivate(context, realm->env()->exit_info_private_symbol(), - realm->env()->exit_info()->fields().GetJSArray()) + realm->env()->exit_info().GetJSArray()) .IsNothing()) { return {}; } diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index b893bed11a345d..4f56d7cca1c42a 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -124,7 +124,6 @@ std::ostream& operator<<(std::ostream& output, const EnvSerializeInfo& i) { << i.performance_state << ",\n" << "// -- performance_state ends --\n" << i.exit_info << ", // exit_info\n" - << i.exiting << ", // exiting\n" << i.stream_base_state << ", // stream_base_state\n" << i.should_abort_on_uncaught_toggle << ", // should_abort_on_uncaught_toggle\n" @@ -667,36 +666,6 @@ size_t FileWriter::Write( return written_total; } -// Layout of ExitInfo::SerializeInfo -// [ 4/8 bytes ] snapshot index of fields -template <> -ExitInfo::SerializeInfo FileReader::Read() { - Debug("Read()\n"); - - ExitInfo::SerializeInfo result; - result.fields = Read(); - - if (is_debug) { - std::string str = ToStr(result); - Debug("Read() %s\n", str.c_str()); - } - - return result; -} - -template <> -size_t FileWriter::Write(const ExitInfo::SerializeInfo& data) { - if (is_debug) { - std::string str = ToStr(data); - Debug("Write() %s\n", str.c_str()); - } - - size_t written_total = Write(data.fields); - - Debug("Write() wrote %d bytes\n", written_total); - return written_total; -} - // Layout of IsolateDataSerializeInfo // [ 4/8 bytes ] length of primitive_values vector // [ ... ] |length| of primitive_values indices @@ -767,8 +736,7 @@ EnvSerializeInfo FileReader::Read() { result.immediate_info = Read(); result.performance_state = Read(); - result.exit_info = Read(); - result.exiting = Read(); + result.exit_info = Read(); result.stream_base_state = Read(); result.should_abort_on_uncaught_toggle = Read(); result.principal_realm = Read(); @@ -789,8 +757,7 @@ size_t FileWriter::Write(const EnvSerializeInfo& data) { written_total += Write(data.immediate_info); written_total += Write( data.performance_state); - written_total += Write(data.exit_info); - written_total += Write(data.exiting); + written_total += Write(data.exit_info); written_total += Write(data.stream_base_state); written_total += Write(data.should_abort_on_uncaught_toggle); From 375ea8df414da8ce211560a5e8502831e00708db Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sat, 5 Nov 2022 03:06:09 +0900 Subject: [PATCH 4/8] fixup: export field identifiers Signed-off-by: Daeyeon Jeong --- lib/internal/bootstrap/node.js | 7 +++---- src/env.h | 4 +++- src/node_util.cc | 11 +++++++++++ 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index c2ca367dc40d13..f98699dc0363f5 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -82,6 +82,9 @@ const { const { exit_info_private_symbol, getHiddenValue, + kExitCode, + kExiting, + kHasExitCode, } = internalBinding('util'); setupProcessObject(); @@ -93,10 +96,6 @@ process.domain = null; // process._exiting and process.exitCode { - // Must match `Environment::ExitInfoFields` in `src/env.h`. - const kExiting = 0; - const kExitCode = 1; - const kHasExitCode = 2; const fields = getHiddenValue(process, exit_info_private_symbol); ObjectDefineProperty(process, '_exiting', { diff --git a/src/env.h b/src/env.h index be8557046d8042..b41164b809d2fe 100644 --- a/src/env.h +++ b/src/env.h @@ -1038,6 +1038,9 @@ class Environment : public MemoryRetainer { inline void RemoveHeapSnapshotNearHeapLimitCallback(size_t heap_limit); + // Field identifiers for exit_info_ + enum ExitInfoField { kExiting = 0, kExitCode, kHasExitCode }; + private: inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); @@ -1103,7 +1106,6 @@ class Environment : public MemoryRetainer { uint32_t script_id_counter_ = 0; uint32_t function_id_counter_ = 0; - enum ExitInfoFields { kExiting = 0, kExitCode, kHasExitCode }; AliasedInt32Array exit_info_; AliasedUint32Array should_abort_on_uncaught_toggle_; diff --git a/src/node_util.cc b/src/node_util.cc index 58f58478e59944..5d2b4f2155d3d2 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -424,6 +424,17 @@ void Initialize(Local target, V(kRejected); #undef V +#define V(name) \ + target \ + ->Set(context, \ + FIXED_ONE_BYTE_STRING(env->isolate(), #name), \ + Integer::New(env->isolate(), Environment::ExitInfoField::name)) \ + .FromJust() + V(kExiting); + V(kExitCode); + V(kHasExitCode); +#undef V + SetMethodNoSideEffect(context, target, "getHiddenValue", GetHiddenValue); SetMethod(context, target, "setHiddenValue", SetHiddenValue); SetMethodNoSideEffect( From fea188564657e1dd24f221bd58e1c7feecaa7393 Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sat, 5 Nov 2022 03:52:32 +0900 Subject: [PATCH 5/8] fixup: change int32_t to ExitCode Signed-off-by: Daeyeon Jeong --- src/api/hooks.cc | 12 +++++------- src/env-inl.h | 6 ++++-- src/env.h | 2 +- src/node_errors.cc | 3 +-- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 236b62c1050e22..f1ced4efc91608 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -40,8 +40,7 @@ Maybe EmitProcessBeforeExit(Environment* env) { Context::Scope context_scope(env->context()); Local exit_code = Integer::New( - isolate, - env->maybe_exit_code(static_cast(ExitCode::kNoFailure))); + isolate, static_cast(env->exit_code(ExitCode::kNoFailure))); return ProcessEmit(env, "beforeExit", exit_code).IsEmpty() ? Nothing() : Just(true); @@ -63,15 +62,14 @@ Maybe EmitProcessExitInternal(Environment* env) { env->set_exiting(true); - const int no_failure = static_cast(ExitCode::kNoFailure); + Local exit_code = Integer::New( + isolate, static_cast(env->exit_code(ExitCode::kNoFailure))); - if (ProcessEmit( - env, "exit", Integer::New(isolate, env->maybe_exit_code(no_failure))) - .IsEmpty()) { + if (ProcessEmit(env, "exit", exit_code).IsEmpty()) { return Nothing(); } // Reload exit code, it may be changed by `emit('exit')` - return Just(static_cast(env->maybe_exit_code(no_failure))); + return Just(env->exit_code(ExitCode::kNoFailure)); } Maybe EmitProcessExit(Environment* env) { diff --git a/src/env-inl.h b/src/env-inl.h index e205ab59161093..a512d175a29cfe 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -367,8 +367,10 @@ inline void Environment::set_exiting(bool value) { exit_info_[kExiting] = value ? 1 : 0; } -inline int32_t Environment::maybe_exit_code(const int32_t default_code) const { - return exit_info_[kHasExitCode] == 0 ? default_code : exit_info_[kExitCode]; +inline ExitCode Environment::exit_code(const ExitCode default_code) const { + return exit_info_[kHasExitCode] == 0 + ? default_code + : static_cast(exit_info_[kExitCode]); } inline AliasedInt32Array& Environment::exit_info() { diff --git a/src/env.h b/src/env.h index b41164b809d2fe..969cdc3f3fc98f 100644 --- a/src/env.h +++ b/src/env.h @@ -746,7 +746,7 @@ class Environment : public MemoryRetainer { // a pseudo-boolean to indicate whether the exit code is undefined. inline AliasedInt32Array& exit_info(); inline void set_exiting(bool value); - inline int32_t maybe_exit_code(const int32_t default_code) const; + inline ExitCode exit_code(const ExitCode default_code) const; // This stores whether the --abort-on-uncaught-exception flag was passed // to Node. diff --git a/src/node_errors.cc b/src/node_errors.cc index 344ba7a351b894..5408c0166b0e3b 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -1152,8 +1152,7 @@ void TriggerUncaughtException(Isolate* isolate, // If the global uncaught exception handler sets process.exitCode, // exit with that code. Otherwise, exit with `ExitCode::kGenericUserError`. - env->Exit(static_cast( - env->maybe_exit_code(static_cast(ExitCode::kGenericUserError)))); + env->Exit(env->exit_code(ExitCode::kGenericUserError)); } void TriggerUncaughtException(Isolate* isolate, const v8::TryCatch& try_catch) { From a3d9ebf9499b83884f2159746145cbfd51d1001a Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Sat, 5 Nov 2022 16:35:33 +0900 Subject: [PATCH 6/8] fixup: add ExitInfoFieldCount and revert unrelated change Signed-off-by: Daeyeon Jeong --- src/env.cc | 3 ++- src/env.h | 11 ++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/env.cc b/src/env.cc index 6c56d1a5597e0d..abfc79a6b72483 100644 --- a/src/env.cc +++ b/src/env.cc @@ -644,7 +644,8 @@ Environment::Environment(IsolateData* isolate_data, exec_argv_(exec_args), argv_(args), exec_path_(GetExecPath(args)), - exit_info_(isolate_, 3, MAYBE_FIELD_PTR(env_info, exit_info)), + exit_info_( + isolate_, kExitInfoFieldCount, MAYBE_FIELD_PTR(env_info, exit_info)), should_abort_on_uncaught_toggle_( isolate_, 1, diff --git a/src/env.h b/src/env.h index 969cdc3f3fc98f..36d92d8869a50c 100644 --- a/src/env.h +++ b/src/env.h @@ -459,8 +459,8 @@ class TickInfo : public MemoryRetainer { AliasedUint8Array fields_; }; -class TrackingTraceStateObserver - : public v8::TracingController::TraceStateObserver { +class TrackingTraceStateObserver : + public v8::TracingController::TraceStateObserver { public: explicit TrackingTraceStateObserver(Environment* env) : env_(env) {} @@ -1039,7 +1039,12 @@ class Environment : public MemoryRetainer { inline void RemoveHeapSnapshotNearHeapLimitCallback(size_t heap_limit); // Field identifiers for exit_info_ - enum ExitInfoField { kExiting = 0, kExitCode, kHasExitCode }; + enum ExitInfoField { + kExiting = 0, + kExitCode, + kHasExitCode, + kExitInfoFieldCount + }; private: inline void ThrowError(v8::Local (*fun)(v8::Local), From e7b5ae5baea0ac2088418921490037a41f5fb2ca Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Wed, 9 Nov 2022 01:14:32 +0900 Subject: [PATCH 7/8] fixup: add checks of whether JS execution is terminating Signed-off-by: Daeyeon Jeong --- src/api/hooks.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/api/hooks.cc b/src/api/hooks.cc index f1ced4efc91608..31f0c188dfca97 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -39,6 +39,10 @@ Maybe EmitProcessBeforeExit(Environment* env) { HandleScope handle_scope(isolate); Context::Scope context_scope(env->context()); + if (isolate->IsExecutionTerminating()) { + return Nothing(); + } + Local exit_code = Integer::New( isolate, static_cast(env->exit_code(ExitCode::kNoFailure))); @@ -62,6 +66,10 @@ Maybe EmitProcessExitInternal(Environment* env) { env->set_exiting(true); + if (isolate->IsExecutionTerminating()) { + return Nothing(); + } + Local exit_code = Integer::New( isolate, static_cast(env->exit_code(ExitCode::kNoFailure))); From 16fb50fdd1e66a2750d351ca40801d0e0be65b9f Mon Sep 17 00:00:00 2001 From: Daeyeon Jeong Date: Wed, 9 Nov 2022 11:15:19 +0900 Subject: [PATCH 8/8] fixup: use `can_call_into_js()` Signed-off-by: Daeyeon Jeong --- src/api/hooks.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 31f0c188dfca97..65d39e6b9ff921 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -39,7 +39,7 @@ Maybe EmitProcessBeforeExit(Environment* env) { HandleScope handle_scope(isolate); Context::Scope context_scope(env->context()); - if (isolate->IsExecutionTerminating()) { + if (!env->can_call_into_js()) { return Nothing(); } @@ -66,7 +66,7 @@ Maybe EmitProcessExitInternal(Environment* env) { env->set_exiting(true); - if (isolate->IsExecutionTerminating()) { + if (!env->can_call_into_js()) { return Nothing(); }