Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 29620c4

Browse filesBrowse files
addaleaxaddaleax
committed
src: use env->RequestInterrupt() for inspector MainThreadInterface
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. #32415 Backport-PR-URL: #35241 PR-URL: #32523 Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 2e4536e commit 29620c4
Copy full SHA for 29620c4

File tree

Expand file treeCollapse file tree

7 files changed

+23
-44
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

7 files changed

+23
-44
lines changed
Open diff view settings
Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
+3-2Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1273,6 +1273,9 @@ class Environment : public MemoryRetainer {
12731273
inline void set_process_exit_handler(
12741274
std::function<void(Environment*, int)>&& handler);
12751275

1276+
void RunAndClearNativeImmediates(bool only_refed = false);
1277+
void RunAndClearInterrupts();
1278+
12761279
private:
12771280
inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
12781281
const char* errmsg);
@@ -1410,8 +1413,6 @@ class Environment : public MemoryRetainer {
14101413
// yet or already have been destroyed.
14111414
bool task_queues_async_initialized_ = false;
14121415

1413-
void RunAndClearNativeImmediates(bool only_refed = false);
1414-
void RunAndClearInterrupts();
14151416
std::atomic<Environment**> interrupt_data_ {nullptr};
14161417
void RequestInterruptFromV8();
14171418
static void CheckImmediate(uv_check_t* handle);
Collapse file

‎src/inspector/main_thread_interface.cc‎

Copy file name to clipboardExpand all lines: src/inspector/main_thread_interface.cc
+8-33Lines changed: 8 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#include "main_thread_interface.h"
22

3+
#include "env-inl.h"
34
#include "node_mutex.h"
45
#include "v8-inspector.h"
56
#include "util-inl.h"
@@ -85,19 +86,6 @@ class CallRequest : public Request {
8586
Fn fn_;
8687
};
8788

88-
class DispatchMessagesTask : public v8::Task {
89-
public:
90-
explicit DispatchMessagesTask(std::weak_ptr<MainThreadInterface> thread)
91-
: thread_(thread) {}
92-
93-
void Run() override {
94-
if (auto thread = thread_.lock()) thread->DispatchMessages();
95-
}
96-
97-
private:
98-
std::weak_ptr<MainThreadInterface> thread_;
99-
};
100-
10189
template <typename T>
10290
class AnotherThreadObjectReference {
10391
public:
@@ -212,36 +200,23 @@ class ThreadSafeDelegate : public InspectorSessionDelegate {
212200
} // namespace
213201

214202

215-
MainThreadInterface::MainThreadInterface(Agent* agent, uv_loop_t* loop,
216-
v8::Isolate* isolate,
217-
v8::Platform* platform)
218-
: agent_(agent), isolate_(isolate),
219-
platform_(platform) {
220-
}
203+
MainThreadInterface::MainThreadInterface(Agent* agent) : agent_(agent) {}
221204

222205
MainThreadInterface::~MainThreadInterface() {
223206
if (handle_)
224207
handle_->Reset();
225208
}
226209

227210
void MainThreadInterface::Post(std::unique_ptr<Request> request) {
211+
CHECK_NOT_NULL(agent_);
228212
Mutex::ScopedLock scoped_lock(requests_lock_);
229213
bool needs_notify = requests_.empty();
230214
requests_.push_back(std::move(request));
231215
if (needs_notify) {
232-
if (isolate_ != nullptr && platform_ != nullptr) {
233-
std::shared_ptr<v8::TaskRunner> taskrunner =
234-
platform_->GetForegroundTaskRunner(isolate_);
235-
std::weak_ptr<MainThreadInterface>* interface_ptr =
236-
new std::weak_ptr<MainThreadInterface>(shared_from_this());
237-
taskrunner->PostTask(
238-
std::make_unique<DispatchMessagesTask>(*interface_ptr));
239-
isolate_->RequestInterrupt([](v8::Isolate* isolate, void* opaque) {
240-
std::unique_ptr<std::weak_ptr<MainThreadInterface>> interface_ptr {
241-
static_cast<std::weak_ptr<MainThreadInterface>*>(opaque) };
242-
if (auto iface = interface_ptr->lock()) iface->DispatchMessages();
243-
}, static_cast<void*>(interface_ptr));
244-
}
216+
std::weak_ptr<MainThreadInterface> weak_self {shared_from_this()};
217+
agent_->env()->RequestInterrupt([weak_self](Environment*) {
218+
if (auto iface = weak_self.lock()) iface->DispatchMessages();
219+
});
245220
}
246221
incoming_message_cond_.Broadcast(scoped_lock);
247222
}
@@ -274,7 +249,7 @@ void MainThreadInterface::DispatchMessages() {
274249
std::swap(dispatching_message_queue_.front(), task);
275250
dispatching_message_queue_.pop_front();
276251

277-
v8::SealHandleScope seal_handle_scope(isolate_);
252+
v8::SealHandleScope seal_handle_scope(agent_->env()->isolate());
278253
task->Call(this);
279254
}
280255
} while (had_messages);
Collapse file

‎src/inspector/main_thread_interface.h‎

Copy file name to clipboardExpand all lines: src/inspector/main_thread_interface.h
+1-4Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,7 @@ class MainThreadHandle : public std::enable_shared_from_this<MainThreadHandle> {
7272
class MainThreadInterface :
7373
public std::enable_shared_from_this<MainThreadInterface> {
7474
public:
75-
MainThreadInterface(Agent* agent, uv_loop_t*, v8::Isolate* isolate,
76-
v8::Platform* platform);
75+
explicit MainThreadInterface(Agent* agent);
7776
~MainThreadInterface();
7877

7978
void DispatchMessages();
@@ -98,8 +97,6 @@ class MainThreadInterface :
9897
ConditionVariable incoming_message_cond_;
9998
// Used from any thread
10099
Agent* const agent_;
101-
v8::Isolate* const isolate_;
102-
v8::Platform* const platform_;
103100
std::shared_ptr<MainThreadHandle> handle_;
104101
std::unordered_map<int, std::unique_ptr<Deletable>> managed_objects_;
105102
};
Collapse file

‎src/inspector_agent.cc‎

Copy file name to clipboardExpand all lines: src/inspector_agent.cc
+2-4Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -660,8 +660,7 @@ class NodeInspectorClient : public V8InspectorClient {
660660
std::shared_ptr<MainThreadHandle> getThreadHandle() {
661661
if (!interface_) {
662662
interface_ = std::make_shared<MainThreadInterface>(
663-
env_->inspector_agent(), env_->event_loop(), env_->isolate(),
664-
env_->isolate_data()->platform());
663+
env_->inspector_agent());
665664
}
666665
return interface_->GetHandle();
667666
}
@@ -697,10 +696,9 @@ class NodeInspectorClient : public V8InspectorClient {
697696

698697
running_nested_loop_ = true;
699698

700-
MultiIsolatePlatform* platform = env_->isolate_data()->platform();
701699
while (shouldRunMessageLoop()) {
702700
if (interface_) interface_->WaitForFrontendEvent();
703-
while (platform->FlushForegroundTasks(env_->isolate())) {}
701+
env_->RunAndClearInterrupts();
704702
}
705703
running_nested_loop_ = false;
706704
}
Collapse file

‎src/inspector_agent.h‎

Copy file name to clipboardExpand all lines: src/inspector_agent.h
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,8 @@ class Agent {
116116
// Interface for interacting with inspectors in worker threads
117117
std::shared_ptr<WorkerManager> GetWorkerManager();
118118

119+
inline Environment* env() const { return parent_env_; }
120+
119121
private:
120122
void ToggleAsyncHook(v8::Isolate* isolate,
121123
const v8::Global<v8::Function>& fn);
Collapse file

‎src/inspector_js_api.cc‎

Copy file name to clipboardExpand all lines: src/inspector_js_api.cc
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ class JSBindingsConnection : public AsyncWrap {
8383

8484
private:
8585
Environment* env_;
86-
JSBindingsConnection* connection_;
86+
BaseObjectPtr<JSBindingsConnection> connection_;
8787
};
8888

8989
JSBindingsConnection(Environment* env,
Collapse file

‎test/parallel/test-inspector-connect-main-thread.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-inspector-connect-main-thread.js
+6Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,12 @@ function doConsoleLog(arrayBuffer) {
7676
// and do not interrupt the main code. Interrupting the code flow
7777
// can lead to unexpected behaviors.
7878
async function ensureListenerDoesNotInterrupt(session) {
79+
// Make sure that the following code is not affected by the fact that it may
80+
// run inside an inspector message callback, during which other inspector
81+
// message callbacks (such as the one triggered by doConsoleLog()) would
82+
// not be processed.
83+
await new Promise(setImmediate);
84+
7985
const currentTime = Date.now();
8086
let consoleLogHappened = false;
8187
session.once('Runtime.consoleAPICalled',

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.