Skip to content

Commit c3cd453

Browse files
bnoordhuisbnoordhuis
committed
src: make IsolateData creation explicit
Make it easier to reason about the lifetime and the ownership of the IsolateData instance by making its creation explicit and by removing reference counting logic. The creator of the Environment is now responsible for passing in the IsolateData instance and for keeping it alive as long as the Environment is alive. PR-URL: #7082 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
1 parent 0301ce9 commit c3cd453

File tree

5 files changed

+63
-113
lines changed

5 files changed

+63
-113
lines changed

‎src/debug-agent.cc‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,9 +172,15 @@ void Agent::WorkerRun() {
172172
Local<Context> context = Context::New(isolate);
173173

174174
Context::Scope context_scope(context);
175+
176+
// FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences
177+
// a nullptr when a context hasn't been entered first. The symbol registry
178+
// is a lazily created JS object but object instantiation does not work
179+
// without a context.
180+
IsolateData isolate_data(isolate, &child_loop_);
181+
175182
Environment* env = CreateEnvironment(
176-
isolate,
177-
&child_loop_,
183+
&isolate_data,
178184
context,
179185
arraysize(argv),
180186
argv,

‎src/env-inl.h‎

Lines changed: 13 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -15,28 +15,6 @@
1515

1616
namespace node {
1717

18-
inline IsolateData* IsolateData::Get(v8::Isolate* isolate) {
19-
return static_cast<IsolateData*>(isolate->GetData(kIsolateSlot));
20-
}
21-
22-
inline IsolateData* IsolateData::GetOrCreate(v8::Isolate* isolate,
23-
uv_loop_t* loop) {
24-
IsolateData* isolate_data = Get(isolate);
25-
if (isolate_data == nullptr) {
26-
isolate_data = new IsolateData(isolate, loop);
27-
isolate->SetData(kIsolateSlot, isolate_data);
28-
}
29-
isolate_data->ref_count_ += 1;
30-
return isolate_data;
31-
}
32-
33-
inline void IsolateData::Put() {
34-
if (--ref_count_ == 0) {
35-
isolate()->SetData(kIsolateSlot, nullptr);
36-
delete this;
37-
}
38-
}
39-
4018
// Create string properties as internalized one byte strings.
4119
//
4220
// Internalized because it makes property lookups a little faster and because
@@ -46,9 +24,8 @@ inline void IsolateData::Put() {
4624
//
4725
// One byte because our strings are ASCII and we can safely skip V8's UTF-8
4826
// decoding step. It's a one-time cost, but why pay it when you don't have to?
49-
inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* loop)
50-
: event_loop_(loop),
51-
isolate_(isolate),
27+
inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop)
28+
:
5229
#define V(PropertyName, StringValue) \
5330
PropertyName ## _( \
5431
isolate, \
@@ -71,16 +48,12 @@ inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* loop)
7148
sizeof(StringValue) - 1).ToLocalChecked()),
7249
PER_ISOLATE_STRING_PROPERTIES(V)
7350
#undef V
74-
ref_count_(0) {}
51+
isolate_(isolate), event_loop_(event_loop) {}
7552

7653
inline uv_loop_t* IsolateData::event_loop() const {
7754
return event_loop_;
7855
}
7956

80-
inline v8::Isolate* IsolateData::isolate() const {
81-
return isolate_;
82-
}
83-
8457
inline Environment::AsyncHooks::AsyncHooks() {
8558
for (int i = 0; i < kFieldsCount; i++) fields_[i] = 0;
8659
}
@@ -176,9 +149,9 @@ inline void Environment::ArrayBufferAllocatorInfo::reset_fill_flag() {
176149
fields_[kNoZeroFill] = 0;
177150
}
178151

