Skip to content

Commit

Permalink
src: make WaitForInspectorDisconnect an exit hook
Browse files Browse the repository at this point in the history
Run inspector cleanup code on Environment teardown.

This is part of a series of changes to make embedding easier, by
requiring fewer internal methods to build a fully functioning
Node.js instance.

PR-URL: #30229
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
  • Loading branch information
addaleax authored and targos committed Dec 1, 2019
1 parent 619b718 commit 79713ed
Show file tree
Hide file tree
Showing 6 changed files with 26 additions and 47 deletions.
7 changes: 7 additions & 0 deletions src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,13 @@ bool Agent::Start(const std::string& path,
StartDebugSignalHandler();
}

AtExit(parent_env_, [](void* env) {
Agent* agent = static_cast<Environment*>(env)->inspector_agent();
if (agent->IsActive()) {
agent->WaitForDisconnect();
}
}, parent_env_);

bool wait_for_connect = options.wait_for_connect();
if (parent_handle_) {
wait_for_connect = parent_handle_->WaitForConnect();
Expand Down
26 changes: 1 addition & 25 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -151,31 +151,6 @@ bool v8_is_profiling = false;
struct V8Platform v8_platform;
} // namespace per_process

#ifdef __POSIX__
static const unsigned kMaxSignal = 32;
#endif

void WaitForInspectorDisconnect(Environment* env) {
#if HAVE_INSPECTOR

if (env->inspector_agent()->IsActive()) {
// Restore signal dispositions, the app is done and is no longer
// capable of handling signals.
#if defined(__POSIX__) && !defined(NODE_SHARED_MODE)
struct sigaction act;
memset(&act, 0, sizeof(act));
for (unsigned nr = 1; nr < kMaxSignal; nr += 1) {
if (nr == SIGKILL || nr == SIGSTOP || nr == SIGPROF)
continue;
act.sa_handler = (nr == SIGPIPE) ? SIG_IGN : SIG_DFL;
CHECK_EQ(0, sigaction(nr, &act, nullptr));
}
#endif
env->inspector_agent()->WaitForDisconnect();
}
#endif
}

void SignalExit(int signo) {
ResetStdio();
#ifdef __FreeBSD__
Expand Down Expand Up @@ -522,6 +497,7 @@ inline void PlatformInit() {
CHECK_EQ(err, 0);
#endif // HAVE_INSPECTOR

// TODO(addaleax): NODE_SHARED_MODE does not really make sense here.
#ifndef NODE_SHARED_MODE
// Restore signal dispositions, the parent process may have changed them.
struct sigaction act;
Expand Down
5 changes: 4 additions & 1 deletion src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ void PrintCaughtException(v8::Isolate* isolate,
v8::Local<v8::Context> context,
const v8::TryCatch& try_catch);

void WaitForInspectorDisconnect(Environment* env);
void ResetStdio(); // Safe to call more than once and from signal handlers.
void SignalExit(int signo);
#ifdef __POSIX__
Expand Down Expand Up @@ -313,6 +312,10 @@ void StartProfilers(Environment* env);
}
#endif // HAVE_INSPECTOR

#ifdef __POSIX__
static constexpr unsigned kMaxSignal = 32;
#endif

bool HasSignalJSHandler(int signum);

#ifdef _WIN32
Expand Down
15 changes: 14 additions & 1 deletion src/node_main_instance.cc
Original file line number Diff line number Diff line change
Expand Up @@ -145,13 +145,26 @@ int NodeMainInstance::Run() {

env->set_trace_sync_io(false);
exit_code = EmitExit(env.get());
WaitForInspectorDisconnect(env.get());
}

env->set_can_call_into_js(false);
env->stop_sub_worker_contexts();
ResetStdio();
env->RunCleanup();

// TODO(addaleax): Neither NODE_SHARED_MODE nor HAVE_INSPECTOR really
// make sense here.
#if HAVE_INSPECTOR && defined(__POSIX__) && !defined(NODE_SHARED_MODE)
struct sigaction act;
memset(&act, 0, sizeof(act));
for (unsigned nr = 1; nr < kMaxSignal; nr += 1) {
if (nr == SIGKILL || nr == SIGSTOP || nr == SIGPROF)
continue;
act.sa_handler = (nr == SIGPIPE) ? SIG_IGN : SIG_DFL;
CHECK_EQ(0, sigaction(nr, &act, nullptr));
}
#endif

RunAtExit(env.get());

per_process::v8_platform.DrainVMTasks(isolate_);
Expand Down
1 change: 0 additions & 1 deletion src/node_process_methods.cc
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,6 @@ static void DebugEnd(const FunctionCallbackInfo<Value>& args) {
static void ReallyExit(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
RunAtExit(env);
WaitForInspectorDisconnect(env);
int code = args[0]->Int32Value(env->context()).FromMaybe(0);
env->Exit(code);
}
Expand Down
19 changes: 0 additions & 19 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,6 @@ using v8::Value;
namespace node {
namespace worker {

namespace {

#if HAVE_INSPECTOR
void WaitForWorkerInspectorToStop(Environment* child) {
child->inspector_agent()->WaitForDisconnect();
child->inspector_agent()->Stop();
}
#endif

} // anonymous namespace

Worker::Worker(Environment* env,
Local<Object> wrap,
const std::string& url,
Expand Down Expand Up @@ -191,9 +180,6 @@ void Worker::Run() {
Locker locker(isolate_);
Isolate::Scope isolate_scope(isolate_);
SealHandleScope outer_seal(isolate_);
#if HAVE_INSPECTOR
bool inspector_started = false;
#endif

DeleteFnPtr<Environment, FreeEnvironment> env_;
OnScopeLeave cleanup_env([&]() {
Expand Down Expand Up @@ -223,10 +209,6 @@ void Worker::Run() {
env_->stop_sub_worker_contexts();
env_->RunCleanup();
RunAtExit(env_.get());
#if HAVE_INSPECTOR
if (inspector_started)
WaitForWorkerInspectorToStop(env_.get());
#endif

// This call needs to be made while the `Environment` is still alive
// because we assume that it is available for async tracking in the
Expand Down Expand Up @@ -270,7 +252,6 @@ void Worker::Run() {
env_->InitializeDiagnostics();
#if HAVE_INSPECTOR
env_->InitializeInspector(inspector_parent_handle_.release());
inspector_started = true;
#endif
HandleScope handle_scope(isolate_);
AsyncCallbackScope callback_scope(env_.get());
Expand Down

0 comments on commit 79713ed

Please sign in to comment.