src: improve embedder API

[addaleax]

Various improvements towards making the embedder API more usable.

src: make FreeEnvironment() perform all necessary cleanup

Make the calls stop_sub_worker_contexts(), RunCleanup()
part of the public API for easier embedding.

(Note that calling RunAtExit() is idempotent because the
at-exit callback queue is cleared after each call.)

src: fix memory leak in CreateEnvironment when bootstrap fails

src: move worker_context from Environment to IsolateData

Workers are fully in control of their Isolates, and this helps
avoid a problem with later changes to CreateEnvironment()
because now we can run the boostrapping code inside the latter.

src: associate is_main_thread() with worker_context()

In our codebase, the assumption generally is that !is_main_thread()
means that the current Environment belongs to a Node.js Worker thread.

src: align worker and main thread code with embedder API

This addresses some long-standing TODOs by Joyee and me about
making the embedder API more powerful and us less reliant on
internal APIs for creating the main thread and Workers.

src: provide a variant of LoadEnvironment taking a callback

This allows embedders to flexibly control how they start JS code
rather than using third_party_main.

src: add LoadEnvironment() variant taking a string

Allow passing a string as the main module rather than using
the callback variant.

test: re-enable cctest that was commented out

Refs: #31910

src: add unique_ptr equivalent of CreatePlatform

This makes this bit of the embedder situation a bit easier to use.

src: make InitializeNodeWithArgs() official public API

This is a decent replacement for the to-be-deprecated Init() API.

src: add ability to look up platform based on Environment*

This should eventually remove any necessity to use the global-state
GetMainThreadMultiIsolatePlatform().

src: allow non-Node.js TracingControllers

We do not need a Node.js-provided v8::TracingController, generally.
Loosen that restriction in order to make it easier for embedders
to provide their own subclass of v8::TracingController,
or none at all.

Refs: electron/electron@9c36576

src: fix what a dispose without checking

If created platform with CreatePlatform, the crash occurs because
it does not check if it was initialized to v8_platform
when DisposePlatform was called.

Refs: #31260

fixup! src: fix what a dispose without checking

src: shutdown platform from FreePlatform()

There is currently no way to properly do this.

src,test: add full-featured embedder API test

doc: add basic embedding example documentation

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[joyeecheung]

I am not sure whether we should expose this to the API..this being optional seems rather like an implementation detail? Would any embedder actually care about this (instead of some higher level setting that suits the use cases as a whole)?

[addaleax]

I am not sure whether we should expose this to the API..this being optional seems rather like an implementation detail?

Is there any downside? In the worst case we can just ignore it, right?

Would any embedder actually care about this (instead of some higher level setting that suits the use cases as a whole)?

Can you explain what you mean by higher level setting?

But yeah, I don’t know, it’s not all that important I think. And I think we’ve also thought about maybe just always turning this option on in the past.

[joyeecheung]

Whar happens if an embedder uses kDoNotStart after passing --prof to ProcessGlobalArgs and then tries to use the CPU profiler (resulting in incorrect profiles?), or uses kStart without passing --prof (no-ops?)? At least for the internal use case it only make sense if you inherit this from the parent environment, then why not just take a parent environment as an argument (races? but how?)?

[addaleax]

Whar happens if an embedder uses kDoNotStart after passing --prof to ProcessGlobalArgs and then tries to use the CPU profiler (resulting in incorrect profiles?), or uses kStart without passing --prof (no-ops?)? At least for the internal use case it only make sense if you inherit this from the parent environment, then why not just take a parent environment as an argument (races? but how?)?

Hm … so, I thought the V8 API CPUProfiler also needs idle notifications, but I’ve looked at V8CpuProfilerConnection and it doesn’t appear to be used?

I’m okay with dropping this and always starting the idle notifier for embedder use cases if you prefer. I don’t think we should be making the assumption that there’s always something that could be referred to as a “parent” environment, though.

[joyeecheung]

I was referring to this

node/src/node.cc

Lines 721 to 726 in 0f58bfd

	// Block SIGPROF signals when sleeping in epoll_wait/kevent/etc. Avoids the
	// performance penalty of frequent EINTR wakeups when the profiler is running.
	// Only do this for v8.log profiling, as it breaks v8::CpuProfiler users.
	if (per_process::v8_is_profiling) {
	uv_loop_configure(uv_default_loop(), UV_LOOP_BLOCK_SIGNAL, SIGPROF);
	}

I think only keeping the default behavior (simply using per_process::v8_is_profiling) probably makes more sense. I suppose you'd want it to be consistent with whether--prof is ever passed to ProcessGlobalArgs() (other wise it either breaks or is a noop), and that's exactly reflected by per_process::v8_is_profiling.

[addaleax]

I’m removing this and switched to always using per_process::v8_is_profiling – I guess in that case InitializeLibuv() also shouldn’t take an argument?

(Will take a while to push because I’m rebasing instead of pushing fixup commits and I want to make sure everything still compiles/passes after this change)

[joyeecheung]

