Skip to content

Commit

Permalink
src: use env->RequestInterrupt() for inspector MainThreadInterface
Browse files Browse the repository at this point in the history
This simplifies the code significantly, and removes the dependency of
the inspector code on the availability of a `MultiIsolatePlatform`
(by removing the dependency on a platform altogether).

It also fixes a memory leak that occurs when `RequestInterrupt()`
is used, but the interrupt handler is never called before the Isolate
is destroyed.

One downside is that this leads to a slight change in timing, because
inspector messages are not dispatched in a re-entrant way. This means
having to harden one test to account for that possibility by making
sure that the stack is always clear through a `setImmediate()`.
This does not affect the assertion made by the test, which is that
messages will not be delivered synchronously while other code is
executing.

nodejs#32415

PR-URL: nodejs#32523
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax committed Sep 23, 2020
1 parent a2ebe11 commit 83015b0
Show file tree
Hide file tree
Showing 7 changed files with 23 additions and 44 deletions.
5 changes: 3 additions & 2 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -1273,6 +1273,9 @@ class Environment : public MemoryRetainer {
inline void set_process_exit_handler(
std::function<void(Environment*, int)>&& handler);

void RunAndClearNativeImmediates(bool only_refed = false);
void RunAndClearInterrupts();

private:
inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
const char* errmsg);
Expand Down Expand Up @@ -1410,8 +1413,6 @@ class Environment : public MemoryRetainer {
// yet or already have been destroyed.
bool task_queues_async_initialized_ = false;

void RunAndClearNativeImmediates(bool only_refed = false);
void RunAndClearInterrupts();
std::atomic<Environment**> interrupt_data_ {nullptr};
void RequestInterruptFromV8();
static void CheckImmediate(uv_check_t* handle);
Expand Down
41 changes: 8 additions & 33 deletions src/inspector/main_thread_interface.cc
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "main_thread_interface.h"

#include "env-inl.h"
#include "node_mutex.h"
#include "v8-inspector.h"
#include "util-inl.h"
Expand Down Expand Up @@ -85,19 +86,6 @@ class CallRequest : public Request {
Fn fn_;
};

class DispatchMessagesTask : public v8::Task {
public:
explicit DispatchMessagesTask(std::weak_ptr<MainThreadInterface> thread)
: thread_(thread) {}

void Run() override {
if (auto thread = thread_.lock()) thread->DispatchMessages();
}

private:
std::weak_ptr<MainThreadInterface> thread_;
};

template <typename T>
class AnotherThreadObjectReference {
public:
Expand Down Expand Up @@ -212,36 +200,23 @@ class ThreadSafeDelegate : public InspectorSessionDelegate {
} // namespace


MainThreadInterface::MainThreadInterface(Agent* agent, uv_loop_t* loop,
v8::Isolate* isolate,
v8::Platform* platform)
: agent_(agent), isolate_(isolate),
platform_(platform) {
}
MainThreadInterface::MainThreadInterface(Agent* agent) : agent_(agent) {}

MainThreadInterface::~MainThreadInterface() {
if (handle_)
handle_->Reset();
}

void MainThreadInterface::Post(std::unique_ptr<Request> request) {
CHECK_NOT_NULL(agent_);
Mutex::ScopedLock scoped_lock(requests_lock_);
bool needs_notify = requests_.empty();
requests_.push_back(std::move(request));
if (needs_notify) {
if (isolate_ != nullptr && platform_ != nullptr) {
std::shared_ptr<v8::TaskRunner> taskrunner =
platform_->GetForegroundTaskRunner(isolate_);
std::weak_ptr<MainThreadInterface>* interface_ptr =
new std::weak_ptr<MainThreadInterface>(shared_from_this());
taskrunner->PostTask(
std::make_unique<DispatchMessagesTask>(*interface_ptr));
isolate_->RequestInterrupt([](v8::Isolate* isolate, void* opaque) {
std::unique_ptr<std::weak_ptr<MainThreadInterface>> interface_ptr {
static_cast<std::weak_ptr<MainThreadInterface>*>(opaque) };
if (auto iface = interface_ptr->lock()) iface->DispatchMessages();
}, static_cast<void*>(interface_ptr));
}
std::weak_ptr<MainThreadInterface> weak_self {shared_from_this()};
agent_->env()->RequestInterrupt([weak_self](Environment*) {
if (auto iface = weak_self.lock()) iface->DispatchMessages();
});
}
incoming_message_cond_.Broadcast(scoped_lock);
}
Expand Down Expand Up @@ -274,7 +249,7 @@ void MainThreadInterface::DispatchMessages() {
std::swap(dispatching_message_queue_.front(), task);
dispatching_message_queue_.pop_front();

v8::SealHandleScope seal_handle_scope(isolate_);
v8::SealHandleScope seal_handle_scope(agent_->env()->isolate());
task->Call(this);
}
} while (had_messages);
Expand Down
5 changes: 1 addition & 4 deletions src/inspector/main_thread_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ class MainThreadHandle : public std::enable_shared_from_this<MainThreadHandle> {
class MainThreadInterface :
public std::enable_shared_from_this<MainThreadInterface> {
public:
MainThreadInterface(Agent* agent, uv_loop_t*, v8::Isolate* isolate,
v8::Platform* platform);
explicit MainThreadInterface(Agent* agent);
~MainThreadInterface();

void DispatchMessages();
Expand All @@ -98,8 +97,6 @@ class MainThreadInterface :
ConditionVariable incoming_message_cond_;
// Used from any thread
Agent* const agent_;
v8::Isolate* const isolate_;
v8::Platform* const platform_;
std::shared_ptr<MainThreadHandle> handle_;
std::unordered_map<int, std::unique_ptr<Deletable>> managed_objects_;
};
Expand Down
6 changes: 2 additions & 4 deletions src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -660,8 +660,7 @@ class NodeInspectorClient : public V8InspectorClient {
std::shared_ptr<MainThreadHandle> getThreadHandle() {
if (!interface_) {
interface_ = std::make_shared<MainThreadInterface>(
env_->inspector_agent(), env_->event_loop(), env_->isolate(),
env_->isolate_data()->platform());
env_->inspector_agent());
}
return interface_->GetHandle();
}
Expand Down Expand Up @@ -697,10 +696,9 @@ class NodeInspectorClient : public V8InspectorClient {

running_nested_loop_ = true;

MultiIsolatePlatform* platform = env_->isolate_data()->platform();
while (shouldRunMessageLoop()) {
if (interface_) interface_->WaitForFrontendEvent();
while (platform->FlushForegroundTasks(env_->isolate())) {}
env_->RunAndClearInterrupts();
}
running_nested_loop_ = false;
}
Expand Down
2 changes: 2 additions & 0 deletions src/inspector_agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ class Agent {
// Interface for interacting with inspectors in worker threads
std::shared_ptr<WorkerManager> GetWorkerManager();

inline Environment* env() const { return parent_env_; }

private:
void ToggleAsyncHook(v8::Isolate* isolate,
const v8::Global<v8::Function>& fn);
Expand Down
2 changes: 1 addition & 1 deletion src/inspector_js_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class JSBindingsConnection : public AsyncWrap {

private:
Environment* env_;
JSBindingsConnection* connection_;
BaseObjectPtr<JSBindingsConnection> connection_;
};

JSBindingsConnection(Environment* env,
Expand Down
6 changes: 6 additions & 0 deletions test/parallel/test-inspector-connect-main-thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ function doConsoleLog(arrayBuffer) {
// and do not interrupt the main code. Interrupting the code flow
// can lead to unexpected behaviors.
async function ensureListenerDoesNotInterrupt(session) {
// Make sure that the following code is not affected by the fact that it may
// run inside an inspector message callback, during which other inspector
// message callbacks (such as the one triggered by doConsoleLog()) would
// not be processed.
await new Promise(setImmediate);

const currentTime = Date.now();
let consoleLogHappened = false;
session.once('Runtime.consoleAPICalled',
Expand Down

0 comments on commit 83015b0

Please sign in to comment.