From f2eecf434a78f05e3a5fc53c9669dce5a1d17353 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 8 Feb 2019 19:22:35 +0100 Subject: [PATCH 1/7] worker: pre-allocate thread id Allocate a thread id before actually creating the Environment instance. --- src/env.cc | 10 ++++++++-- src/env.h | 5 ++++- src/node_worker.cc | 7 ++++--- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/env.cc b/src/env.cc index 2e0fa251b35350..3c9b4024591e6a 100644 --- a/src/env.cc +++ b/src/env.cc @@ -169,9 +169,14 @@ void Environment::TrackingTraceStateObserver::UpdateTraceCategoryState() { static std::atomic next_thread_id{0}; +uint64_t Environment::AllocateThreadId() { + return next_thread_id++; +} + Environment::Environment(IsolateData* isolate_data, Local context, - Flags flags) + Flags flags, + uint64_t thread_id) : isolate_(context->GetIsolate()), isolate_data_(isolate_data), immediate_info_(context->GetIsolate()), @@ -181,7 +186,8 @@ Environment::Environment(IsolateData* isolate_data, trace_category_state_(isolate_, kTraceCategoryCount), stream_base_state_(isolate_, StreamBase::kNumStreamBaseStateFields), flags_(flags), - thread_id_(next_thread_id++), + thread_id_(thread_id == static_cast(-1) ? + AllocateThreadId() : thread_id), fs_stats_field_array_(isolate_, kFsStatsBufferLength), fs_stats_field_bigint_array_(isolate_, kFsStatsBufferLength), context_(context->GetIsolate(), context) { diff --git a/src/env.h b/src/env.h index 40bd0797a2ffd0..9df0147fb756ee 100644 --- a/src/env.h +++ b/src/env.h @@ -618,7 +618,8 @@ class Environment { Environment(IsolateData* isolate_data, v8::Local context, - Flags flags = Flags()); + Flags flags = Flags(), + uint64_t thread_id = static_cast(-1)); ~Environment(); void Start(bool start_profiler_idle_notifier); @@ -769,6 +770,8 @@ class Environment { inline bool has_run_bootstrapping_code() const; inline void set_has_run_bootstrapping_code(bool has_run_bootstrapping_code); + static uint64_t AllocateThreadId(); + inline bool is_main_thread() const; inline bool owns_process_state() const; inline bool owns_inspector() const; diff --git a/src/node_worker.cc b/src/node_worker.cc index 3fd19de97ce3de..36b4106d137eb5 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -71,7 +71,8 @@ Worker::Worker(Environment* env, Local wrap, const std::string& url, std::shared_ptr per_isolate_opts) - : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_WORKER), url_(url) { + : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_WORKER), url_(url), + thread_id_(Environment::AllocateThreadId()) { Debug(this, "Creating new worker instance at %p", static_cast(this)); // Set up everything that needs to be set up in the parent environment. @@ -114,11 +115,11 @@ Worker::Worker(Environment* env, Context::Scope context_scope(context); // TODO(addaleax): Use CreateEnvironment(), or generally another public API. - env_.reset(new Environment(isolate_data_.get(), context)); + env_.reset(new Environment( + isolate_data_.get(), context, Flags::kNoFlags, thread_id_)); CHECK_NOT_NULL(env_); env_->set_abort_on_uncaught_exception(false); env_->set_worker_context(this); - thread_id_ = env_->thread_id(); env_->Start(env->profiler_idle_notifier_started()); env_->ProcessCliArgs(std::vector{}, From 406c8058fed5f55de3eb6b0a3c566c5aa7e4ee9b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 8 Feb 2019 19:23:09 +0100 Subject: [PATCH 2/7] worker: set up child Isolate inside Worker thread Refs: https://github.com/nodejs/node/issues/24016 --- src/inspector_agent.cc | 14 +- src/inspector_agent.h | 4 +- src/node_worker.cc | 330 +++++++++++++++++++++-------------------- src/node_worker.h | 22 ++- 4 files changed, 198 insertions(+), 172 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 272f1a986da71f..8d7aad70e600e4 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -885,12 +885,14 @@ bool Agent::IsActive() { return io_ != nullptr || client_->IsActive(); } -void Agent::AddWorkerInspector(int thread_id, - const std::string& url, - Agent* agent) { - CHECK_NOT_NULL(client_); - agent->parent_handle_ = - client_->getWorkerManager()->NewParentHandle(thread_id, url); +void Agent::SetParentHandle( + std::unique_ptr parent_handle) { + parent_handle_ = std::move(parent_handle); +} + +std::unique_ptr Agent::GetParentHandle( + int thread_id, const std::string& url) { + return client_->getWorkerManager()->NewParentHandle(thread_id, url); } void Agent::WaitForConnect() { diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 5e599a6339e903..905b1e2841ebc8 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -85,7 +85,9 @@ class Agent { void EnableAsyncHook(); void DisableAsyncHook(); - void AddWorkerInspector(int thread_id, const std::string& url, Agent* agent); + void SetParentHandle(std::unique_ptr parent_handle); + std::unique_ptr GetParentHandle( + int thread_id, const std::string& url); // Called to create inspector sessions that can be used from the main thread. // The inspector responds by using the delegate to send messages back. diff --git a/src/node_worker.cc b/src/node_worker.cc index 36b4106d137eb5..9da5622f5e2d18 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -8,6 +8,10 @@ #include "async_wrap.h" #include "async_wrap-inl.h" +#if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR +#include "inspector/worker_inspector.h" // ParentInspectorHandle +#endif + #include #include @@ -35,34 +39,21 @@ namespace worker { namespace { #if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR -void StartWorkerInspector(Environment* child, const std::string& url) { +void StartWorkerInspector( + Environment* child, + std::unique_ptr parent_handle, + const std::string& url) { + child->inspector_agent()->SetParentHandle(std::move(parent_handle)); child->inspector_agent()->Start(url, child->options()->debug_options(), child->inspector_host_port(), false); } -void AddWorkerInspector(Environment* parent, - Environment* child, - int id, - const std::string& url) { - parent->inspector_agent()->AddWorkerInspector(id, url, - child->inspector_agent()); -} - void WaitForWorkerInspectorToStop(Environment* child) { child->inspector_agent()->WaitForDisconnect(); child->inspector_agent()->Stop(); } - -#else -// No-ops -void StartWorkerInspector(Environment* child, const std::string& url) {} -void AddWorkerInspector(Environment* parent, - Environment* child, - int id, - const std::string& url) {} -void WaitForWorkerInspectorToStop(Environment* child) {} #endif } // anonymous namespace @@ -71,9 +62,13 @@ Worker::Worker(Environment* env, Local wrap, const std::string& url, std::shared_ptr per_isolate_opts) - : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_WORKER), url_(url), + : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_WORKER), + url_(url), + per_isolate_opts_(per_isolate_opts), + platform_(env->isolate_data()->platform()), + profiler_idle_notifier_started_(env->profiler_idle_notifier_started()), thread_id_(Environment::AllocateThreadId()) { - Debug(this, "Creating new worker instance at %p", static_cast(this)); + Debug(this, "Creating new worker instance with thread id %llu", thread_id_); // Set up everything that needs to be set up in the parent environment. parent_port_ = MessagePort::New(env, env->context()); @@ -89,57 +84,17 @@ Worker::Worker(Environment* env, env->message_port_string(), parent_port_->object()).FromJust(); - array_buffer_allocator_.reset(CreateArrayBufferAllocator()); - - CHECK_EQ(uv_loop_init(&loop_), 0); - isolate_ = NewIsolate(array_buffer_allocator_.get(), &loop_); - CHECK_NOT_NULL(isolate_); - - { - // Enter an environment capable of executing code in the child Isolate - // (and only in it). - Locker locker(isolate_); - Isolate::Scope isolate_scope(isolate_); - HandleScope handle_scope(isolate_); - - isolate_data_.reset(CreateIsolateData(isolate_, - &loop_, - env->isolate_data()->platform(), - array_buffer_allocator_.get())); - if (per_isolate_opts != nullptr) { - isolate_data_->set_options(per_isolate_opts); - } - CHECK(isolate_data_); - - Local context = NewContext(isolate_); - Context::Scope context_scope(context); - - // TODO(addaleax): Use CreateEnvironment(), or generally another public API. - env_.reset(new Environment( - isolate_data_.get(), context, Flags::kNoFlags, thread_id_)); - CHECK_NOT_NULL(env_); - env_->set_abort_on_uncaught_exception(false); - env_->set_worker_context(this); - - env_->Start(env->profiler_idle_notifier_started()); - env_->ProcessCliArgs(std::vector{}, - std::vector{}); - // Done while on the parent thread - AddWorkerInspector(env, env_.get(), thread_id_, url_); - } - - // The new isolate won't be bothered on this thread again. - isolate_->DiscardThreadSpecificMetadata(); - - wrap->Set(env->context(), - env->thread_id_string(), - Number::New(env->isolate(), static_cast(thread_id_))) + object()->Set(env->context(), + env->thread_id_string(), + Number::New(env->isolate(), static_cast(thread_id_))) .FromJust(); - Debug(this, - "Set up worker at %p with id %llu", - static_cast(this), - thread_id_); +#if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR + inspector_parent_handle_ = + env->inspector_agent()->GetParentHandle(thread_id_, url); +#endif + + Debug(this, "Preparation for worker %llu finished", thread_id_); } bool Worker::is_stopped() const { @@ -147,14 +102,76 @@ bool Worker::is_stopped() const { return stopped_; } +class WorkerThreadData { + public: + explicit WorkerThreadData(Worker* w) + : w_(w), + array_buffer_allocator_(CreateArrayBufferAllocator()) { + CHECK_EQ(uv_loop_init(&loop_), 0); + + Isolate* isolate = NewIsolate(array_buffer_allocator_.get(), &loop_); + CHECK_NOT_NULL(isolate); + + { + Locker locker(isolate); + Isolate::Scope isolate_scope(isolate); + HandleScope handle_scope(isolate); + isolate_data_.reset(CreateIsolateData(isolate, + &loop_, + w_->platform_, + array_buffer_allocator_.get())); + CHECK(isolate_data_); + if (w_->per_isolate_opts_) + isolate_data_->set_options(std::move(w_->per_isolate_opts_)); + } + + Mutex::ScopedLock lock(w_->mutex_); + w_->isolate_ = isolate; + } + + ~WorkerThreadData() { + Debug(w_, "Worker %llu dispose isolate", w_->thread_id_); + Isolate* isolate; + { + Mutex::ScopedLock lock(w_->mutex_); + isolate = w_->isolate_; + w_->isolate_ = nullptr; + } + + w_->platform_->CancelPendingDelayedTasks(isolate); + + isolate_data_.reset(); + w_->platform_->UnregisterIsolate(isolate); + + isolate->Dispose(); + + // Need to run the loop one more time to close the platform's uv_async_t + uv_run(&loop_, UV_RUN_ONCE); + + CheckedUvLoopClose(&loop_); + } + + private: + Worker* w_; + uv_loop_t loop_; + DeleteFnPtr + array_buffer_allocator_; + DeleteFnPtr isolate_data_; + + friend class Worker; +}; + void Worker::Run() { std::string name = "WorkerThread "; name += std::to_string(thread_id_); TRACE_EVENT_METADATA1( "__metadata", "thread_name", "name", TRACE_STR_COPY(name.c_str())); - MultiIsolatePlatform* platform = isolate_data_->platform(); - CHECK_NOT_NULL(platform); + CHECK_NOT_NULL(platform_); + + Debug(this, "Creating isolate for worker with id %llu", thread_id_); + + WorkerThreadData data(this); Debug(this, "Starting worker with id %llu", thread_id_); { @@ -163,10 +180,73 @@ void Worker::Run() { SealHandleScope outer_seal(isolate_); bool inspector_started = false; + DeleteFnPtr env_; + OnScopeLeave cleanup_env([&]{ + if (!env_) return; + env_->set_can_call_into_js(false); + Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_, + Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE); + + // Grab the parent-to-child channel and render is unusable. + MessagePort* child_port; + { + Mutex::ScopedLock lock(mutex_); + child_port = child_port_; + child_port_ = nullptr; + } + + { + Context::Scope context_scope(env_->context()); + if (child_port != nullptr) + child_port->Close(); + env_->stop_sub_worker_contexts(); + env_->RunCleanup(); + RunAtExit(env_.get()); +#if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR + if (inspector_started) + WaitForWorkerInspectorToStop(env_.get()); +#endif + + { + Mutex::ScopedLock stopped_lock(stopped_mutex_); + stopped_ = true; + } + + env_->RunCleanup(); + + // This call needs to be made while the `Environment` is still alive + // because we assume that it is available for async tracking in the + // NodePlatform implementation. + platform_->DrainTasks(isolate_); + } + }); + { - Context::Scope context_scope(env_->context()); HandleScope handle_scope(isolate_); + Local context = NewContext(isolate_); + if (is_stopped()) return; + + CHECK(!context.IsEmpty()); + Context::Scope context_scope(context); + { + // TODO(addaleax): Use CreateEnvironment(), or generally another + // public API. + env_.reset(new Environment(data.isolate_data_.get(), + context, + Environment::kNoFlags, + thread_id_)); + CHECK_NOT_NULL(env_); + env_->set_abort_on_uncaught_exception(false); + env_->set_worker_context(this); + + env_->Start(profiler_idle_notifier_started_); + env_->ProcessCliArgs(std::vector{}, + std::vector{}); + } + + Debug(this, "Created Environment for worker with id %llu", thread_id_); + if (is_stopped()) return; { HandleScope handle_scope(isolate_); Mutex::ScopedLock lock(mutex_); @@ -182,8 +262,13 @@ void Worker::Run() { Debug(this, "Created message port for worker %llu", thread_id_); } - if (!is_stopped()) { - StartWorkerInspector(env_.get(), url_); + if (is_stopped()) return; + { +#if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR + StartWorkerInspector(env_.get(), + std::move(inspector_parent_handle_), + url_); +#endif inspector_started = true; HandleScope handle_scope(isolate_); @@ -198,6 +283,7 @@ void Worker::Run() { Debug(this, "Loaded environment for worker %llu", thread_id_); } + if (is_stopped()) return; { SealHandleScope seal(isolate_); bool more; @@ -205,12 +291,12 @@ void Worker::Run() { node::performance::NODE_PERFORMANCE_MILESTONE_LOOP_START); do { if (is_stopped()) break; - uv_run(&loop_, UV_RUN_DEFAULT); + uv_run(&data.loop_, UV_RUN_DEFAULT); if (is_stopped()) break; - platform->DrainTasks(isolate_); + platform_->DrainTasks(isolate_); - more = uv_loop_alive(&loop_); + more = uv_loop_alive(&data.loop_); if (more && !is_stopped()) continue; @@ -218,7 +304,7 @@ void Worker::Run() { // Emit `beforeExit` if the loop became alive either after emitting // event, or after running some callbacks. - more = uv_loop_alive(&loop_); + more = uv_loop_alive(&data.loop_); } while (more == true); env_->performance_state()->Mark( node::performance::NODE_PERFORMANCE_MILESTONE_LOOP_EXIT); @@ -237,79 +323,11 @@ void Worker::Run() { Debug(this, "Exiting thread for worker %llu with exit code %d", thread_id_, exit_code_); } - - env_->set_can_call_into_js(false); - Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_, - Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE); - - // Grab the parent-to-child channel and render is unusable. - MessagePort* child_port; - { - Mutex::ScopedLock lock(mutex_); - child_port = child_port_; - child_port_ = nullptr; - } - - { - Context::Scope context_scope(env_->context()); - child_port->Close(); - env_->stop_sub_worker_contexts(); - env_->RunCleanup(); - RunAtExit(env_.get()); - if (inspector_started) - WaitForWorkerInspectorToStop(env_.get()); - - { - Mutex::ScopedLock stopped_lock(stopped_mutex_); - stopped_ = true; - } - - env_->RunCleanup(); - - // This call needs to be made while the `Environment` is still alive - // because we assume that it is available for async tracking in the - // NodePlatform implementation. - platform->DrainTasks(isolate_); - } - - env_.reset(); - } - - DisposeIsolate(); - - { - Mutex::ScopedLock lock(mutex_); - CHECK(thread_exit_async_); - scheduled_on_thread_stopped_ = true; - uv_async_send(thread_exit_async_.get()); } Debug(this, "Worker %llu thread stops", thread_id_); } -void Worker::DisposeIsolate() { - if (env_) { - CHECK_NOT_NULL(isolate_); - Locker locker(isolate_); - Isolate::Scope isolate_scope(isolate_); - env_.reset(); - } - - if (isolate_ == nullptr) - return; - - Debug(this, "Worker %llu dispose isolate", thread_id_); - CHECK(isolate_data_); - MultiIsolatePlatform* platform = isolate_data_->platform(); - platform->CancelPendingDelayedTasks(isolate_); - - isolate_data_.reset(); - platform->UnregisterIsolate(isolate_); - - isolate_->Dispose(); - isolate_ = nullptr; -} - void Worker::JoinThread() { if (thread_joined_) return; @@ -340,7 +358,6 @@ void Worker::OnThreadStopped() { CHECK(stopped_); } - CHECK_NULL(child_port_); parent_port_ = nullptr; } @@ -370,16 +387,9 @@ Worker::~Worker() { CHECK(stopped_); CHECK(thread_joined_); - CHECK_NULL(child_port_); // This has most likely already happened within the worker thread -- this // is just in case Worker creation failed early. - DisposeIsolate(); - - // Need to run the loop one more time to close the platform's uv_async_t - uv_run(&loop_, UV_RUN_ONCE); - - CheckedUvLoopClose(&loop_); Debug(this, "Worker %llu destroyed", thread_id_); } @@ -476,7 +486,13 @@ void Worker::StartThread(const FunctionCallbackInfo& args) { }), 0); CHECK_EQ(uv_thread_create(&w->tid_, [](void* arg) { - static_cast(arg)->Run(); + Worker* w = static_cast(arg); + w->Run(); + + Mutex::ScopedLock lock(w->mutex_); + CHECK(w->thread_exit_async_); + w->scheduled_on_thread_stopped_ = true; + uv_async_send(w->thread_exit_async_.get()); }, static_cast(w)), 0); } @@ -510,12 +526,12 @@ void Worker::Exit(int code) { Debug(this, "Worker %llu called Exit(%d)", thread_id_, code); if (!stopped_) { - CHECK_NOT_NULL(env_); stopped_ = true; exit_code_ = code; if (child_port_ != nullptr) child_port_->StopEventLoop(); - isolate_->TerminateExecution(); + if (isolate_ != nullptr) + isolate_->TerminateExecution(); } } diff --git a/src/node_worker.h b/src/node_worker.h index 5ba9ceade3dc6b..4d7a7335ca6d63 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -9,6 +9,8 @@ namespace node { namespace worker { +class WorkerThreadData; + // A worker thread, as represented in its parent thread. class Worker : public AsyncWrap { public: @@ -49,17 +51,19 @@ class Worker : public AsyncWrap { private: void OnThreadStopped(); - void DisposeIsolate(); - uv_loop_t loop_; - DeleteFnPtr isolate_data_; - DeleteFnPtr env_; const std::string url_; + + std::shared_ptr per_isolate_opts_; + MultiIsolatePlatform* platform_; v8::Isolate* isolate_ = nullptr; - DeleteFnPtr - array_buffer_allocator_; + bool profiler_idle_notifier_started_; uv_thread_t tid_; +#if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR + std::unique_ptr inspector_parent_handle_; +#endif + // This mutex protects access to all variables listed below it. mutable Mutex mutex_; @@ -79,12 +83,14 @@ class Worker : public AsyncWrap { std::unique_ptr child_port_data_; - // The child port is always kept alive by the child Environment's persistent - // handle to it. + // The child port is kept alive by the child Environment's persistent + // handle to it, as long as that child Environment exists. MessagePort* child_port_ = nullptr; // This is always kept alive because the JS object associated with the Worker // instance refers to it via its [kPort] property. MessagePort* parent_port_ = nullptr; + + friend class WorkerThreadData; }; } // namespace worker From 2265a476cd1a1a3a22f821d2715b35e857a7e17c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 8 Feb 2019 19:13:35 +0100 Subject: [PATCH 3/7] test: add `Worker` + `--prof` regression test Fixes: https://github.com/nodejs/node/issues/24016 --- test/parallel/test-worker-prof.js | 41 +++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 test/parallel/test-worker-prof.js diff --git a/test/parallel/test-worker-prof.js b/test/parallel/test-worker-prof.js new file mode 100644 index 00000000000000..5b0703f5f9b5ca --- /dev/null +++ b/test/parallel/test-worker-prof.js @@ -0,0 +1,41 @@ +'use strict'; +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const fs = require('fs'); +const assert = require('assert'); +const { spawnSync } = require('child_process'); +const { Worker } = require('worker_threads'); + +if (!common.isMainThread) + common.skip('process.chdir is not available in Workers'); + +// Test that --prof also tracks Worker threads. +// Refs: https://github.com/nodejs/node/issues/24016 + +if (process.argv[2] === 'child') { + const spin = ` + const start = Date.now(); + while (Date.now() - start < 200); + `; + new Worker(spin, { eval: true }); + eval(spin); + return; +} + +tmpdir.refresh(); +process.chdir(tmpdir.path); +spawnSync(process.execPath, ['--prof', __filename, 'child']); +const logfiles = fs.readdirSync('.').filter((name) => /\.log$/.test(name)); +assert.strictEqual(logfiles.length, 2); // Parent thread + child thread. + +for (const logfile of logfiles) { + const lines = fs.readFileSync(logfile, 'utf8').split('\n'); + const ticks = lines.filter((line) => /^tick,/.test(line)).length; + + // Test that at least 20 ticks have been recorded for both parent and child + // threads. When not tracking Worker threads, only 1 or 2 ticks would + // have been recorded. + // When running locally on x64 Linux, this number is usually at least 150 + // for both threads, so 15 seems like a very safe threshold. + assert(ticks >= 15, `${ticks} >= 15`); +} From 482715b0704a8259d1a7a91d5957b43452ccf1cd Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 9 Feb 2019 20:42:39 +0100 Subject: [PATCH 4/7] fixup! worker: pre-allocate thread id --- src/env.cc | 3 +-- src/env.h | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/env.cc b/src/env.cc index 3c9b4024591e6a..5507fc7b7ba638 100644 --- a/src/env.cc +++ b/src/env.cc @@ -186,8 +186,7 @@ Environment::Environment(IsolateData* isolate_data, trace_category_state_(isolate_, kTraceCategoryCount), stream_base_state_(isolate_, StreamBase::kNumStreamBaseStateFields), flags_(flags), - thread_id_(thread_id == static_cast(-1) ? - AllocateThreadId() : thread_id), + thread_id_(thread_id == kNoThreadId ? AllocateThreadId() : thread_id), fs_stats_field_array_(isolate_, kFsStatsBufferLength), fs_stats_field_bigint_array_(isolate_, kFsStatsBufferLength), context_(context->GetIsolate(), context) { diff --git a/src/env.h b/src/env.h index 9df0147fb756ee..f3b372f12a4763 100644 --- a/src/env.h +++ b/src/env.h @@ -619,7 +619,7 @@ class Environment { Environment(IsolateData* isolate_data, v8::Local context, Flags flags = Flags(), - uint64_t thread_id = static_cast(-1)); + uint64_t thread_id = kNoThreadId); ~Environment(); void Start(bool start_profiler_idle_notifier); @@ -771,6 +771,7 @@ class Environment { inline void set_has_run_bootstrapping_code(bool has_run_bootstrapping_code); static uint64_t AllocateThreadId(); + static constexpr uint64_t kNoThreadId = -1; inline bool is_main_thread() const; inline bool owns_process_state() const; From 542d3bbecf18465d6300b239b427aa2b645bfd8a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 9 Feb 2019 20:42:55 +0100 Subject: [PATCH 5/7] fixup! worker: set up child Isolate inside Worker thread --- src/node_worker.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node_worker.cc b/src/node_worker.cc index 9da5622f5e2d18..0063b18eea435f 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -152,7 +152,7 @@ class WorkerThreadData { } private: - Worker* w_; + Worker* const w_; uv_loop_t loop_; DeleteFnPtr array_buffer_allocator_; @@ -181,7 +181,7 @@ void Worker::Run() { bool inspector_started = false; DeleteFnPtr env_; - OnScopeLeave cleanup_env([&]{ + OnScopeLeave cleanup_env([&]() { if (!env_) return; env_->set_can_call_into_js(false); Isolate::DisallowJavascriptExecutionScope disallow_js(isolate_, @@ -204,7 +204,7 @@ void Worker::Run() { RunAtExit(env_.get()); #if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR if (inspector_started) - WaitForWorkerInspectorToStop(env_.get()); + WaitForWorkerInspectorToStop(env_.get()); #endif { From 124ce9b794eb46aa431ffe73c69ff1ab130c86b2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 10 Feb 2019 00:32:28 +0100 Subject: [PATCH 6/7] fixup! fixup! worker: set up child Isolate inside Worker thread --- src/node_worker.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_worker.cc b/src/node_worker.cc index 0063b18eea435f..104cced2a3f2d5 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -203,8 +203,8 @@ void Worker::Run() { env_->RunCleanup(); RunAtExit(env_.get()); #if NODE_USE_V8_PLATFORM && HAVE_INSPECTOR - if (inspector_started) - WaitForWorkerInspectorToStop(env_.get()); + if (inspector_started) + WaitForWorkerInspectorToStop(env_.get()); #endif { From 557e241c4a23eed2a4dbb8d8bfcf045ea2d60e86 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 10 Feb 2019 14:03:20 +0100 Subject: [PATCH 7/7] fixup! worker: set up child Isolate inside Worker thread --- src/node_worker.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/node_worker.cc b/src/node_worker.cc index 104cced2a3f2d5..ebd1924b8f2479 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -102,6 +102,9 @@ bool Worker::is_stopped() const { return stopped_; } +// This class contains data that is only relevant to the child thread itself, +// and only while it is running. +// (Eventually, the Environment instance should probably also be moved here.) class WorkerThreadData { public: explicit WorkerThreadData(Worker* w)