This currently runs the preparation for main thread execution - which means creating two environments with this flag is going to be kind of wonky.

Is this the best way to divide the options? It also seems possible to do this preparation step in a method and the user can choose to call it themselves. The flags here seem to anchor on the decision that Environment should be the primary abstraction for embedders to interact with the Node.js instance, but I believe previously it is more like an opaque structure being passed around (albeit being hacked a lot in the wild)?

My original plan is to make an abstraction over Worker and NodeMainInstance somehow, and let the users interact with the instance primarily through that. Then Environment would become more of an implementation detail that we are free to refactor with. AFAICT, the embedding use cases of Node.js mostly differ on whether the embdder wants to own the platform, or the event loop, etc. and I think making options out of those needs first and add a few more setters for lower-level options when they are not flexible enough instead of exposing low-level options from the start would result in a more maintainable API. That would also help us add more cctests for different scenarios without sneaking into the internal namespace.

[addaleax]

This currently runs the preparation for main thread execution - which means creating two environments with this flag is going to be kind of wonky.

Just to be clear, this particular flag should go away anyway. It’s only here because CreateEnvironment() does run prepareMainThreadExecution() currently.

Is this the best way to divide the options? It also seems possible to do this preparation step in a method and the user can choose to call it themselves.

If you’re suggesting to add more flags that control which parts of the setup to run, then I think that’s a good idea. What flags would you have in mind?

The flags here seem to anchor on the decision that Environment should be the primary abstraction for embedders to interact with the Node.js instance, but I believe previously it is more like an opaque structure being passed around (albeit being hacked a lot in the wild)?

I would say that Environment is the Node.js instance, so it should be what embedders interact with when they’re dealing with Node.js instances, too.

My original plan is to make an abstraction over Worker and NodeMainInstance somehow, and let the users interact with the instance primarily through that. Then Environment would become more of an implementation detail that we are free to refactor with. AFAICT, the embedding use cases of Node.js mostly differ on whether the embdder wants to own the platform, or the event loop, etc. and I think making options out of those needs first and add a few more setters for lower-level options when they are not flexible enough instead of exposing low-level options from the start would result in a more maintainable API.

I’m sorry, but I disagree on this… Environment is not an implementation detail to me, and embedders really should have fine-grained control over the full setup of Platform/Isolate/event loop if they want to.

There can always be a helper that abstracts away things like V8/libuv setup, but it’s imo better to expose the more powerful API first rather than restricting usage to only specific patterns.

That would also help us add more cctests for different scenarios without sneaking into the internal namespace.

I don’t really see how that makes it easier to avoid the internal namespace than the approach here? This API should give an embedder everything they need to at least put a Node.js instance into a usable state, and the same goes for our cctests.

[addaleax]

@joyeecheung Maybe it's better to put it this way... If Environment is an implementation detail, then what would you have in mind in terms of public API for managing Node.js instances and how does that differ from Environment? I think that would clear things up for me a bit...

[jasnell]

I'd definitely have to agree that Environment is not an internal implementation detail. I'm generally not all that happy with the way Environment is defined in terms of "public" api but it's what we've got.

[joyeecheung]

How do you envision the routine for creating a worker thread by the embedder with this? This currently allows the embedder to create an environment with half of the bootstrap suitable for workers, but I cannot envision how they can create a worker and establish a communication channel with another Node.js instance with that environment. It seems somewhat weird if that ends up being calling a function with two environments?

[addaleax]

How do you envision the routine for creating a worker thread by the embedder with this?

Yeah, this doesn’t really cover that case explicitly, but I don’t see fundamental issues with that – with this API one is able to create an Isolate + event loop + Environment on a different thread that the embedder has full control over.

This currently allows the embedder to create an environment with half of the bootstrap suitable for workers,

This goes back to your previous point about having more fine-grained control over which parts of the setup to run, right?

but I cannot envision how they can create a worker and establish a communication channel with another Node.js instance with that environment.

Well … the same way that we do it, basically? They’d have to recreate the code for Worker threads and messaging APIs if they really wanted to re-create Worker threads, and I guess we could look into helpers for that, but I feel like that opens another can of worms. For example, currently we assume that Node.js Workers never outlive their parent threads, but with embedders we’d lose control over that.

It seems somewhat weird if that ends up being calling a function with two environments?

Can you expand a bit on this? I.e. which function are you referring to?

[joyeecheung]

Just to be clear, this particular flag should go away anyway. It’s only here because CreateEnvironment() does run prepareMainThreadExecution() currently.

I think my concern with this is that I think we should refrain from adding API that are not covered by tests and are planned to be removed soon. This adds confusion for people who want to embded Node.js - imagine you are new to this code base and just want to embed Node.js into some executable but with the event loop customized - now it's already kind of confusing that the abstraction you need to look at is called Environment, but it gets more difficult when you see all these flags and data structures that seem to allow you to do things, and then realize that they are not even tested and will go away soon.

