Skip to content

Commit

Permalink
src: fix creating Isolates from addons
Browse files Browse the repository at this point in the history
daae938 broke addons which create their own `Isolate`
instances, because enabling the shared-readonly-heap feature
of V8 requires all snapshots used for different `Isolate`s to
be identical. Usage of addons that do this has probably
decreased quite a bit since Worker threads were introduced
in Node.js, but it’s still a valid use case, and in any case
the breakage was probably not intentional (although the referenced
commit did require test changes because of this issue).

This commit addresses this issue partially by caching the
V8 snapshot parameters and ignoring ones passed in from users
in `NewIsolate()` when this feature is enabled, and makes
the `NodeMainInstance` snapshot-based isolate creation
also re-use this code.

PR-URL: #45885
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax authored and juanarbol committed Jan 31, 2023
1 parent 2047577 commit 27d6a8b
Show file tree
Hide file tree
Showing 9 changed files with 132 additions and 26 deletions.
3 changes: 3 additions & 0 deletions node.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@
'NODE_USE_V8_PLATFORM=0',
],
}],
[ 'v8_enable_shared_ro_heap==1', {
'defines': ['NODE_V8_SHARED_RO_HEAP',],
}],
[ 'node_tag!=""', {
'defines': [ 'NODE_TAG="<(node_tag)"' ],
}],
Expand Down
25 changes: 22 additions & 3 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,8 @@ void SetIsolateCreateParamsForNode(Isolate::CreateParams* params) {
const uint64_t total_memory = constrained_memory > 0 ?
std::min(uv_get_total_memory(), constrained_memory) :
uv_get_total_memory();
if (total_memory > 0) {
if (total_memory > 0 &&
params->constraints.max_old_generation_size_in_bytes() == 0) {
// V8 defaults to 700MB or 1.4GB on 32 and 64 bit platforms respectively.
// This default is based on browser use-cases. Tell V8 to configure the
// heap based on the actual physical memory.
Expand Down Expand Up @@ -303,17 +304,35 @@ void SetIsolateUpForNode(v8::Isolate* isolate) {
// careful about what we override in the params.
Isolate* NewIsolate(Isolate::CreateParams* params,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform) {
MultiIsolatePlatform* platform,
bool has_snapshot_data) {
Isolate* isolate = Isolate::Allocate();
if (isolate == nullptr) return nullptr;
#ifdef NODE_V8_SHARED_RO_HEAP
{
// In shared-readonly-heap mode, V8 requires all snapshots used for
// creating Isolates to be identical. This isn't really memory-safe
// but also otherwise just doesn't work, and the only real alternative
// is disabling shared-readonly-heap mode altogether.
static Isolate::CreateParams first_params = *params;
params->snapshot_blob = first_params.snapshot_blob;
params->external_references = first_params.external_references;
}
#endif

// Register the isolate on the platform before the isolate gets initialized,
// so that the isolate can access the platform during initialization.
platform->RegisterIsolate(isolate, event_loop);

SetIsolateCreateParamsForNode(params);
Isolate::Initialize(isolate, *params);
SetIsolateUpForNode(isolate);
if (!has_snapshot_data) {
// If in deserialize mode, delay until after the deserialization is
// complete.
SetIsolateUpForNode(isolate);
} else {
SetIsolateMiscHandlers(isolate, {});
}

return isolate;
}
Expand Down
3 changes: 2 additions & 1 deletion src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,8 @@ bool SafeGetenv(const char* key,
void DefineZlibConstants(v8::Local<v8::Object> target);
v8::Isolate* NewIsolate(v8::Isolate::CreateParams* params,
uv_loop_t* event_loop,
MultiIsolatePlatform* platform);
MultiIsolatePlatform* platform,
bool has_snapshot_data = false);
// This overload automatically picks the right 'main_script_id' if no callback
// was provided by the embedder.
v8::MaybeLocal<v8::Value> StartExecution(Environment* env,
Expand Down
16 changes: 3 additions & 13 deletions src/node_main_instance.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,9 @@ NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data,
isolate_params_.get());
}

isolate_ = Isolate::Allocate();
isolate_ = NewIsolate(
isolate_params_.get(), event_loop, platform, snapshot_data != nullptr);
CHECK_NOT_NULL(isolate_);
// Register the isolate on the platform before the isolate gets initialized,
// so that the isolate can access the platform during initialization.
platform->RegisterIsolate(isolate_, event_loop);
SetIsolateCreateParamsForNode(isolate_params_.get());
Isolate::Initialize(isolate_, *isolate_params_);

// If the indexes are not nullptr, we are not deserializing
isolate_data_ = std::make_unique<IsolateData>(
Expand All @@ -91,13 +87,7 @@ NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data,
platform,
array_buffer_allocator_.get(),
snapshot_data == nullptr ? nullptr : &(snapshot_data->isolate_data_info));
IsolateSettings s;
SetIsolateMiscHandlers(isolate_, s);
if (snapshot_data == nullptr) {
// If in deserialize mode, delay until after the deserialization is
// complete.
SetIsolateErrorHandlers(isolate_, s);
}

isolate_data_->max_young_gen_size =
isolate_params_->constraints.max_young_generation_size_in_bytes();
}
Expand Down
12 changes: 3 additions & 9 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -147,21 +147,15 @@ class WorkerThreadData {
ArrayBufferAllocator::Create();
Isolate::CreateParams params;
SetIsolateCreateParamsForNode(&params);
params.array_buffer_allocator_shared = allocator;

if (w->snapshot_data() != nullptr) {
SnapshotBuilder::InitializeIsolateParams(w->snapshot_data(), &params);
}
w->UpdateResourceConstraints(&params.constraints);

Isolate* isolate = Isolate::Allocate();
params.array_buffer_allocator_shared = allocator;
Isolate* isolate =
NewIsolate(&params, &loop_, w->platform_, w->snapshot_data());
if (isolate == nullptr) {
w->Exit(1, "ERR_WORKER_INIT_FAILED", "Failed to create new Isolate");
return;
}

w->platform_->RegisterIsolate(isolate, &loop_);
Isolate::Initialize(isolate, params);
SetIsolateUpForNode(isolate);

// Be sure it's called before Environment::InitializeDiagnostics()
Expand Down
76 changes: 76 additions & 0 deletions test/addons/new-isolate-addon/binding.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#include <assert.h>
#include <node.h>

using node::Environment;
using node::MultiIsolatePlatform;
using v8::Context;
using v8::FunctionCallbackInfo;
using v8::HandleScope;
using v8::Isolate;
using v8::Local;
using v8::Locker;
using v8::MaybeLocal;
using v8::Object;
using v8::SharedArrayBuffer;
using v8::String;
using v8::Unlocker;
using v8::Value;

void RunInSeparateIsolate(const FunctionCallbackInfo<Value>& args) {
Isolate* parent_isolate = args.GetIsolate();

assert(args[0]->IsString());
String::Utf8Value code(parent_isolate, args[0]);
assert(*code != nullptr);
assert(args[1]->IsSharedArrayBuffer());
auto arg_bs = args[1].As<SharedArrayBuffer>()->GetBackingStore();

Environment* parent_env =
node::GetCurrentEnvironment(parent_isolate->GetCurrentContext());
assert(parent_env != nullptr);
MultiIsolatePlatform* platform = node::GetMultiIsolatePlatform(parent_env);
assert(parent_env != nullptr);

{
Unlocker unlocker(parent_isolate);

std::vector<std::string> errors;
const std::vector<std::string> empty_args;
auto setup =
node::CommonEnvironmentSetup::Create(platform,
&errors,
empty_args,
empty_args,
node::EnvironmentFlags::kNoFlags);
assert(setup);

{
Locker locker(setup->isolate());
Isolate::Scope isolate_scope(setup->isolate());
HandleScope handle_scope(setup->isolate());
Context::Scope context_scope(setup->context());
auto arg = SharedArrayBuffer::New(setup->isolate(), arg_bs);
auto result = setup->context()->Global()->Set(
setup->context(),
v8::String::NewFromUtf8Literal(setup->isolate(), "arg"),
arg);
assert(!result.IsNothing());

MaybeLocal<Value> loadenv_ret =
node::LoadEnvironment(setup->env(), *code);
assert(!loadenv_ret.IsEmpty());

(void)node::SpinEventLoop(setup->env());

node::Stop(setup->env());
}
}
}

void Initialize(Local<Object> exports,
Local<Value> module,
Local<Context> context) {
NODE_SET_METHOD(exports, "runInSeparateIsolate", RunInSeparateIsolate);
}

NODE_MODULE_CONTEXT_AWARE(NODE_GYP_MODULE_NAME, Initialize)
9 changes: 9 additions & 0 deletions test/addons/new-isolate-addon/binding.gyp
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
'targets': [
{
'target_name': 'binding',
'sources': [ 'binding.cc' ],
'includes': ['../common.gypi'],
}
]
}
6 changes: 6 additions & 0 deletions test/addons/new-isolate-addon/test-nonodesnapshot.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Flags: --no-node-snapshot
'use strict';
require('../../common');

// Just re-execute the main test.
require('./test');
8 changes: 8 additions & 0 deletions test/addons/new-isolate-addon/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict';
const common = require('../../common');
const binding = require(`./build/${common.buildType}/binding`);
const assert = require('assert');

const arg = new SharedArrayBuffer(1);
binding.runInSeparateIsolate('const arr = new Uint8Array(arg); arr[0] = 0x42;', arg);
assert.deepStrictEqual(new Uint8Array(arg), new Uint8Array([0x42]));

0 comments on commit 27d6a8b

Please sign in to comment.