179-
inline Environment* Environment::New(v8::Local<v8::Context> context,
180-
uv_loop_t* loop) {
181-
Environment* env = new Environment(context, loop);
152+
inline Environment* Environment::New(IsolateData* isolate_data,
153+
v8::Local<v8::Context> context) {
154+
Environment* env = new Environment(isolate_data, context);
182155
env->AssignToContext(context);
183156
return env;
184157
}
@@ -212,11 +185,11 @@ inline Environment* Environment::GetCurrent(
212185
return static_cast<Environment*>(data.As<v8::External>()->Value());
213186
}
214187

215-
inline Environment::Environment(v8::Local<v8::Context> context,
216-
uv_loop_t* loop)
188+
inline Environment::Environment(IsolateData* isolate_data,
189+
v8::Local<v8::Context> context)
217190
: isolate_(context->GetIsolate()),
218-
isolate_data_(IsolateData::GetOrCreate(context->GetIsolate(), loop)),
219-
timer_base_(uv_now(loop)),
191+
isolate_data_(isolate_data),
192+
timer_base_(uv_now(isolate_data->event_loop())),
220193
using_domains_(false),
221194
printed_error_(false),
222195
trace_sync_io_(false),
@@ -253,7 +226,6 @@ inline Environment::~Environment() {
253226
#define V(PropertyName, TypeName) PropertyName ## _.Reset();
254227
ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
255228
#undef V
256-
isolate_data()->Put();
257229

258230
delete[] heap_statistics_buffer_;
259231
delete[] heap_space_statistics_buffer_;
@@ -541,9 +513,9 @@ inline v8::Local<v8::Object> Environment::NewInternalFieldObject() {
541513
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
542514
#define V(TypeName, PropertyName, StringValue) \
543515
inline \
544-
v8::Local<TypeName> IsolateData::PropertyName() const { \
516+
v8::Local<TypeName> IsolateData::PropertyName(v8::Isolate* isolate) const { \
545517
/* Strings are immutable so casting away const-ness here is okay. */ \
546-
return const_cast<IsolateData*>(this)->PropertyName ## _.Get(isolate()); \
518+
return const_cast<IsolateData*>(this)->PropertyName ## _.Get(isolate); \
547519
}
548520
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
549521
PER_ISOLATE_STRING_PROPERTIES(VS)
@@ -555,7 +527,7 @@ inline v8::Local<v8::Object> Environment::NewInternalFieldObject() {
555527
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
556528
#define V(TypeName, PropertyName, StringValue) \
557529
inline v8::Local<TypeName> Environment::PropertyName() const { \
558-
return isolate_data()->PropertyName(); \
530+
return isolate_data()->PropertyName(isolate()); \
559531
}
560532
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
561533
PER_ISOLATE_STRING_PROPERTIES(VS)

‎src/env.h‎

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -304,30 +304,20 @@ RB_HEAD(ares_task_list, ares_task_t);
304304

305305
class IsolateData {
306306
public:
307-
static inline IsolateData* GetOrCreate(v8::Isolate* isolate, uv_loop_t* loop);
308-
inline void Put();
307+
inline IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop);
309308
inline uv_loop_t* event_loop() const;
310309

311310
#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue)
312311
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
313312
#define V(TypeName, PropertyName, StringValue) \
314-
inline v8::Local<TypeName> PropertyName() const;
313+
inline v8::Local<TypeName> PropertyName(v8::Isolate* isolate) const;
315314
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
316315
PER_ISOLATE_STRING_PROPERTIES(VS)
317316
#undef V
318317
#undef VS
319318
#undef VP
320319

321320
private:
322-
static const int kIsolateSlot = NODE_ISOLATE_SLOT;
323-
324-
inline static IsolateData* Get(v8::Isolate* isolate);
325-
inline explicit IsolateData(v8::Isolate* isolate, uv_loop_t* loop);
326-
inline v8::Isolate* isolate() const;
327-
328-
uv_loop_t* const event_loop_;
329-
v8::Isolate* const isolate_;
330-
331321
#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue)
332322
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
333323
#define V(TypeName, PropertyName, StringValue) \
@@ -338,7 +328,8 @@ class IsolateData {
338328
#undef VS
339329
#undef VP
340330

341-
unsigned int ref_count_;
331+
v8::Isolate* const isolate_;
332+
uv_loop_t* const event_loop_;
342333

343334
DISALLOW_COPY_AND_ASSIGN(IsolateData);
344335
};
@@ -474,8 +465,8 @@ class Environment {
474465
const v8::PropertyCallbackInfo<T>& info);
475466

476467
// See CreateEnvironment() in src/node.cc.
477-
static inline Environment* New(v8::Local<v8::Context> context,
478-
uv_loop_t* loop);
468+
static inline Environment* New(IsolateData* isolate_data,
469+
v8::Local<v8::Context> context);
479470
inline void CleanupHandles();
480471
inline void Dispose();
481472

@@ -609,7 +600,7 @@ class Environment {
609600
static const int kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX;
610601

611602
private:
612-
inline Environment(v8::Local<v8::Context> context, uv_loop_t* loop);
603+
inline Environment(IsolateData* isolate_data, v8::Local<v8::Context> context);
613604
inline ~Environment();
614605
inline IsolateData* isolate_data() const;
615606

‎src/node.cc‎

Lines changed: 28 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -4264,42 +4264,6 @@ int EmitExit(Environment* env) {
42644264
}
42654265

42664266

4267-
// Just a convenience method
4268-
Environment* CreateEnvironment(Isolate* isolate,
4269-
Local<Context> context,
4270-
int argc,
4271-
const char* const* argv,
4272-
int exec_argc,
4273-
const char* const* exec_argv) {
4274-
Environment* env;
4275-
Context::Scope context_scope(context);
4276-
4277-
env = CreateEnvironment(isolate,
4278-
uv_default_loop(),
4279-
context,
4280-
argc,
4281-
argv,
4282-
exec_argc,
4283-
exec_argv);
4284-
4285-
LoadEnvironment(env);
4286-
4287-
return env;
4288-
}
4289-
4290-
static Environment* CreateEnvironment(Isolate* isolate,
4291-
Local<Context> context,
4292-
NodeInstanceData* instance_data) {
4293-
return CreateEnvironment(isolate,
4294-
instance_data->event_loop(),
4295-
context,
4296-
instance_data->argc(),
4297-
instance_data->argv(),
4298-
instance_data->exec_argc(),
4299-
instance_data->exec_argv());
4300-
}
4301-
4302-
43034267
static void HandleCloseCb(uv_handle_t* handle) {
43044268
Environment* env = reinterpret_cast<Environment*>(handle->data);
43054269
env->FinishHandleCleanup(handle);
@@ -4314,17 +4278,27 @@ static void HandleCleanup(Environment* env,
43144278
}
43154279

43164280

4317-
Environment* CreateEnvironment(Isolate* isolate,
4318-
uv_loop_t* loop,
4281+
IsolateData* CreateIsolateData(Isolate* isolate, uv_loop_t* loop) {
4282+
return new IsolateData(isolate, loop);
4283+
}
4284+
4285+
4286+
void FreeIsolateData(IsolateData* isolate_data) {
4287+
delete isolate_data;
4288+
}
4289+
4290+
4291+
Environment* CreateEnvironment(IsolateData* isolate_data,
43194292
Local<Context> context,
43204293
int argc,
43214294
const char* const* argv,
43224295
int exec_argc,
43234296
const char* const* exec_argv) {
4297+
Isolate* isolate = context->GetIsolate();
43244298
HandleScope handle_scope(isolate);
43254299

43264300
Context::Scope context_scope(context);
4327-
Environment* env = Environment::New(context, loop);
4301+
Environment* env = Environment::New(isolate_data, context);
43284302

43294303
isolate->SetAutorunMicrotasks(false);
43304304

@@ -4412,10 +4386,23 @@ static void StartNodeInstance(void* arg) {
44124386
Isolate::Scope isolate_scope(isolate);
44134387
HandleScope handle_scope(isolate);
44144388
Local<Context> context = Context::New(isolate);
4415-
Environment* env = CreateEnvironment(isolate, context, instance_data);
4416-
array_buffer_allocator->set_env(env);
44174389
Context::Scope context_scope(context);
44184390

4391+
// FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences
4392+
// a nullptr when a context hasn't been entered first. The symbol registry
4393+
// is a lazily created JS object but object instantiation does not work
4394+
// without a context.
4395+
IsolateData isolate_data(isolate, instance_data->event_loop());
4396+
4397+
Environment* env = CreateEnvironment(&isolate_data,
4398+
context,
4399+
instance_data->argc(),
4400+
instance_data->argv(),
4401+
instance_data->exec_argc(),
4402+
instance_data->exec_argv());
4403+
4404+
array_buffer_allocator->set_env(env);
4405+
44194406
isolate->SetAbortOnUncaughtExceptionCallback(
44204407
ShouldAbortOnUncaughtException);
44214408

‎src/node.h‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -190,28 +190,22 @@ NODE_EXTERN void Init(int* argc,
190190
int* exec_argc,
191191
const char*** exec_argv);
192192

193+
class IsolateData;
193194
class Environment;
194195

195-
NODE_EXTERN Environment* CreateEnvironment(v8::Isolate* isolate,
196-
struct uv_loop_s* loop,
197-
v8::Local<v8::Context> context,
198-
int argc,
199-
const char* const* argv,
200-
int exec_argc,
201-
const char* const* exec_argv);
202-
NODE_EXTERN void LoadEnvironment(Environment* env);
203-
NODE_EXTERN void FreeEnvironment(Environment* env);
196+
NODE_EXTERN IsolateData* CreateIsolateData(v8::Isolate* isolate,
197+
struct uv_loop_s* loop);
198+
NODE_EXTERN void FreeIsolateData(IsolateData* isolate_data);
204199

205-
// NOTE: Calling this is the same as calling
206-
// CreateEnvironment() + LoadEnvironment() from above.
207-
// `uv_default_loop()` will be passed as `loop`.
208-
NODE_EXTERN Environment* CreateEnvironment(v8::Isolate* isolate,
200+
NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data,
209201
v8::Local<v8::Context> context,
210202
int argc,
211203
const char* const* argv,
212204
int exec_argc,
213205
const char* const* exec_argv);
214206

207+
NODE_EXTERN void LoadEnvironment(Environment* env);
208+
NODE_EXTERN void FreeEnvironment(Environment* env);
215209

216210
NODE_EXTERN void EmitBeforeExit(Environment* env);
217211
NODE_EXTERN int EmitExit(Environment* env);

0 commit comments

Comments
 (0)