In the changes to node.h, the addition of StartExecutionCallback makes sense to me, can we decouple that from other additions either in this PR, or another one? Though I am not sure if we need StartExecutionCallbackInfo - it already seems enough to take a source string and run it as if it's a main script, which has access to process and require anyway? That's a concept that I think is pretty solid and is not going to go away.

If you’re suggesting to add more flags that control which parts of the setup to run, then I think that’s a good idea. What flags would you have in mind?

I was suggesting that, instead of adding flags that control which parts of the setup to run, we could expose the setup in pieces as methods to call. Both approach can be limited - with flags, it's easier to make sure that the embedder doesn't run things in the wrong order; with methods, they get more freedom when adding logic in the middle of the setup. But I think the latter is probably more maintainable, and in addition we can add specific options to these methods to allow even finer granularity of control, which may be harder when what they need has to be composed out of flags.

Environment is not an implementation detail to me...There can always be a helper that abstracts away things like V8/libuv setup, but it’s imo better to expose the more powerful API first rather than restricting usage to only specific patterns.

I see, I think that's where we mostly disagree about - I believe that API design, if possible, should be done top-down instead of bottom-up. From my experience explaining to other people about Node.js internals, one of the first obstacles of understanding the C++ part of Node.js is exactly understanding:

Environment is the Node.js instance

This is not that clear until you learn enough about the Node.js internals. So I am skeptical about using Environment as the primary abstraction in the embedder API. It is not a deal breaker - v8 also uses Isolate and it is not immediately clear that this is the primary abstraction of the engine. But I think having one obscure lingo is probably still better than having two.

Maybe it's better to put it this way... If Environment is an implementation detail, then what would you have in mind in terms of public API for managing Node.js instances and how does that differ from Environment? I think that would clear things up for me a bit...

Good question, I am running out of time as I write this comment, I'll post some imaginary snippet later. I am not necessarily against making the Environment-based API more powerful, but I think it should be more like an temporary thing that will be deprecated eventually in favor of a different abstraction that'll be more powerful and flexible.

[addaleax]

Just to be clear, this particular flag should go away anyway. It’s only here because CreateEnvironment() does run prepareMainThreadExecution() currently.

I think my concern with this is that I think we should refrain from adding API that are not covered by tests and are planned to be removed soon.

Fwiw, the reason that this PR “adds” things that are going to be removed is that I’m going for a semver-minor change here, partly to avoid merge conflicts and partly so that the few embedders (maybe only one – Electron?) have an easier time keeping up with Node’s stable releases.

Right after this PR lands, if it does so in its current form, I’d like to get rid of some of the cruft in a semver-major follow-up, and work on a document explaining how to use the embedder API with examples that we could then also use as the basis for tests.

I can open a PR like that too, or add to this PR if you like, but I’d like to avoid building upon work that’s already actively being discussed and then possibly having to re-do all that :)

This adds confusion for people who want to embded Node.js - imagine you are new to this code base and just want to embed Node.js into some executable but with the event loop customized - now it's already kind of confusing that the abstraction you need to look at is called Environment,

Fwiw, if the naming is your primary concern – I realize it’s not ideal for sure – then maybe using NodeInstance = Environment and using NodeInstance throughout the public API is just a good solution? It’s easy enough but it would also introduce even more legacy functions that we’re renaming away from Environment, e.g. CreateEnvironment(), AddEnvironmentCleanupHook, etc.

but it gets more difficult when you see all these flags and data structures that seem to allow you to do things, and then realize that they are not even tested and will go away soon.

Tbh I wouldn’t consider this a huge issue if we add documentation and do remove anything that’s suppose to be removed on master. While we do not cover all combinations of flags, I think it’s actually a bit unfair to call them untested, because this PR makes both the Node.js main thread and Worker threads use them internally.

In the changes to node.h, the addition of StartExecutionCallback makes sense to me, can we decouple that from other additions either in this PR, or another one?

What would “in this PR” mean, besides being a separate commit (which is is right now)? I’m not keen on splitting it out into another PR and happy to incorporate feedback on it into this one.

Though I am not sure if we need StartExecutionCallbackInfo - it already seems enough to take a source string and run it as if it's a main script, which has access to process and require anyway? That's a concept that I think is pretty solid and is not going to go away.

It seems like these two are equivalent as long as we allow returning something like { process, require } from that script and passing it back to the embedder…

One reason for picking the current approach is that it enables the embedder to store the values and re-use them for multiple scripts (or their own ES modules, I guess) a bit more easily, and another reason is that our code for running built-in scripts is beyond what I understand. (This is a bit tangential, but I think it would also be great to be able to add scripts that embedders could load with the internal require() function, rather than dropping files into lib/ and adding them to js2c.py … but yeah, I don’t quite know enough about how our native module loader works on the C++ side and it would take a bit to build that for me.)

Ultimately, I’m good with either solution.

If you’re suggesting to add more flags that control which parts of the setup to run, then I think that’s a good idea. What flags would you have in mind?

