Skip to content

Commit aac79df

Browse files
bnoordhuisbnoordhuis
committed
src: use stack-allocated Environment instances
Makes it easier to reason about the lifetime of the Environment object. PR-URL: nodejs#7090 Refs: nodejs#7082 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
1 parent 58cec4e commit aac79df

File tree

4 files changed

+51
-80
lines changed

4 files changed

+51
-80
lines changed

‎src/debug-agent.cc‎

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -174,31 +174,24 @@ void Agent::WorkerRun() {
174174
Local<Context> context = Context::New(isolate);
175175

176176
Context::Scope context_scope(context);
177-
Environment* env = CreateEnvironment(
178-
&isolate_data,
179-
context,
180-
arraysize(argv),
181-
argv,
182-
arraysize(argv),
183-
argv);
177+
Environment env(&isolate_data, context);
184178

185-
child_env_ = env;
179+
const bool start_profiler_idle_notifier = false;
180+
env.Start(arraysize(argv), argv,
181+
arraysize(argv), argv,
182+
start_profiler_idle_notifier);
183+
184+
child_env_ = &env;
186185

187186
// Expose API
188-
InitAdaptor(env);
189-
LoadEnvironment(env);
187+
InitAdaptor(&env);
188+
LoadEnvironment(&env);
190189

191-
CHECK_EQ(&child_loop_, env->event_loop());
190+
CHECK_EQ(&child_loop_, env.event_loop());
192191
uv_run(&child_loop_, UV_RUN_DEFAULT);
193192

194193
// Clean-up peristent
195194
api_.Reset();
196-
197-
// Clean-up all running handles
198-
env->CleanupHandles();
199-
200-
env->Dispose();
201-
env = nullptr;
202195
}
203196
isolate->Dispose();
204197
}

‎src/env-inl.h‎

Lines changed: 11 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -134,13 +134,6 @@ inline void Environment::TickInfo::set_index(uint32_t value) {
134134
fields_[kIndex] = value;
135135
}
136136

