Skip to content

Commit

Permalink
src: wrap HostPort in ExclusiveAccess
Browse files Browse the repository at this point in the history
I found it exceedingly hard to figure out if there is a race condition
where one thread reads the inspector agent's HostPort's properties while
another modifies them concurrently.

I think the answer is "no, there isn't" but with this commit use sites
are forced to unwrap the object (and acquire the mutex in the process),
making it a great deal easier to reason about correctness.

PR-URL: #31717
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
  • Loading branch information
bnoordhuis authored and codebytere committed Feb 17, 2020
1 parent e23023d commit 1dec9d1
Show file tree
Hide file tree
Showing 9 changed files with 36 additions and 21 deletions.
3 changes: 2 additions & 1 deletion src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -740,7 +740,8 @@ inline uint64_t Environment::heap_prof_interval() const {

#endif // HAVE_INSPECTOR

inline std::shared_ptr<HostPort> Environment::inspector_host_port() {
inline
std::shared_ptr<ExclusiveAccess<HostPort>> Environment::inspector_host_port() {
return inspector_host_port_;
}

Expand Down
3 changes: 2 additions & 1 deletion src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,8 @@ Environment::Environment(IsolateData* isolate_data,
// part of the per-Isolate option set, for which in turn the defaults are
// part of the per-process option set.
options_.reset(new EnvironmentOptions(*isolate_data->options()->per_env));
inspector_host_port_.reset(new HostPort(options_->debug_options().host_port));
inspector_host_port_.reset(
new ExclusiveAccess<HostPort>(options_->debug_options().host_port));

#if HAVE_INSPECTOR
// We can only create the inspector agent after having cloned the options.
Expand Down
4 changes: 2 additions & 2 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -1221,7 +1221,7 @@ class Environment : public MemoryRetainer {
void* data);

inline std::shared_ptr<EnvironmentOptions> options();
inline std::shared_ptr<HostPort> inspector_host_port();
inline std::shared_ptr<ExclusiveAccess<HostPort>> inspector_host_port();

// The BaseObject count is a debugging helper that makes sure that there are
// no memory leaks caused by BaseObjects staying alive longer than expected
Expand Down Expand Up @@ -1319,7 +1319,7 @@ class Environment : public MemoryRetainer {
// server starts listening), but when the inspector server starts
// the inspector_host_port_->port() will be the actual port being
// used.
std::shared_ptr<HostPort> inspector_host_port_;
std::shared_ptr<ExclusiveAccess<HostPort>> inspector_host_port_;
std::vector<std::string> exec_argv_;
std::vector<std::string> argv_;
std::string exec_path_;
Expand Down
2 changes: 1 addition & 1 deletion src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -757,7 +757,7 @@ Agent::~Agent() {}

bool Agent::Start(const std::string& path,
const DebugOptions& options,
std::shared_ptr<HostPort> host_port,
std::shared_ptr<ExclusiveAccess<HostPort>> host_port,
bool is_main) {
path_ = path;
debug_options_ = options;
Expand Down
6 changes: 3 additions & 3 deletions src/inspector_agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class Agent {
// Create client_, may create io_ if option enabled
bool Start(const std::string& path,
const DebugOptions& options,
std::shared_ptr<HostPort> host_port,
std::shared_ptr<ExclusiveAccess<HostPort>> host_port,
bool is_main);
// Stop and destroy io_
void Stop();
Expand Down Expand Up @@ -110,7 +110,7 @@ class Agent {
void RequestIoThreadStart();

const DebugOptions& options() { return debug_options_; }
std::shared_ptr<HostPort> host_port() { return host_port_; }
std::shared_ptr<ExclusiveAccess<HostPort>> host_port() { return host_port_; }
void ContextCreated(v8::Local<v8::Context> context, const ContextInfo& info);

// Interface for interacting with inspectors in worker threads
Expand All @@ -133,7 +133,7 @@ class Agent {
// pointer which is meant to store the actual host and port of the inspector
// server.
DebugOptions debug_options_;
std::shared_ptr<HostPort> host_port_;
std::shared_ptr<ExclusiveAccess<HostPort>> host_port_;

bool pending_enable_async_hook_ = false;
bool pending_disable_async_hook_ = false;
Expand Down
21 changes: 15 additions & 6 deletions src/inspector_io.cc
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ class InspectorIoDelegate: public node::inspector::SocketServerDelegate {
std::unique_ptr<InspectorIo> InspectorIo::Start(
std::shared_ptr<MainThreadHandle> main_thread,
const std::string& path,
std::shared_ptr<HostPort> host_port,
std::shared_ptr<ExclusiveAccess<HostPort>> host_port,
const InspectPublishUid& inspect_publish_uid) {
auto io = std::unique_ptr<InspectorIo>(
new InspectorIo(main_thread,
Expand All @@ -254,7 +254,7 @@ std::unique_ptr<InspectorIo> InspectorIo::Start(

InspectorIo::InspectorIo(std::shared_ptr<MainThreadHandle> main_thread,
const std::string& path,
std::shared_ptr<HostPort> host_port,
std::shared_ptr<ExclusiveAccess<HostPort>> host_port,
const InspectPublishUid& inspect_publish_uid)
: main_thread_(main_thread),
host_port_(host_port),
Expand Down Expand Up @@ -293,18 +293,26 @@ void InspectorIo::ThreadMain() {
std::unique_ptr<InspectorIoDelegate> delegate(
new InspectorIoDelegate(queue, main_thread_, id_,
script_path, script_name_));
std::string host;
int port;
{
ExclusiveAccess<HostPort>::Scoped host_port(host_port_);
host = host_port->host();
port = host_port->port();
}
InspectorSocketServer server(std::move(delegate),
&loop,
host_port_->host(),
host_port_->port(),
std::move(host),
port,
inspect_publish_uid_);
request_queue_ = queue->handle();
// Its lifetime is now that of the server delegate
queue.reset();
{
Mutex::ScopedLock scoped_lock(thread_start_lock_);
if (server.Start()) {
host_port_->set_port(server.Port());
ExclusiveAccess<HostPort>::Scoped host_port(host_port_);
host_port->set_port(server.Port());
}
thread_start_condition_.Broadcast(scoped_lock);
}
Expand All @@ -313,7 +321,8 @@ void InspectorIo::ThreadMain() {
}

std::string InspectorIo::GetWsUrl() const {
return FormatWsAddress(host_port_->host(), host_port_->port(), id_, true);
ExclusiveAccess<HostPort>::Scoped host_port(host_port_);
return FormatWsAddress(host_port->host(), host_port->port(), id_, true);
}

InspectorIoDelegate::InspectorIoDelegate(
Expand Down
6 changes: 3 additions & 3 deletions src/inspector_io.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class InspectorIo {
static std::unique_ptr<InspectorIo> Start(
std::shared_ptr<MainThreadHandle> main_thread,
const std::string& path,
std::shared_ptr<HostPort> host_port,
std::shared_ptr<ExclusiveAccess<HostPort>> host_port,
const InspectPublishUid& inspect_publish_uid);

// Will block till the transport thread shuts down
Expand All @@ -42,7 +42,7 @@ class InspectorIo {
private:
InspectorIo(std::shared_ptr<MainThreadHandle> handle,
const std::string& path,
std::shared_ptr<HostPort> host_port,
std::shared_ptr<ExclusiveAccess<HostPort>> host_port,
const InspectPublishUid& inspect_publish_uid);

// Wrapper for agent->ThreadMain()
Expand All @@ -57,7 +57,7 @@ class InspectorIo {
// Used to post on a frontend interface thread, lives while the server is
// running
std::shared_ptr<RequestQueue> request_queue_;
std::shared_ptr<HostPort> host_port_;
std::shared_ptr<ExclusiveAccess<HostPort>> host_port_;
InspectPublishUid inspect_publish_uid_;

// The IO thread runs its own uv_loop to implement the TCP server off
Expand Down
6 changes: 4 additions & 2 deletions src/inspector_js_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -278,12 +278,14 @@ void Open(const FunctionCallbackInfo<Value>& args) {

if (args.Length() > 0 && args[0]->IsUint32()) {
uint32_t port = args[0].As<Uint32>()->Value();
agent->host_port()->set_port(static_cast<int>(port));
ExclusiveAccess<HostPort>::Scoped host_port(agent->host_port());
host_port->set_port(static_cast<int>(port));
}

if (args.Length() > 1 && args[1]->IsString()) {
Utf8Value host(env->isolate(), args[1].As<String>());
agent->host_port()->set_host(*host);
ExclusiveAccess<HostPort>::Scoped host_port(agent->host_port());
host_port->set_host(*host);
}

agent->StartIoThread();
Expand Down
6 changes: 4 additions & 2 deletions src/node_process_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ static void ProcessTitleSetter(Local<Name> property,
static void DebugPortGetter(Local<Name> property,
const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
int port = env->inspector_host_port()->port();
ExclusiveAccess<HostPort>::Scoped host_port(env->inspector_host_port());
int port = host_port->port();
info.GetReturnValue().Set(port);
}

Expand All @@ -60,7 +61,8 @@ static void DebugPortSetter(Local<Name> property,
const PropertyCallbackInfo<void>& info) {
Environment* env = Environment::GetCurrent(info);
int32_t port = value->Int32Value(env->context()).FromMaybe(0);
env->inspector_host_port()->set_port(static_cast<int>(port));
ExclusiveAccess<HostPort>::Scoped host_port(env->inspector_host_port());
host_port->set_port(static_cast<int>(port));
}

static void GetParentProcessId(Local<Name> property,
Expand Down

0 comments on commit 1dec9d1

Please sign in to comment.