I was suggesting that, instead of adding flags that control which parts of the setup to run, we could expose the setup in pieces as methods to call. Both approach can be limited - with flags, it's easier to make sure that the embedder doesn't run things in the wrong order; with methods, they get more freedom when adding logic in the middle of the setup.

The flags approach also means that it’s a lot simpler to get started in the default case, though, and API complexity seems to be an important concern to you? :)

But yeah, I’m okay with splitting this up if a) there’s a suggestion for how to split it up and b) this PR does not become a breaking change.

But I think the latter is probably more maintainable, and in addition we can add specific options to these methods to allow even finer granularity of control, which may be harder when what they need has to be composed out of flags.

I think anything that could be added to more granular calls could also be added to the model in this PR, and in this case we do have more control over things like the order in which things happen.

Environment is not an implementation detail to me...There can always be a helper that abstracts away things like V8/libuv setup, but it’s imo better to expose the more powerful API first rather than restricting usage to only specific patterns.

I see, I think that's where we mostly disagree about - I believe that API design, if possible, should be done top-down instead of bottom-up.

Yes, I do disagree about this. I’m a fan of adding simpler utilities for embedding Node.js than what this PR exposes, but it’s imo more important to define a) a working API and b) one where it’s clear what embedders can and cannot do.

From my experience explaining to other people about Node.js internals, one of the first obstacles of understanding the C++ part of Node.js is exactly understanding:

Environment is the Node.js instance

This is not that clear until you learn enough about the Node.js internals. So I am skeptical about using Environment as the primary abstraction in the embedder API.

What is the connection between these two points here? The former seems like a docmentation isue, the latter like an API structure question.

It is not a deal breaker - v8 also uses Isolate and it is not immediately clear that this is the primary abstraction of the engine. But I think having one obscure lingo is probably still better than having two.

Yeah, see my comments about aliasing – I think if this is the main issue, then it’s an actually easily solved one.

Maybe it's better to put it this way... If Environment is an implementation detail, then what would you have in mind in terms of public API for managing Node.js instances and how does that differ from Environment? I think that would clear things up for me a bit...

Good question, I am running out of time as I write this comment, I'll post some imaginary snippet later. I am not necessarily against making the Environment-based API more powerful, but I think it should be more like an temporary thing that will be deprecated eventually in favor of a different abstraction that'll be more powerful and flexible.

Yeah, a snippet might be good – I’m not really clear on what you have in mind for a more powerful/flexible API that would replace this one.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/26671/

[joyeecheung]

I’m going for a semver-minor change here, partly to avoid merge conflicts and partly so that the few embedders (maybe only one – Electron?) have an easier time keeping up with Node’s stable releases.

Why would this be otherwise semver-major? ("otherwise" being "keeping all changes internal"?)

I can open a PR like that too, or add to this PR if you like, but I’d like to avoid building upon work that’s already actively being discussed and then possibly having to re-do all that :)

I understand, though in terms of API work I generally prefer a design-first-implementation-after approach (which comes back to the top-down v.s. bottom-up thing), but that's more about a rough idea of the design instead of detailed examples and thorough tests.

then maybe using NodeInstance = Environment and using NodeInstance throughout the public API is just a good solution?

I think in my mental model the primary abstraction should be separated from the internal Environment mostly because the internal Environment is more of an hotch-potch at this point (e.g. with all the v8::Eternals). I'd prefer to see a new class with methods selected from the internal Environment, as this makes the encapsulation stronger (kind of like v8::internal::Isolate and v8::Isolate).

What would “in this PR” mean, besides being a separate commit (which is is right now)? I’m not keen on splitting it out into another PR and happy to incorporate feedback on it into this one.

I meant to only make CreatetEnvironment does what the defaults get it to do and add a callback argument to LoadEnvironment in one PR, while separating the other changes to node.h in another PR. Because the CreatetEnvironment options outside of the defaults (in particular that these are provided along with an Environment-based API) are where my disagreement comes from.

One reason for picking the current approach is that it enables the embedder to store the values and re-use them for multiple scripts (or their own ES modules, I guess) a bit more easily, and another reason is that our code for running built-in scripts is beyond what I understand.

I think there probably is not too much difference whether these values are passed into C++ or JS (they can store the values somehow either way), but I'd assume it's easier to program a JS entry point directly than having to compile the script yourself in a C++ entry point.

Something like this probably should already work - this is the bare minimum, so the scripts added would be visible to the entire process unless it's removed immediately after being executed, and the caller needs to own the source array, etc. Other scripts can also be added in a similar fashion we probably want to be more careful about those since that can be abused to run arbitrary code with access to internals at runtime.

See diff

diff --git a/src/node.cc b/src/node.cc
index 5a8e6ea8c0..669b2135ae 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -441,6 +441,21 @@ void LoadEnvironment(Environment* env) {
   USE(StartMainThreadExecution(env));
 }