137-
inline Environment* Environment::New(IsolateData* isolate_data,
138-
v8::Local<v8::Context> context) {
139-
Environment* env = new Environment(isolate_data, context);
140-
env->AssignToContext(context);
141-
return env;
142-
}
143-
144137
inline void Environment::AssignToContext(v8::Local<v8::Context> context) {
145138
context->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex, this);
146139
}
@@ -184,6 +177,7 @@ inline Environment::Environment(IsolateData* isolate_data,
184177
#if HAVE_INSPECTOR
185178
inspector_agent_(this),
186179
#endif
180+
handle_cleanup_waiting_(0),
187181
http_parser_buffer_(nullptr),
188182
context_(context->GetIsolate(), context) {
189183
// We'll be creating new objects so make sure we've entered the context.
@@ -200,24 +194,12 @@ inline Environment::Environment(IsolateData* isolate_data,
200194
set_generic_internal_field_template(obj);
201195

202196
RB_INIT(&cares_task_list_);
203-
handle_cleanup_waiting_ = 0;
197+
AssignToContext(context);
204198
}
205199

206200
inline Environment::~Environment() {
207201
v8::HandleScope handle_scope(isolate());
208202

209-
context()->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex,
210-
nullptr);
211-
#define V(PropertyName, TypeName) PropertyName ## _.Reset();
212-
ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
213-
#undef V
214-
215-
delete[] heap_statistics_buffer_;
216-
delete[] heap_space_statistics_buffer_;
217-
delete[] http_parser_buffer_;
218-
}
219-
220-
inline void Environment::CleanupHandles() {
221203
while (HandleCleanup* hc = handle_cleanup_queue_.PopFront()) {
222204
handle_cleanup_waiting_++;
223205
hc->cb_(this, hc->handle_, hc->arg_);
@@ -226,10 +208,16 @@ inline void Environment::CleanupHandles() {
226208

227209
while (handle_cleanup_waiting_ != 0)
228210
uv_run(event_loop(), UV_RUN_ONCE);
229-
}
230211

231-
inline void Environment::Dispose() {
232-
delete this;
212+
context()->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex,
213+
nullptr);
214+
#define V(PropertyName, TypeName) PropertyName ## _.Reset();
215+
ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
216+
#undef V
217+
218+
delete[] heap_statistics_buffer_;
219+
delete[] heap_space_statistics_buffer_;
220+
delete[] http_parser_buffer_;
233221
}
234222

235223
inline v8::Isolate* Environment::isolate() const {

‎src/env.h‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -446,17 +446,14 @@ class Environment {
446446
static inline Environment* GetCurrent(
447447
const v8::PropertyCallbackInfo<T>& info);
448448

449-
// See CreateEnvironment() in src/node.cc.
450-
static inline Environment* New(IsolateData* isolate_data,
451-
v8::Local<v8::Context> context);
449+
inline Environment(IsolateData* isolate_data, v8::Local<v8::Context> context);
450+
inline ~Environment();
451+
452452
void Start(int argc,
453453
const char* const* argv,
454454
int exec_argc,
455455
const char* const* exec_argv,
456456
bool start_profiler_idle_notifier);
457-
inline void CleanupHandles();
458-
inline void Dispose();
459-
460457
void AssignToContext(v8::Local<v8::Context> context);
461458

462459
void StartProfilerIdleNotifier();
@@ -472,7 +469,7 @@ class Environment {
472469
inline uv_check_t* immediate_check_handle();
473470
inline uv_idle_t* immediate_idle_handle();
474471

475-
// Register clean-up cb to be called on env->Dispose()
472+
// Register clean-up cb to be called on environment destruction.
476473
inline void RegisterHandleCleanup(uv_handle_t* handle,
477474
HandleCleanupCb cb,
478475
void *arg);
@@ -584,9 +581,6 @@ class Environment {
584581
static const int kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX;
585582

586583
private:
587-
inline Environment(IsolateData* isolate_data, v8::Local<v8::Context> context);
588-
inline ~Environment();
589-
590584
v8::Isolate* const isolate_;
591585
IsolateData* const isolate_data_;
592586
uv_check_t immediate_check_handle_;

‎src/node.cc‎

Lines changed: 26 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -3363,12 +3363,6 @@ void LoadEnvironment(Environment* env) {
33633363
}
33643364

33653365

3366-
void FreeEnvironment(Environment* env) {
3367-
CHECK_NE(env, nullptr);
3368-
env->Dispose();
3369-
}
3370-
3371-
33723366
static void PrintHelp();
33733367

33743368
static bool ParseDebugOpt(const char* arg) {
@@ -4256,12 +4250,17 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
42564250
Isolate* isolate = context->GetIsolate();
42574251
HandleScope handle_scope(isolate);
42584252
Context::Scope context_scope(context);
4259-
Environment* env = Environment::New(isolate_data, context);
4253+
auto env = new Environment(isolate_data, context);
42604254
env->Start(argc, argv, exec_argc, exec_argv, v8_is_profiling);
42614255
return env;
42624256
}
42634257

42644258

4259+
void FreeEnvironment(Environment* env) {
4260+
delete env;
4261+
}
4262+
4263+
42654264
// Entry point for new node instances, also called directly for the main
42664265
// node instance.
42674266
static void StartNodeInstance(void* arg) {
@@ -4293,60 +4292,60 @@ static void StartNodeInstance(void* arg) {
42934292
array_buffer_allocator.zero_fill_field());
42944293
Local<Context> context = Context::New(isolate);
42954294
Context::Scope context_scope(context);
4296-
Environment* env = CreateEnvironment(&isolate_data,
4297-
context,
4298-
instance_data->argc(),
4299-
instance_data->argv(),
4300-
instance_data->exec_argc(),
4301-
instance_data->exec_argv());
4295+
Environment env(&isolate_data, context);
4296+
env.Start(instance_data->argc(),
4297+
instance_data->argv(),
4298+
instance_data->exec_argc(),
4299+
instance_data->exec_argv(),
4300+
v8_is_profiling);
43024301

43034302
isolate->SetAbortOnUncaughtExceptionCallback(
43044303
ShouldAbortOnUncaughtException);
43054304

43064305
// Start debug agent when argv has --debug
43074306
if (instance_data->use_debug_agent())
4308-
StartDebug(env, debug_wait_connect);
4307+
StartDebug(&env, debug_wait_connect);
43094308

43104309
{
4311-
Environment::AsyncCallbackScope callback_scope(env);
4312-
LoadEnvironment(env);
4310+
Environment::AsyncCallbackScope callback_scope(&env);
4311+
LoadEnvironment(&env);
43134312
}
43144313

4315-
env->set_trace_sync_io(trace_sync_io);
4314+
env.set_trace_sync_io(trace_sync_io);
43164315

43174316
// Enable debugger
43184317
if (instance_data->use_debug_agent())
4319-
EnableDebug(env);
4318+
EnableDebug(&env);
43204319

43214320
{
43224321
SealHandleScope seal(isolate);
43234322
bool more;
43244323
do {
43254324
v8::platform::PumpMessageLoop(default_platform, isolate);
4326-
more = uv_run(env->event_loop(), UV_RUN_ONCE);
4325+
more = uv_run(env.event_loop(), UV_RUN_ONCE);
43274326

43284327
if (more == false) {
43294328
v8::platform::PumpMessageLoop(default_platform, isolate);
4330-
EmitBeforeExit(env);
4329+
EmitBeforeExit(&env);
43314330

43324331
// Emit `beforeExit` if the loop became alive either after emitting
43334332
// event, or after running some callbacks.
4334-
more = uv_loop_alive(env->event_loop());
4335-
if (uv_run(env->event_loop(), UV_RUN_NOWAIT) != 0)
4333+
more = uv_loop_alive(env.event_loop());
4334+
if (uv_run(env.event_loop(), UV_RUN_NOWAIT) != 0)
43364335
more = true;
43374336
}
43384337
} while (more == true);
43394338
}
43404339

4341-
env->set_trace_sync_io(false);
4340+
env.set_trace_sync_io(false);
43424341

4343-
int exit_code = EmitExit(env);
4342+
int exit_code = EmitExit(&env);
43444343
if (instance_data->is_main())
43454344
instance_data->set_exit_code(exit_code);
4346-
RunAtExit(env);
4345+
RunAtExit(&env);
43474346

43484347
#if HAVE_INSPECTOR
4349-
if (env->inspector_agent()->connected()) {
4348+
if (env.inspector_agent()->connected()) {
43504349
// Restore signal dispositions, the app is done and is no longer
43514350
// capable of handling signals.
43524351
#ifdef __POSIX__
@@ -4359,16 +4358,13 @@ static void StartNodeInstance(void* arg) {
43594358
CHECK_EQ(0, sigaction(nr, &act, nullptr));
43604359
}
43614360
#endif
4362-
env->inspector_agent()->WaitForDisconnect();
4361+
env.inspector_agent()->WaitForDisconnect();
43634362
}
43644363
#endif
43654364

43664365
#if defined(LEAK_SANITIZER)
43674366
__lsan_do_leak_check();
43684367
#endif
4369-
4370-
env->Dispose();
4371-
env = nullptr;
43724368
}
43734369

43744370
uv_mutex_lock(&node_isolate_mutex);

0 commit comments

Comments
 (0)