Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

embedding: allow creating context within inspectable node::Environment #31424

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,40 @@ Local<Context> NewContext(Isolate* isolate,
return context;
}

NewContextOptions::NewContextOptions() {
initialize = false;
microtask_queue = nullptr;
}

Local<Context> NewContext(Environment* env,
Local<ObjectTemplate> object_template,
const NewContextOptions& options) {
auto context = Context::New(
env->isolate(),
nullptr,
object_template,
v8::MaybeLocal<v8::Value>(),
v8::DeserializeInternalFieldsCallback(),
options.microtask_queue);
if (context.IsEmpty()) return context;

// Call InitializeContext only if initialize options is true
if (options.initialize && !InitializeContext(context)) {
return Local<Context>();
}
addaleax marked this conversation as resolved.
Show resolved Hide resolved

ContextInfo info(options.debug_name);
info.origin = options.debug_origin;

// Inspector agent is an (optional) member of node::Environment.
// This notifies the environment's inspector agent for the new context.
// Also assigns methods required for Environment::GetCurrent(context) to work.
// The latter is require for properly processing stack traces in node.
env->AssignToContext(context, info);

return context;
}

// This runs at runtime, regardless of whether the context
// is created from a snapshot.
void InitializeContextRuntime(Local<Context> context) {
Expand Down
26 changes: 26 additions & 0 deletions src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,32 @@ NODE_EXTERN v8::Local<v8::Context> NewContext(
v8::Local<v8::ObjectTemplate> object_template =
v8::Local<v8::ObjectTemplate>());

// Preferred way to add additional options to node::NewContext below
// We can add more fields (as needed) later, without changing the
// interface of node::NewContext.
struct NewContextOptions {
NewContextOptions();
// If false creates "pure" context as created by V8
// If true, setup node specific initialization,
// required for running node code (like loading modules, etc)
bool initialize;
// This parameters is passed to v8::Context::New.
// The default is nullptr (use the default queue from the isolate).
v8::MicrotaskQueue* microtask_queue;
// The following two parameters are passed to the inspector agent.
// Use them to control how the new context will appear during debugging.
std::string debug_name;
std::string debug_origin;
};

// Create a new context for an existing environment.
// This add several fields to make inspector work properly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// This add several fields to make inspector work properly.
// This adds several fields to make the inspector work properly.

Copy link
Author

@Dragiyski Dragiyski Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would disagree, since there are three major problems in what you have described:

  • The API through which v8_inspector::V8Inspector is accessed must be aware both in compile-time and in runtime whether an inspector agent actually exists (currently node::Environment does that);
  • v8_inspector::V8Inspector::ContextCreated() is intended to use from the context-creating functions. It is not a notification and it will happily re-assign the debug_context_id leading to undefined behavior for any outstanding requests from the frontend with the old debug_context_id.
  • In the V8 API there are per-isolate properties like v8::Isolate::SetPrepareStackTraceCallback(). If the environment does not handle that there should be another class in the hierarchy: node::MultiIsolatePlatform -> (per-isolate object) -> node::Environment. This would require major untangling of the node::Environment and it is not related to the main goal here: creating a new context.

I think that hiding the context creation process steps from the API is way more stable than exposing the inspector agent to the embedder. Currently node::Environment perform some per-isolate processes like v8::Isolate::SetPrepareStackTraceCallback. In the V8 engine v8::Context objects are part of v8::Isolate, and there are certain API that are per-isolate. Therefore, if node::Environment does not handle the per-isolate API, there must be another class that handles it (and node::Environment would probably contains shared pointer towards it). And since the inspector agent would always be hidden within an instance of a node's class (see the first two points above), the best approach for the context creation process would be to have node::NewContext that accepts the node's per-isolate object and perform node's specific initialization (like reporting the context to its inspector agent).

For the current minor version, node::Environment is still per-isolate, and the exposed function signature allow proper initialization with provided embedder's v8::ObjectTemplate while reporting to the inspector agent is not exposed and how it is done can be changed in the future versions.

The only problem is the initialize argument. To become future-proof compatible, it would be better if the third parameter is some white-box (publicly exposed) structure, that can be expanded with other properties in the future versions.

NODE_EXTERN v8::Local<v8::Context> NewContext(
Environment* env,
v8::Local<v8::ObjectTemplate> object_template =
v8::Local<v8::ObjectTemplate>(),
const NewContextOptions& options = NewContextOptions());

// Runs Node.js-specific tweaks on an already constructed context
// Return value indicates success of operation
NODE_EXTERN bool InitializeContext(v8::Local<v8::Context> context);
Expand Down