+void LoadEnvironment(Environment* env, const char* name, const uint16_t* source,
+                     size_t length) {
+  CHECK(env->is_main_thread());
+  // Note: this is process-wide. Need to do a Remove() if this should not
+  // be persistent across calls.
+  NativeModuleEnv::Add(name, UnionBytes(source, length));
+  std::vector<Local<String>> params = {
+      env->process_string(),
+      env->require_string()};
+  std::vector<Local<Value>> args = {
+      env->process_object(),
+      env->native_module_require()};
+  ExecuteBootstrapper(env, name, &params, &args);
+}
+
 #ifdef __POSIX__
 typedef void (*sigaction_cb)(int signo, siginfo_t* info, void* ucontext);
 #endif
diff --git a/src/node.h b/src/node.h
index f1e769a182..d89b3688dc 100644
--- a/src/node.h
+++ b/src/node.h
@@ -365,6 +365,8 @@ NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data,
                                            const char* const* exec_argv);

 NODE_EXTERN void LoadEnvironment(Environment* env);
+NODE_EXTERN void LoadEnvironment(Environment* env, const char* name,
+                                 const uint16_t* source, size_t length);
 NODE_EXTERN void FreeEnvironment(Environment* env);

 // This may return nullptr if context is not associated with a Node instance.
diff --git a/src/node_native_module.cc b/src/node_native_module.cc
index 814adb620d..423ac477d8 100644
--- a/src/node_native_module.cc
+++ b/src/node_native_module.cc
@@ -33,6 +33,14 @@ bool NativeModuleLoader::Exists(const char* id) {
   return source_.find(id) != source_.end();
 }

+bool NativeModuleLoader::Add(const char* id, const UnionBytes& source) {
+  if (Exists(id)) {
+    return false;
+  }
+  source_.emplace(id, source);
+  return true;
+}
+
 Local<Object> NativeModuleLoader::GetSourceObject(Local<Context> context) {
   Isolate* isolate = context->GetIsolate();
   Local<Object> out = Object::New(isolate);
diff --git a/src/node_native_module.h b/src/node_native_module.h
index fabaea7568..564b617e88 100644
--- a/src/node_native_module.h
+++ b/src/node_native_module.h
@@ -47,6 +47,8 @@ class NativeModuleLoader {
   UnionBytes GetConfig();       // Return data for config.gypi

   bool Exists(const char* id);
+  bool Add(const char* id, const UnionBytes& source);
+
   v8::Local<v8::Object> GetSourceObject(v8::Local<v8::Context> context);
   v8::Local<v8::String> GetConfigString(v8::Isolate* isolate);
   std::vector<std::string> GetModuleIds();
diff --git a/src/node_native_module_env.cc b/src/node_native_module_env.cc
index 31536000fc..01e28669b7 100644
--- a/src/node_native_module_env.cc
+++ b/src/node_native_module_env.cc
@@ -33,6 +33,10 @@ Local<Set> ToJsSet(Local<Context> context, const std::set<std::string>& in) {
   return out;
 }

+bool NativeModuleEnv::Add(const char* id, const UnionBytes& source) {
+  NativeModuleLoader::GetInstance()->Add(id, source);
+}
+
 bool NativeModuleEnv::Exists(const char* id) {
   return NativeModuleLoader::GetInstance()->Exists(id);
 }
diff --git a/src/node_native_module_env.h b/src/node_native_module_env.h
index f662c67be5..bc36be7510 100644
--- a/src/node_native_module_env.h
+++ b/src/node_native_module_env.h
@@ -29,6 +29,7 @@ class NativeModuleEnv {
   // Returns config.gypi as a JSON string
   static v8::Local<v8::String> GetConfigString(v8::Isolate* isolate);
   static bool Exists(const char* id);
+  static bool Add(const char* id, const UnionBytes& source);

   // Loads data into NativeModuleLoader::.instance.code_cache_
   // Generated by mkcodecache as node_code_cache.cc when

it’s imo more important to define a) a working API and b) one where it’s clear what embedders can and cannot do.

I think our ideal state is to have an embedder API that is a) easy-to-use and can be tested by simple example embedders and b) fully meets the requirements of existing emebedders. Until we reach b), one way or another, existing emebedders like Electron are always going to modify Node.js's sources to reach b) themselves.

With the bottom-up approach we'll probably reach b) first then a) later, and it's probably easier to converge the current API to b). But the risks are:

1. We will be in an temporary state where we meet neither a) or b) - that is, where we are now
2. We may end up doing several semver-major changes to meet a) because of the internals exposed by b)

With the top-down approach, we may reach a) first , then gradually become powerful enough to reach b). The upside is we'll get to a) out of the box which should be probably more friendly to future embedders, however:

1. It might take longer to reach a) because it'll be a redesign instead of being built upon an existing API
2. It might also take longer for existing embedders to transition to the new API (for the same reason as 1)

I think my primary concern is about risk 2 with the bottom-up approach (compatibility) because that may be entirely out of our hands. Although that depends on the expectation of compatibility that we set with these API changes. Compared to that the risks with the top-down approach are, IMO, at least controllable by us so they are less scary.

[joyeecheung]

