Skip to content

Commit

Permalink
src: make BuiltinLoader threadsafe and non-global
Browse files Browse the repository at this point in the history
As discussed in #45888, using a
global `BuiltinLoader` instance is probably undesirable in a world
in which embedders are able to create Node.js Environments with
different sources and therefore mutually incompatible code
caching properties.

This PR makes it so that `BuiltinLoader` is no longer a global
singleton and instead only shared between `Environment`s that
have a direct relation to each other, and addresses a few
thread safety issues along with that.
  • Loading branch information
addaleax committed Dec 22, 2022
1 parent 71951a0 commit 42f9b93
Show file tree
Hide file tree
Showing 13 changed files with 147 additions and 122 deletions.
6 changes: 3 additions & 3 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -458,9 +458,8 @@ MaybeLocal<Value> LoadEnvironment(
main_script_source_utf8).ToLocalChecked();
auto main_utf16 = std::make_unique<String::Value>(isolate, str);

// TODO(addaleax): Avoid having a global table for all scripts.
std::string name = "embedder_main_" + std::to_string(env->thread_id());
builtins::BuiltinLoader::Add(
env->builtin_loader()->Add(
name.c_str(), UnionBytes(**main_utf16, main_utf16->length()));
env->set_main_utf16(std::move(main_utf16));
Realm* realm = env->principal_realm();
Expand Down Expand Up @@ -703,9 +702,10 @@ Maybe<bool> InitializePrimordials(Local<Context> context) {
"internal/per_context/messageport",
nullptr};

auto builtin_loader = builtins::BuiltinLoader::Create();
for (const char** module = context_files; *module != nullptr; module++) {
Local<Value> arguments[] = {exports, primordials};
if (builtins::BuiltinLoader::CompileAndCall(
if (builtin_loader->CompileAndCall(
context, *module, arraysize(arguments), arguments, nullptr)
.IsEmpty()) {
// Execution failed during context creation.
Expand Down
9 changes: 9 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,15 @@ inline std::vector<double>* Environment::destroy_async_id_list() {
return &destroy_async_id_list_;
}

inline std::shared_ptr<builtins::BuiltinLoader> Environment::builtin_loader() {
DCHECK(builtin_loader_);
return builtin_loader_;
}

inline void Environment::set_builtin_loader(std::shared_ptr<builtins::BuiltinLoader> loader) {
builtin_loader_ = loader;
}

inline double Environment::new_async_id() {
async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter] += 1;
return async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter];
Expand Down
2 changes: 2 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,8 @@ Environment::Environment(IsolateData* isolate_data,
env_info,
flags,
thread_id) {
// TODO(addaleax): Make this part of CreateEnvironment().
set_builtin_loader(builtins::BuiltinLoader::Create());
InitializeMainContext(context, env_info);
}

Expand Down
5 changes: 5 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,8 @@ class Environment : public MemoryRetainer {
inline std::vector<double>* destroy_async_id_list();

std::set<struct node_module*> internal_bindings;
std::shared_ptr<builtins::BuiltinLoader> builtin_loader();
void set_builtin_loader(std::shared_ptr<builtins::BuiltinLoader> loader);
std::set<std::string> builtins_with_cache;
std::set<std::string> builtins_without_cache;
// This is only filled during deserialization. We use a vector since
Expand Down Expand Up @@ -1147,7 +1149,10 @@ class Environment : public MemoryRetainer {
// Keeps the main script source alive is one was passed to LoadEnvironment().
// We should probably find a way to just use plain `v8::String`s created from
// the source passed to LoadEnvironment() directly instead.
// TODO(addaleax): Move this to BuiltinLoader, like we do for
// BuiltinLoader::AddExternalizedBuiltin().
std::unique_ptr<v8::String::Value> main_utf16_;
std::shared_ptr<builtins::BuiltinLoader> builtin_loader_;

// Used by allocate_managed_buffer() and release_managed_buffer() to keep
// track of the BackingStore for a given pointer.
Expand Down
3 changes: 0 additions & 3 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1183,9 +1183,6 @@ ExitCode LoadSnapshotDataAndRun(const SnapshotData** snapshot_data_ptr,
}
}

if ((*snapshot_data_ptr) != nullptr) {
BuiltinLoader::RefreshCodeCache((*snapshot_data_ptr)->code_cache);
}
NodeMainInstance main_instance(*snapshot_data_ptr,
uv_default_loop(),
per_process::v8_platform.Platform(),
Expand Down
4 changes: 2 additions & 2 deletions src/node_binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -586,13 +586,13 @@ void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
exports->SetPrototype(env->context(), Null(env->isolate())).FromJust());
DefineConstants(env->isolate(), exports);
} else if (!strcmp(*module_v, "natives")) {
exports = builtins::BuiltinLoader::GetSourceObject(env->context());
exports = env->builtin_loader()->GetSourceObject(env->context());
// Legacy feature: process.binding('natives').config contains stringified
// config.gypi
CHECK(exports
->Set(env->context(),
env->config_string(),
builtins::BuiltinLoader::GetConfigString(env->isolate()))
env->builtin_loader()->GetConfigString(env->isolate()))
.FromJust());
} else {
return THROW_ERR_INVALID_MODULE(env, "No such binding: %s", *module_v);
Expand Down
Loading

0 comments on commit 42f9b93

Please sign in to comment.