Skip to content

Commit

Permalink
inspector: split the HostPort being used and the one parsed from CLI
Browse files Browse the repository at this point in the history
Instead of using a shared pointer of the entire debug option set,
pass the parsed debug option to inspector classes by value because
they are set once the CLI argument parsing is done. Add another shared
pointer to HostPort being used by the inspector server, which is copied
from the one in the debug options initially. The port of the shared
HostPort is 9229 by default and can be specified as 0 initially but
will be set to the actual port of the server once it starts listening.

This makes the shared state clearer and makes it possible to use
`require('internal/options')` in JS land to query the CLI options
instead of using `process._breakFirstLine` and other underscored
properties of `process` since we are now certain that these
values should not be altered once the parsing is done and can be
passed around in copies without locks.

PR-URL: #24772
Reviewed-By: Anna Henningsen <[email protected]>
  • Loading branch information
joyeecheung authored and BethGriggs committed Dec 17, 2018
1 parent 81dce68 commit 56b2a72
Show file tree
Hide file tree
Showing 15 changed files with 160 additions and 98 deletions.
4 changes: 4 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,10 @@ inline std::shared_ptr<EnvironmentOptions> Environment::options() {
return options_;
}

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

inline std::shared_ptr<PerIsolateOptions> IsolateData::options() {
return options_;
}
Expand Down
3 changes: 2 additions & 1 deletion src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "node_file.h"
#include "node_internals.h"
#include "node_native_module.h"
#include "node_options-inl.h"
#include "node_platform.h"
#include "node_worker.h"
#include "tracing/agent.h"
Expand Down Expand Up @@ -192,7 +193,7 @@ 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));
options_->debug_options.reset(new DebugOptions(*options_->debug_options));
inspector_host_port_.reset(new HostPort(options_->debug_options().host_port));

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

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