class NodeInstance {
  // Used by snapshot builders.  Isolate is owned by the caller.
  static std::unique_ptr<NodeInstance> Create(
      v8::Isolate* isolate,
      uv_loop_t* event_loop,
      MultiIsolatePlatform* platform,
      const std::vector<std::string>& args,
      const std::vector<std::string>& exec_args);
  void Dispose();

  // Used by snapshot consumers. Isolate owned by the instance.
  NodeInstance(
      v8::Isolate::CreateParams* params,
      uv_loop_t* event_loop,
      MultiIsolatePlatform* platform,
      const std::vector<std::string>& args,
      const std::vector<std::string>& exec_args,
      /* optional data used to deserialize from snapshot */);
  ~NodeInstance();

  /* Data used to deserialize from snapshot */ CreateSnapshot();

  // Evaluate CJS
  v8::Maybe<v8::Value> EvaluateScript(/* source params */);
  // Evaluate ESM
  v8::Maybe<v8::Value> EvaluateModule(/* source params */);

  // Do necessary fixups to prepare for main script execution
  v8::Maybe<v8::Value> PrepareForExecution(/* inspector control */) virtual;
  // Execute a main script
  v8::Maybe<v8::Value> ExecuteMainScript(/* source params */);

  // Start running the event loop, return the exit code when finished.
  int Run() virtual;

  Environment* env();  // Should this be protected?

  // All kinds of getters, setters, hooks, access control flags, exposed if necessary
};

class NodeMainInstance : public NodeInstance {
  v8::Maybe<v8::Value> PrepareForExecution(/* inspector control */) override;
  int Run() override;
};

class NodeWorkerInstance : public NodeInstance {
  v8::Maybe<v8::Value> PrepareForExecution(/* inspector control */) override;
  int Run() override;
};

As requested this is roughly what I have in mind, we already use something like NodeInstance::Create() and NodeInstance::Dispose() in our own snapshot builder and the constructor/deconstructor in node::Start(). Within this class the kPrepareForExecution and kOwnsInspector in this PR can be controlled by PrepareForExecution, while kDoNotAbortOnUncaughtException and kOwnsProcessState can be set on-the-fly (we can simply remove things to disallow access, instead of adding things when the flag is set to a certain value).

Now with something like this it'll take more refactoring to use it internally in the worker code (compared to the changes to CreateEnvironment in this PR), but it's already not too far away from what the main thread uses, and that's what most of the embedders currently use anyway.

[addaleax]

I’m going for a semver-minor change here, partly to avoid merge conflicts and partly so that the few embedders (maybe only one – Electron?) have an easier time keeping up with Node’s stable releases.

Why would this be otherwise semver-major? ("otherwise" being "keeping all changes internal"?)

To clarify, this was specifically about things like the added kPrepareForExecution flag, which should go away but is there so that CreateEnvironment() doesn’t stop running the prepareMainThreadExecution() code.

And since this is a PR that specifically focuses on providing APIs, I’m not sure how keeping changes internal makes sense?

I can open a PR like that too, or add to this PR if you like, but I’d like to avoid building upon work that’s already actively being discussed and then possibly having to re-do all that :)

I understand, though in terms of API work I generally prefer a design-first-implementation-after approach (which comes back to the top-down v.s. bottom-up thing), but that's more about a rough idea of the design instead of detailed examples and thorough tests.

The API that’s being added here is not lacking in having a design, imo. It might not be pretty, but there is a specific goal that’s being followed here, with clear ideas of what the steps are to set something up and how embedders can use the flexibility they get from this.

then maybe using NodeInstance = Environment and using NodeInstance throughout the public API is just a good solution?

I think in my mental model the primary abstraction should be separated from the internal Environment mostly because the internal Environment is more of an hotch-potch at this point (e.g. with all the v8::Eternals). I'd prefer to see a new class with methods selected from the internal Environment, as this makes the encapsulation stronger (kind of like v8::internal::Isolate and v8::Isolate).

I don’t mind a V8-like approach here, if it’s important to you to have a class rather than methods that take pointer arguments (which, again, I agree would be prettier) – feel free to open a PR with that if you like. But again, I’d prefer to keep this PR fully backwards-compatible and I’m also open to making any number of changes in the next one.

What would “in this PR” mean, besides being a separate commit (which is is right now)? I’m not keen on splitting it out into another PR and happy to incorporate feedback on it into this one.

I meant to only make CreatetEnvironment does what the defaults get it to do

Again, I really really want to avoid breaking changes here. They can be helpful but are usually just a pain for everybody who already uses APIs.

and add a callback argument to LoadEnvironment in one PR, while separating the other changes to node.h in another PR. Because the CreatetEnvironment options outside of the defaults (in particular that these are provided along with an Environment-based API) are where my disagreement comes from.

Is that a blocker for you? Because this PR would formally be pretty close to landing, and, as I’ve said, I’m happy to make changes that are more to your liking in a breaking follow-up. I do see your point about the callback-vs-script thing, but I’m also okay with working that out here.

