nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.33k stars 29.47k forks source link

UNREACHABLE in WorkerThreadsTaskRunner::PostDelayedTask #22157

Closed devsnek closed 6 years ago

devsnek commented 6 years ago

v8_inspector::V8InspectorImpl::EvaluateScope::setTimeout calls NodePlatform::CallDelayedOnWorkerThread which calls WorkerThreadsTaskRunner::PostDelayedTask which has UNREACHABLE().

any calls to Runtime.evaluate with a timeout will therefore abort the process...

/cc @addaleax @TimothyGu

devsnek commented 6 years ago

would this diff work? its sorta copied from the foreground runner's PostDelayedTask

diff --git a/src/node_platform.cc b/src/node_platform.cc
index 6a3ae2e5dc..c41e6624e8 100644
--- a/src/node_platform.cc
+++ b/src/node_platform.cc
@@ -46,7 +46,15 @@ void WorkerThreadsTaskRunner::PostTask(std::unique_ptr<Task> task) {

 void WorkerThreadsTaskRunner::PostDelayedTask(std::unique_ptr<v8::Task> task,
                                               double delay_in_seconds) {
-  UNREACHABLE();
+  uv_timer_t timer;
+  timer.data = std::move(task).get();
+  uint64_t delay_millis = static_cast<uint64_t>(delay_in_seconds + 0.5) * 1000;
+  uv_timer_init(uv_default_loop(), &timer);
+  uv_timer_start(&timer, [](uv_timer_t* timer) {
+    auto task = reinterpret_cast<v8::Task*>(timer->data);
+    task->Run();
+  }, delay_millis, 0);
+  uv_unref(reinterpret_cast<uv_handle_t*>(&timer));
 }

 void WorkerThreadsTaskRunner::BlockingDrain() {
addaleax commented 6 years ago

@devsnek Well … accessing the default loop is not thread-safe (and that loop does not run in the background). So, for that general approach to work, I think we’d need to have a separate loop/thread for the delayed tasks?

devsnek commented 6 years ago

@addaleax i think i'll just leave this to someone who knows more about it :P

addaleax commented 6 years ago

I think I’m working on enough things for now, but if somebody wants to take this and feels up for it, I’m happy to support as needed

alexkozy commented 6 years ago

It looks like this crash is reproducible in Node 10.9.0 and it breaks DevTools console in dedicated frontend and in ndb. I am wondering what is right label to mark issues like this next time to make it release blocker? I will work on fix tomorrow.