Skip to content

Commit

Permalink
src: simplify exit code accesses
Browse files Browse the repository at this point in the history
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 <daeyeon.dev@gmail.com>
  • Loading branch information
daeyeon committed Oct 22, 2022
1 parent e43d191 commit a107886
Show file tree
Hide file tree
Showing 7 changed files with 75 additions and 36 deletions.
7 changes: 7 additions & 0 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ const {
exiting_aliased_Uint32Array,
getHiddenValue,
} = internalBinding('util');
const {
exit_code_symbol: kExitCode,
} = internalBinding('symbols');

setupProcessObject();

Expand Down Expand Up @@ -123,6 +126,10 @@ process._exiting = false;
value = code;
}
validateInteger(value, 'code');
process[kExitCode] = value;
} else {
// unset exit code
process[kExitCode] = code;
}
exitCode = code;
},
Expand Down
38 changes: 13 additions & 25 deletions src/api/hooks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,14 @@ Maybe<bool> 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> context = env->context();
Context::Scope context_scope(context);

Local<Value> exit_code_v;
if (!env->process_object()->Get(context, env->exit_code_string())
.ToLocal(&exit_code_v)) return Nothing<bool>();

Local<Integer> exit_code;
if (!exit_code_v->ToInteger(context).ToLocal(&exit_code)) {
return Nothing<bool>();
}
Local<Integer> exit_code = v8::Integer::New(
isolate,
env->exit_code().value_or(static_cast<int32_t>(ExitCode::kNoFailure)));

return ProcessEmit(env, "beforeExit", exit_code).IsEmpty() ?
Nothing<bool>() : Just(true);
Expand All @@ -67,27 +63,19 @@ Maybe<ExitCode> EmitProcessExitInternal(Environment* env) {
HandleScope handle_scope(isolate);
Local<Context> context = env->context();
Context::Scope context_scope(context);
Local<Object> 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<String> exit_code = env->exit_code_string();
Local<Value> 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<int32_t>& exit_code = env->exit_code();
const int no_failure = static_cast<int32_t>(ExitCode::kNoFailure);

if (ProcessEmit(
env, "exit", Integer::New(isolate, exit_code.value_or(no_failure)))
.IsEmpty()) {
return Nothing<ExitCode>();
}

return Just(static_cast<ExitCode>(code));
// Reload exit code, it may be changed by `emit('exit')`
return Just(static_cast<ExitCode>(exit_code.value_or(no_failure)));
}

Maybe<int> EmitProcessExit(Environment* env) {
Expand Down
8 changes: 8 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,14 @@ inline AliasedUint32Array& Environment::exiting() {
return exiting_;
}

inline void Environment::set_exit_code(const std::optional<int32_t> value) {
exit_code_ = value;
}

inline const std::optional<int32_t>& Environment::exit_code() const {
return exit_code_;
}

inline void Environment::set_abort_on_uncaught_exception(bool value) {
options_->abort_on_uncaught_exception = value;
}
Expand Down
5 changes: 5 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
#include <functional>
#include <list>
#include <memory>
#include <optional>
#include <ostream>
#include <set>
#include <string>
Expand Down Expand Up @@ -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<int32_t> value);
inline const std::optional<int32_t>& exit_code() const;

// This stores whether the --abort-on-uncaught-exception flag was passed
// to Node.
inline bool abort_on_uncaught_exception() const;
Expand Down Expand Up @@ -1102,6 +1106,7 @@ class Environment : public MemoryRetainer {
uint32_t function_id_counter_ = 0;

AliasedUint32Array exiting_;
std::optional<int32_t> exit_code_;

AliasedUint32Array should_abort_on_uncaught_toggle_;
int should_not_abort_scope_counter_ = 0;
Expand Down
4 changes: 2 additions & 2 deletions src/env_properties.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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") \
Expand Down
12 changes: 3 additions & 9 deletions src/node_errors.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> exit_code = env->exit_code_string();
Local<Value> code;
if (process_object->Get(env->context(), exit_code).ToLocal(&code) &&
code->IsInt32()) {
env->Exit(static_cast<ExitCode>(code.As<Int32>()->Value()));
} else {
env->Exit(ExitCode::kGenericUserError);
}
// exit with that code. Otherwise, exit with `ExitCode::kGenericUserError`.
env->Exit(static_cast<ExitCode>(env->exit_code().value_or(
static_cast<int32_t>(ExitCode::kGenericUserError))));
}

void TriggerUncaughtException(Isolate* isolate, const v8::TryCatch& try_catch) {
Expand Down
37 changes: 37 additions & 0 deletions src/node_process_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -73,6 +76,27 @@ static void DebugPortSetter(Local<Name> property,
host_port->set_port(static_cast<int>(port));
}

static void ExitCodeGetter(Local<Name> property,
const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
std::optional<int32_t> maybe_exit_code = env->exit_code();
if (maybe_exit_code) {
info.GetReturnValue().Set(*maybe_exit_code);
}
}

static void ExitCodeSetter(Local<Name> property,
Local<Value> value,
const PropertyCallbackInfo<void>& 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<int32_t>(ExitCode::kNoFailure)));
}

static void GetParentProcessId(Local<Name> property,
const PropertyCallbackInfo<Value>& info) {
info.GetReturnValue().Set(uv_os_getppid());
Expand Down Expand Up @@ -216,6 +240,17 @@ void PatchProcessObject(const FunctionCallbackInfo<Value>& args) {
env->owns_process_state() ? DebugPortSetter : nullptr,
Local<Value>())
.FromJust());

// process[exit_code_symbol]
CHECK(process
->SetAccessor(context,
env->exit_code_symbol(),
ExitCodeGetter,
ExitCodeSetter,
Local<Value>(),
DEFAULT,
DontEnum)
.FromJust());
}

void RegisterProcessExternalReferences(ExternalReferenceRegistry* registry) {
Expand All @@ -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
Expand Down

0 comments on commit a107886

Please sign in to comment.