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 <[email protected]>
nodejs · Oct 22, 2022 · eef1c86 · eef1c86
1 parent e43d191
commit eef1c86
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 37 deletions.
diff --git 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
@@ -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<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);
@@ -67,27 +62,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) {

diff --git 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<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;
 }

diff --git a/src/env.h b/src/env.h
@@ -54,6 +54,7 @@
 #include <functional>
 #include <list>
 #include <memory>
+#include <optional>
 #include <ostream>
 #include <set>
 #include <string>
@@ -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;
@@ -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;

diff --git 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
@@ -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) {

diff --git 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<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());
@@ -216,6 +238,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) {
@@ -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