One reason for picking the current approach is that it enables the embedder to store the values and re-use them for multiple scripts (or their own ES modules, I guess) a bit more easily, and another reason is that our code for running built-in scripts is beyond what I understand.

I think there probably is not too much difference whether these values are passed into C++ or JS (they can store the values somehow either way), but I'd assume it's easier to program a JS entry point directly than having to compile the script yourself in a C++ entry point.

Something like this probably should already work - this is the bare minimum, so the scripts added would be visible to the entire process unless it's removed immediately after being executed, and the caller needs to own the source array, etc. Other scripts can also be added in a similar fashion we probably want to be more careful about those since that can be abused to run arbitrary code with access to internals at runtime.
See diff

Thanks for this! I’ll update this PR shortly with something based on your diff.

it’s imo more important to define a) a working API and b) one where it’s clear what embedders can and cannot do.

I think our ideal state is to have an embedder API that is a) easy-to-use and can be tested by simple example embedders and b) fully meets the requirements of existing emebedders. Until we reach b), one way or another, existing emebedders like Electron are always going to modify Node.js's sources to reach b) themselves.

With the bottom-up approach we'll probably reach b) first then a) later, and it's probably easier to converge the current API to b). But the risks are:

1. We will be in an temporary state where we meet neither a) or b) - that is, where we are now

2. We may end up doing several semver-major changes to meet a) because of the internals exposed by b)

With the top-down approach, we may reach a) first , then gradually become powerful enough to reach b). The upside is we'll get to a) out of the box which should be probably more friendly to future embedders, however:

1. It might take longer to reach a) because it'll be a redesign instead of being built upon an existing API

2. It might also take longer for existing embedders to transition to the new API (for the same reason as 1)

I think my primary concern is about risk 2 with the bottom-up approach (compatibility) because that may be entirely out of our hands. Although that depends on the expectation of compatibility that we set with these API changes. Compared to that the risks with the top-down approach are, IMO, at least controllable by us so they are less scary.

I don’t think we’re going to get to agreement on this – like, I’d agree with what you’re saying if we were starting with a clean slate and working on an embedder API from scratch, but that’s kind of not my reality. From my PoV, the existing embedder(s) already try to make do with what they have, and giving them something that does fulfill their needs has the highest priority for me.

I’m not scared of breaking changes for embedders – they have a very different model of using Node.js anyway, so we could imo skip things like deprecation cycles, I just don’t like breaking changes from a backporting process angle.

---