private:
inline void CreateImmediate(native_immediate_callback cb,
Expand Down Expand Up @@ -942,6 +943,14 @@ class Environment {
std::vector<double> destroy_async_id_list_;

std::shared_ptr<EnvironmentOptions> options_;
// options_ contains debug options parsed from CLI arguments,
// while inspector_host_port_ stores the actual inspector host
// and port being used. For example the port is -1 by default
// and can be specified as 0 (meaning any port allocated when the
// 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_;

uint32_t module_id_counter_ = 0;
uint32_t script_id_counter_ = 0;
Expand Down
29 changes: 17 additions & 12 deletions src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -669,8 +669,9 @@ class NodeInspectorClient : public V8InspectorClient {
};

Agent::Agent(Environment* env)
: parent_env_(env),
debug_options_(env->options()->debug_options) {}
: parent_env_(env),
debug_options_(env->options()->debug_options()),
host_port_(env->inspector_host_port()) {}

Agent::~Agent() {
if (start_io_thread_async.data == this) {
Expand All @@ -681,13 +682,14 @@ Agent::~Agent() {
}

bool Agent::Start(const std::string& path,
std::shared_ptr<DebugOptions> options,
const DebugOptions& options,
std::shared_ptr<HostPort> host_port,
bool is_main) {
if (options == nullptr) {
options = std::make_shared<DebugOptions>();
}
path_ = path;
debug_options_ = options;
CHECK_NE(host_port, nullptr);
host_port_ = host_port;

client_ = std::make_shared<NodeInspectorClient>(parent_env_, is_main);
if (parent_env_->is_main_thread()) {
CHECK_EQ(0, uv_async_init(parent_env_->event_loop(),
Expand All @@ -699,13 +701,18 @@ bool Agent::Start(const std::string& path,
StartDebugSignalHandler();
}

bool wait_for_connect = options->wait_for_connect();
bool wait_for_connect = options.wait_for_connect();
if (parent_handle_) {
wait_for_connect = parent_handle_->WaitForConnect();
parent_handle_->WorkerStarted(client_->getThreadHandle(), wait_for_connect);
} else if (!options->inspector_enabled || !StartIoThread()) {
} else if (!options.inspector_enabled || !StartIoThread()) {
return false;
}

// TODO(joyeecheung): we should not be using process as a global object
// to transport --inspect-brk. Instead, the JS land can get this through
// require('internal/options') since it should be set once CLI parsing
// is done.
if (wait_for_connect) {
HandleScope scope(parent_env_->isolate());
parent_env_->process_object()->DefineOwnProperty(
Expand All @@ -725,8 +732,7 @@ bool Agent::StartIoThread() {

CHECK_NOT_NULL(client_);

io_ = InspectorIo::Start(
client_->getThreadHandle(), path_, debug_options_);
io_ = InspectorIo::Start(client_->getThreadHandle(), path_, host_port_);
if (io_ == nullptr) {
return false;
}
Expand Down Expand Up @@ -867,8 +873,7 @@ void Agent::ContextCreated(Local<Context> context, const ContextInfo& info) {
}

bool Agent::WillWaitForConnect() {
if (debug_options_->wait_for_connect())
return true;
if (debug_options_.wait_for_connect()) return true;
if (parent_handle_)
return parent_handle_->WaitForConnect();
return false;
Expand Down
18 changes: 13 additions & 5 deletions src/inspector_agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#error("This header can only be used when inspector is enabled")
#endif

#include "node_options.h"
#include "node_options-inl.h"
#include "node_persistent.h"
#include "v8.h"

Expand Down Expand Up @@ -50,8 +50,9 @@ class Agent {

// Create client_, may create io_ if option enabled
bool Start(const std::string& path,
std::shared_ptr<DebugOptions> options,
bool is_worker);
const DebugOptions& options,
std::shared_ptr<HostPort> host_port,
bool is_main);
// Stop and destroy io_
void Stop();

Expand Down Expand Up @@ -104,7 +105,8 @@ class Agent {
// Calls StartIoThread() from off the main thread.
void RequestIoThreadStart();

std::shared_ptr<DebugOptions> options() { return debug_options_; }
const DebugOptions& options() { return debug_options_; }
std::shared_ptr<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 @@ -121,7 +123,13 @@ class Agent {
std::unique_ptr<InspectorIo> io_;
std::unique_ptr<ParentInspectorHandle> parent_handle_;
std::string path_;
std::shared_ptr<DebugOptions> debug_options_;

// This is a copy of the debug options parsed from CLI in the Environment.
// Do not use the host_port in that, instead manipulate the shared host_port_
// pointer which is meant to store the actual host and port of the inspector
// server.
DebugOptions debug_options_;
std::shared_ptr<HostPort> host_port_;

bool pending_enable_async_hook_ = false;
bool pending_disable_async_hook_ = false;
Expand Down
22 changes: 13 additions & 9 deletions src/inspector_io.cc
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,9 @@ 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<DebugOptions> options) {
std::shared_ptr<HostPort> host_port) {
auto io = std::unique_ptr<InspectorIo>(
new InspectorIo(main_thread, path, options));
new InspectorIo(main_thread, path, host_port));
if (io->request_queue_->Expired()) { // Thread is not running
return nullptr;
}
Expand All @@ -253,9 +253,12 @@ std::unique_ptr<InspectorIo> InspectorIo::Start(

InspectorIo::InspectorIo(std::shared_ptr<MainThreadHandle> main_thread,
const std::string& path,
std::shared_ptr<DebugOptions> options)
: main_thread_(main_thread), options_(options),
thread_(), script_name_(path), id_(GenerateID()) {
std::shared_ptr<HostPort> host_port)
: main_thread_(main_thread),
host_port_(host_port),
thread_(),
script_name_(path),
id_(GenerateID()) {
Mutex::ScopedLock scoped_lock(thread_start_lock_);
CHECK_EQ(uv_thread_create(&thread_, InspectorIo::ThreadMain, this), 0);
thread_start_condition_.Wait(scoped_lock);
Expand Down Expand Up @@ -287,16 +290,17 @@ void InspectorIo::ThreadMain() {
std::unique_ptr<InspectorIoDelegate> delegate(
new InspectorIoDelegate(queue, main_thread_, id_,
script_path, script_name_));
InspectorSocketServer server(std::move(delegate), &loop,
options_->host().c_str(),
options_->port());
InspectorSocketServer server(std::move(delegate),
&loop,
host_port_->host().c_str(),
host_port_->port());
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()) {
port_ = server.Port();
host_port_->set_port(server.Port());
}
thread_start_condition_.Broadcast(scoped_lock);
}
Expand Down
14 changes: 7 additions & 7 deletions src/inspector_io.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,22 @@ class InspectorIo {
// bool Start();
// Returns empty pointer if thread was not started
static std::unique_ptr<InspectorIo> Start(
std::shared_ptr<MainThreadHandle> main_thread, const std::string& path,
std::shared_ptr<DebugOptions> options);
std::shared_ptr<MainThreadHandle> main_thread,
const std::string& path,
std::shared_ptr<HostPort> host_port);

// Will block till the transport thread shuts down
~InspectorIo();

void StopAcceptingNewConnections();
const std::string& host() const { return options_->host(); }
int port() const { return port_; }
const std::string& host() const { return host_port_->host(); }
int port() const { return host_port_->port(); }
std::vector<std::string> GetTargetIds() const;

private:
InspectorIo(std::shared_ptr<MainThreadHandle> handle,
const std::string& path,
std::shared_ptr<DebugOptions> options);
std::shared_ptr<HostPort> host_port);

// Wrapper for agent->ThreadMain()
static void ThreadMain(void* agent);
Expand All @@ -74,7 +75,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<DebugOptions> options_;
std::shared_ptr<HostPort> host_port_;

// The IO thread runs its own uv_loop to implement the TCP server off
// the main thread.
Expand All @@ -84,7 +85,6 @@ class InspectorIo {
Mutex thread_start_lock_;
ConditionVariable thread_start_condition_;
std::string script_name_;
int port_ = -1;
// May be accessed from any thread
const std::string id_;
};
Expand Down
4 changes: 2 additions & 2 deletions src/inspector_js_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -243,12 +243,12 @@ void Open(const FunctionCallbackInfo<Value>& args) {

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

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

if (args.Length() > 2 && args[2]->IsBoolean()) {
Expand Down
32 changes: 16 additions & 16 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include "node_internals.h"
#include "node_metadata.h"
#include "node_native_module.h"
#include "node_options-inl.h"
#include "node_perf.h"
#include "node_platform.h"
#include "node_revert.h"
Expand Down Expand Up @@ -259,13 +260,15 @@ static struct {
}

#if HAVE_INSPECTOR
bool StartInspector(Environment* env, const char* script_path,
std::shared_ptr<DebugOptions> options) {
bool StartInspector(Environment* env, const char* script_path) {
// Inspector agent can't fail to start, but if it was configured to listen
// right away on the websocket port and fails to bind/etc, this will return
// false.
return env->inspector_agent()->Start(
script_path == nullptr ? "" : script_path, options, true);
script_path == nullptr ? "" : script_path,
env->options()->debug_options(),
env->inspector_host_port(),
true);
}

bool InspectorStarted(Environment* env) {
Expand Down Expand Up @@ -306,8 +309,7 @@ static struct {
void Dispose() {}
void DrainVMTasks(Isolate* isolate) {}
void CancelVMTasks(Isolate* isolate) {}
bool StartInspector(Environment* env, const char* script_path,
const DebugOptions& options) {
bool StartInspector(Environment* env, const char* script_path) {
env->ThrowError("Node compiled with NODE_USE_V8_PLATFORM=0");
return true;
}
Expand Down Expand Up @@ -1127,24 +1129,24 @@ void SetupProcessObject(Environment* env,

// TODO(refack): move the following 4 to `node_config`
// --inspect-brk
if (env->options()->debug_options->wait_for_connect()) {
if (env->options()->debug_options().wait_for_connect()) {
READONLY_DONT_ENUM_PROPERTY(process,
"_breakFirstLine", True(env->isolate()));
}

if (env->options()->debug_options->break_node_first_line) {
if (env->options()->debug_options().break_node_first_line) {
READONLY_DONT_ENUM_PROPERTY(process,
"_breakNodeFirstLine", True(env->isolate()));
}

// --inspect --debug-brk
if (env->options()->debug_options->deprecated_invocation()) {
if (env->options()->debug_options().deprecated_invocation()) {
READONLY_DONT_ENUM_PROPERTY(process,
"_deprecatedDebugBrk", True(env->isolate()));
}

// --debug or, --debug-brk without --inspect
if (env->options()->debug_options->invalid_invocation()) {
if (env->options()->debug_options().invalid_invocation()) {
READONLY_DONT_ENUM_PROPERTY(process,
"_invalidDebug", True(env->isolate()));
}
Expand Down Expand Up @@ -1356,7 +1358,7 @@ void LoadEnvironment(Environment* env) {
get_linked_binding_fn,
get_internal_binding_fn,
Boolean::New(env->isolate(),
env->options()->debug_options->break_node_first_line)
env->options()->debug_options().break_node_first_line)
};

// Bootstrap internal loaders
Expand Down Expand Up @@ -1390,12 +1392,10 @@ void LoadEnvironment(Environment* env) {
}
}


static void StartInspector(Environment* env, const char* path,
std::shared_ptr<DebugOptions> debug_options) {
static void StartInspector(Environment* env, const char* path) {
#if HAVE_INSPECTOR
CHECK(!env->inspector_agent()->IsListening());
v8_platform.StartInspector(env, path, debug_options);
v8_platform.StartInspector(env, path);
#endif // HAVE_INSPECTOR
}

Expand Down Expand Up @@ -2038,9 +2038,9 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
env.Start(args, exec_args, v8_is_profiling);

const char* path = args.size() > 1 ? args[1].c_str() : nullptr;
StartInspector(&env, path, env.options()->debug_options);
StartInspector(&env, path);

if (env.options()->debug_options->inspector_enabled &&
if (env.options()->debug_options().inspector_enabled &&
!v8_platform.InspectorStarted(&env)) {
return 12; // Signal internal error.
}
Expand Down
Loading

0 comments on commit 56b2a72

Please sign in to comment.