Skip to content

Commit e7627fe

Browse files
committed
src: make BuiltinLoader threadsafe and non-global
As discussed in nodejs#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.
1 parent b14c6b4 commit e7627fe

13 files changed

+167
-131
lines changed

src/api/environment.cc

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -470,7 +470,7 @@ MaybeLocal<Value> LoadEnvironment(
470470
return LoadEnvironment(
471471
env, [&](const StartExecutionCallbackInfo& info) -> MaybeLocal<Value> {
472472
std::string name = "embedder_main_" + std::to_string(env->thread_id());
473-
builtins::BuiltinLoader::Add(name.c_str(), main_script_source_utf8);
473+
env->builtin_loader()->Add(name.c_str(), main_script_source_utf8);
474474
Realm* realm = env->principal_realm();
475475

476476
return realm->ExecuteBootstrapper(name.c_str());
@@ -711,10 +711,12 @@ Maybe<bool> InitializePrimordials(Local<Context> context) {
711711
"internal/per_context/messageport",
712712
nullptr};
713713

714+
auto builtin_loader = builtins::BuiltinLoader::Create();
714715
for (const char** module = context_files; *module != nullptr; module++) {
715716
Local<Value> arguments[] = {exports, primordials};
716-
if (builtins::BuiltinLoader::CompileAndCall(
717-
context, *module, arraysize(arguments), arguments, nullptr)
717+
if (builtin_loader
718+
->CompileAndCall(
719+
context, *module, arraysize(arguments), arguments, nullptr)
718720
.IsEmpty()) {
719721
// Execution failed during context creation.
720722
return Nothing<bool>();

src/env-inl.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,16 @@ inline std::vector<double>* Environment::destroy_async_id_list() {
436436
return &destroy_async_id_list_;
437437
}
438438

439+
inline std::shared_ptr<builtins::BuiltinLoader> Environment::builtin_loader() {
440+
DCHECK(builtin_loader_);
441+
return builtin_loader_;
442+
}
443+
444+
inline void Environment::set_builtin_loader(
445+
std::shared_ptr<builtins::BuiltinLoader> loader) {
446+
builtin_loader_ = loader;
447+
}
448+
439449
inline double Environment::new_async_id() {
440450
async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter] += 1;
441451
return async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter];

src/env.cc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -752,6 +752,8 @@ Environment::Environment(IsolateData* isolate_data,
752752
env_info,
753753
flags,
754754
thread_id) {
755+
// TODO(addaleax): Make this part of CreateEnvironment().
756+
set_builtin_loader(builtins::BuiltinLoader::Create());
755757
InitializeMainContext(context, env_info);
756758
}
757759

src/env.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -721,6 +721,9 @@ class Environment : public MemoryRetainer {
721721
// List of id's that have been destroyed and need the destroy() cb called.
722722
inline std::vector<double>* destroy_async_id_list();
723723

724+
std::shared_ptr<builtins::BuiltinLoader> builtin_loader();
725+
void set_builtin_loader(std::shared_ptr<builtins::BuiltinLoader> loader);
726+
724727
std::unordered_multimap<int, loader::ModuleWrap*> hash_to_module_map;
725728
std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
726729
std::unordered_map<uint32_t, contextify::ContextifyScript*>
@@ -1143,6 +1146,8 @@ class Environment : public MemoryRetainer {
11431146

11441147
std::unique_ptr<Realm> principal_realm_ = nullptr;
11451148

1149+
std::shared_ptr<builtins::BuiltinLoader> builtin_loader_;
1150+
11461151
// Used by allocate_managed_buffer() and release_managed_buffer() to keep
11471152
// track of the BackingStore for a given pointer.
11481153
std::unordered_map<char*, std::unique_ptr<v8::BackingStore>>

src/node.cc

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1183,9 +1183,6 @@ ExitCode LoadSnapshotDataAndRun(const SnapshotData** snapshot_data_ptr,
11831183
}
11841184
}
11851185

1186-
if ((*snapshot_data_ptr) != nullptr) {
1187-
BuiltinLoader::RefreshCodeCache((*snapshot_data_ptr)->code_cache);
1188-
}
11891186
NodeMainInstance main_instance(*snapshot_data_ptr,
11901187
uv_default_loop(),
11911188
per_process::v8_platform.Platform(),

src/node_binding.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -631,13 +631,13 @@ void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
631631
CHECK(exports->SetPrototype(context, Null(isolate)).FromJust());
632632
DefineConstants(isolate, exports);
633633
} else if (!strcmp(*module_v, "natives")) {
634-
exports = builtins::BuiltinLoader::GetSourceObject(context);
634+
exports = realm->env()->builtin_loader()->GetSourceObject(context);
635635
// Legacy feature: process.binding('natives').config contains stringified
636636
// config.gypi
637637
CHECK(exports
638638
->Set(context,
639639
realm->isolate_data()->config_string(),
640-
builtins::BuiltinLoader::GetConfigString(isolate))
640+
realm->env()->builtin_loader()->GetConfigString(isolate))
641641
.FromJust());
642642
} else {
643643
return THROW_ERR_INVALID_MODULE(isolate, "No such binding: %s", *module_v);

0 commit comments

Comments
 (0)