class NodeInstance {

I like this approach, but at the same time it seems like syntactic sugar for an Environment*. That’s not bad, I’m a fan of it, but it feels a bit out of scope for this PR? In particular things like snapshot support and evaluating CJS/ESM is a larger issue, and tbh you are probably better fit to actually implement this at this point.

class NodeMainInstance : public NodeInstance {
class NodeWorkerInstance : public NodeInstance {

This is something I’m not a fan of – it seems like a antipattern to me to have separate classes where flags seem to make more sense, because fundamentally these two aren’t different things, they just behave a bit differently in some regards. I’d also prefer not to expose a way to create Workers for embedders other than what the worker_threads module already provides.

[addaleax]

Okay, updated this with a LoadEnvironment() overload that takes a main script (and hopefully made it work in a way that works somewhat well with multiple calls to LoadEnvironment() by using the thread id … that would leak memory but I think that’s not the end of the world).

Also found a bug in the process :)

Btw, maybe putting it this way helps: I think this PR strictly improves the situation for embedders, and this PR doesn’t claim to provide a perfect and final API. It’s okay not to like that, but I would prefer to keep moving forward with work on this. And I’m really, really not disagreeing with your goals – if you have the time to work on one big leap towards reaching something like what you’re proposing, please feel free to do so.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/26752/

[joyeecheung]

Again, I really really want to avoid breaking changes here. They can be helpful but are usually just a pain for everybody who already uses APIs.

hmm, I still don't quite understand why not expsoing the flags to CreateEnviroment or the inspector/ProfilerIdleNotifierMode options to LoadEnvironment in the initial PR can be breaking? AFAICT, these internals are exposed here just so that the setups used in the internal worker code can be publicly available though modifications to CreateEnviroment and LoadEnvironment? What difference does it make in terms of breaking embdders if those new signatures (other than the main script options which are tested by cctest here) are placed in node_internals.h this PR? To clarify I am okay with landing this PR if those are in node_internals.h, and then moving on discussing whether these should be moved to node.h in another PR. I can't see how this approach breaks anyone?

Is that a blocker for you?

I think yes, until my question above can be answered.

From my PoV, the existing embedder(s) already try to make do with what they have, and giving them something that does fulfill their needs has the highest priority for me.

I see. I do not mind that we go ahead with the Environment-based API first if this is of higher priority for you, as long as we don't break too much encapsulation that may make it too hard to start from the clean slate later.

The API that’s being added here is not lacking in having a design, imo. It might not be pretty, but there is a specific goal that’s being followed here, with clear ideas of what the steps are to set something up and how embedders can use the flexibility they get from this.

I was not saying the API here lacks design, though, but for me it's more like "this is what we do internally and we are exposing the internals so that you can do what we do internally". When design (prettiness) is less of a priority compared implementation complexity, how pretty the API can be comes down to how pretty the internals are.

it feels a bit out of scope for this PR?

Yes, this is just to answer the request of showing what I originally had in mind, I am not asking this to be implemented, and certainly not at the cost of abandoning what's being done here - given the different priorities, I think a "clean-slate" design, if it ends up being implemented, will have to coexist with the existing APIs anyway.

it seems like a antipattern to me to have separate classes where flags seem to make more sense, because fundamentally these two aren’t different things, they just behave a bit differently in some regards.

Yeah, this is still pretty much in the flux in my head, and for me this sketch is still more like in the state of "we'll know which direction to go when the code is refactored to the point where this can be implemented".

I’d also prefer not to expose a way to create Workers for embedders other than what the worker_threads module already provides.

Good point, and I was not certain about that either as a new class for workers probably does not offer anything that the embedder can't readily do in JS. But this discussion on an imaginary snippet is probably too off-topic now so I'll stop.

[addaleax]

Again, I really really want to avoid breaking changes here. They can be helpful but are usually just a pain for everybody who already uses APIs.

hmm, I still don't quite understand why not expsoing the flags to CreateEnviroment or the inspector/ProfilerIdleNotifierMode options to LoadEnvironment in the initial PR can be breaking?

Oh, yeah, not exposing the new methods of course would not technically be a breaking change.

AFAICT, these internals are exposed here just so that the setups used in the internal worker code can be publicly available though modifications to CreateEnviroment and LoadEnvironment?

Not really – they are here because while useful for Workers, they would also be meaningful to embedders. We still have plenty of real internals that only make sense for Workers. (That’s why, for example, the set_env_vars() call is kept as an internal here but the set_abort_on_uncaught_exception() call isn’t.).

What difference does it make in terms of breaking embdders if those new signatures (other than the main script options which are tested by cctest here) are placed in node_internals.h this PR?

I mean, technically things like moving libuv + inspector + diagnostics initialization into LoadEnvironment() should break Electron as a very concrete case, because they’re currently doing that manually by using our internal methods for that (because they don’t really have another choice with the current state of the API).

To clarify I am okay with landing this PR if those are in node_internals.h, and then moving on discussing whether these should be moved to node.h in another PR. I can't see how this approach breaks anyone?

I mean, sure, I can do that, I just don’t really see what we gain by splitting this out into another PR? If there are concrete issues, I’d still prefer to work these out here.

But yeah, I think the Electron example is a good one (and also part of why I pinged Shelley here) – this should enable at least them to move from internal APIs to public ones. Keeping this internal for now would kind of undo that advantage.

[joyeecheung]

I mean, technically things like moving libuv + inspector + diagnostics initialization into LoadEnvironment() should break Electron as a very concrete case, because they’re currently doing that manually by using our internal methods for that (because they don’t really have another choice with the current state of the API).

It may break Electron because this PR already may break Electron, not because the changes are split? What if the LoadEnvironment(Environment* env) overload is left as-is in this PR though?

this should enable at least them to move from internal APIs to public ones. Keeping this internal for now would kind of undo that advantage.

From #30494 and some other past PRs, my guess is for Electron it does not seem to make too much a difference whether the options are in node.h or node_internals.h?

[addaleax]

this should enable at least them to move from internal APIs to public ones. Keeping this internal for now would kind of undo that advantage.

From #30494 and some other past PRs, my guess is for Electron it does not seem to make too much a difference whether the options are in node.h or node_internals.h?

I mean, yes, but I do really want to get them off using Node.js internals and this is part of that 😄

[addaleax]

@joyeecheung Trying to move forward here … what exactly are you a hard -1 on? Is it just the flags, or all additions to node.h here?

[joyeecheung]

@addaleax the additions to node.h other than the start calllback (the part that are not covered by cctest in this PR). Also as I said I am OK if the untested part are moved into node_internals.h for now. We can revisit when there are actual use cases of these options (e.g. when Electron actually uses them)

[addaleax]

@addaleax the additions to node.h other than the start calllback (the part that are not covered by cctest in this PR).

Would it make a difference for you to see explicit cctests? The other parts are already heavily covered through tests through our internal usage, but it shouldn’t be hard to add more.

Also as I said I am OK if the untested part are moved into node_internals.h for now. We can revisit when there are actual use cases of these options (e.g. when Electron actually uses them)

Yeah then let’s put this on the TSC agenda, I’ve put a ton of work into this and related PRs over the last weeks and I’d rather not see it go unused – and I’d definitely disagree with saying that there are no actual use cases right now.

[mhdawson]

I'll add my 2 cents that there are definitely use cases for a better embedder API. I know of at least one case in IBM were Node.js is being embedded. I've not had time to go through this PR in detail but I'm definitely in support of taking steps towards improving/exposing a documented API.