Making node_process and part of bootstrapper.cc an internalBinding

[joyeecheung]

Right now some bindings used to setup the process object are placed in bootstrapper.cc and are put onto a big object during bootstrap, then passed into other smaller lib/internal/process/something.js modules for further setup:

node/src/bootstrapper.cc

Lines 133 to 168 in b416daf

	// The Bootstrapper object is an ephemeral object that is used only during
	// the bootstrap process of the Node.js environment. A reference to the
	// bootstrap object must not be kept around after the bootstrap process
	// completes so that it can be gc'd as soon as possible.
	void SetupBootstrapObject(Environment* env,
	Local<Object> bootstrapper) {
	BOOTSTRAP_METHOD(_setupTraceCategoryState, SetupTraceCategoryState);
	BOOTSTRAP_METHOD(_setupNextTick, SetupNextTick);
	BOOTSTRAP_METHOD(_setupPromises, SetupPromises);
	BOOTSTRAP_METHOD(_chdir, Chdir);
	BOOTSTRAP_METHOD(_cpuUsage, CPUUsage);
	BOOTSTRAP_METHOD(_hrtime, Hrtime);
	BOOTSTRAP_METHOD(_hrtimeBigInt, HrtimeBigInt);
	BOOTSTRAP_METHOD(_memoryUsage, MemoryUsage);
	BOOTSTRAP_METHOD(_rawDebug, RawDebug);
	BOOTSTRAP_METHOD(_umask, Umask);
	#if defined(__POSIX__) && !defined(__ANDROID__) && !defined(__CloudABI__)
	if (env->is_main_thread()) {
	BOOTSTRAP_METHOD(_initgroups, InitGroups);
	BOOTSTRAP_METHOD(_setegid, SetEGid);
	BOOTSTRAP_METHOD(_seteuid, SetEUid);
	BOOTSTRAP_METHOD(_setgid, SetGid);
	BOOTSTRAP_METHOD(_setuid, SetUid);
	BOOTSTRAP_METHOD(_setgroups, SetGroups);
	}
	#endif // __POSIX__ && !defined(__ANDROID__) && !defined(__CloudABI__)
	Local<String> should_abort_on_uncaught_toggle =
	FIXED_ONE_BYTE_STRING(env->isolate(), "_shouldAbortOnUncaughtToggle");
	CHECK(bootstrapper->Set(env->context(),
	should_abort_on_uncaught_toggle,
	env->should_abort_on_uncaught_toggle().GetJSArray())
	.FromJust());
	}
	#undef BOOTSTRAP_METHOD

Note that not all the definitions of these methods are in bootstrapper.cc, some of those are placed in node_process.cc with declarations in node_internals.h, so for example, to bootstrap process.setgid, on the C++ side:

// 1. node_internals.h: declaration
void SetGid(const v8::FunctionCallbackInfo<v8::Value>& args);
// 2. node_process.cc: definition
void SetGid(const FunctionCallbackInfo<Value>& args) { ... }
// 3. bootstrapper.cc: SetupBootstrapObject() put it onto the bootstrap object
void SetupBootstrapObject(...) {
  ...
  BOOTSTRAP_METHOD(_setgid, SetGid);
}
// 4. node.cc: call SetupBootstrapObject() during bootstrap and pass the object into node.js
SetupBootstrapObject(env, bootstrapper);

On the JS side:

// 1. bootstrap/node.js: get this through a big bootstrap object passed from C++
const { _setgid } = bootstrappers;
// 2. bootstrap/node.js: Then it pass this to a method exposed by main_thread_only.js
mainThreadSetup.setupProcessMethods(... _setgid ...);
// 3. main_thread_only.js: wrap this binding with some validation, and
// directly write the method to the process object
function setupProcessMethods(_setgid) {
  if (_setgid !== undefined) { // POSIX
    setupPosixMethods(..._setgid..);
  }
}
function setupPosixMethods (..._setgid..) {
  process.setgid = function setgid(id) {
     return execId(id, 'Group', _setgid);
  };
}

There are several problems with the current setup:

1. The code involved in bootstrapping these small methods span across too many files which make them hard to track down
2. We write to the process object in a separate file, doing this everywhere makes it difficult to figure out the order of write access to the process object and the state of it, which creates difficulty for the v8 snapshot effort
3. Methods like process.setgid is not that frequently used, we don't really need these code to be in the highlight of the bootstrap process

I propose we refactor these process methods to this setup:

// 1. Remove declaration in node_internals.h and declaration bootstrapper.cc
// 2. Make node_process.cc a binding available through `internalBinding('process')`,
//     then SetGid would be available to JS land as `internalBinding('process').setgid`
// node_process.cc: contains both definition and initialization
void SetGid(const FunctionCallbackInfo<Value>& args) { ... }
void Initialize(...) {
  env->SetMethod(target, "setgid", SetGid);
}
NODE_MODULE_CONTEXT_AWARE_INTERNAL(process, node::process::Initialize)

In JS

// 1. main_thread_only.js: return an implementation of setgid in a side-effect free manner, and load
// the binding directly in this file
const binding = internalBinding('process');
exports.hasPosixCredentials = !!binding.setgid;
if (exports.hasPosixCredentials) {  // TODO: this can be passed into the bootstrap script directly
  exports.setgid = function setgid(id) {
     return execId(id, 'Group', binding.setgid);
  };
}
// 2. bootstrap/node.js: re-export the implementation to process by writing to the process object
if (isMainThread) {
  const mainThreadSetup = NativeModule.require('internal/process/main_thread_only');
  if (mainThreadSetup.hasPosixCredentials) {
    process.setgid = mainThreadSetup.setgid;
  }
}

This way, the number of files involved in the implementation is reduced from 4(C++) + 2(JS) to 1(C++) + 2(JS), and the write to the process object are centralized in bootstrap/node.js so it's easier to figure out the state of the process object during the bootstrap.

WDYT?

cc @nodejs/process @addaleax @jasnell @devsnek

[jasnell]

@joyeecheung ... does this need to remain open?

[joyeecheung]

@jasnell I think this is already done. I'll close this.