Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UNREACHABLE in WorkerThreadsTaskRunner::PostDelayedTask #22157

Closed
devsnek opened this issue Aug 6, 2018 · 5 comments
Closed

UNREACHABLE in WorkerThreadsTaskRunner::PostDelayedTask #22157

devsnek opened this issue Aug 6, 2018 · 5 comments
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. inspector Issues and PRs related to the V8 inspector protocol v8 engine Issues and PRs related to the V8 dependency.

Comments

@devsnek
Copy link
Member

devsnek commented Aug 6, 2018

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 devsnek added the inspector Issues and PRs related to the V8 inspector protocol label Aug 6, 2018
@devsnek devsnek changed the title UNREACHABLE in NodePlatform::CallDelayedOnWorkerThread UNREACHABLE in WorkerThreadsTaskRunner::PostDelayedTask Aug 6, 2018
@devsnek
Copy link
Member Author

devsnek commented Aug 6, 2018

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
Copy link
Member

addaleax commented Aug 6, 2018

@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
Copy link
Member Author

devsnek commented Aug 6, 2018

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

@addaleax addaleax added the v8 engine Issues and PRs related to the V8 dependency. label Aug 6, 2018
@addaleax
Copy link
Member

addaleax commented Aug 6, 2018

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

@devsnek devsnek added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Aug 6, 2018
@alexkozy
Copy link
Member

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.

addaleax pushed a commit that referenced this issue Aug 28, 2018
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

Backport-PR-URL: #22567
PR-URL: #22383
Fixes: #22157
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this issue Sep 3, 2018
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

Backport-PR-URL: #22567
PR-URL: #22383
Fixes: #22157
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this issue Sep 6, 2018
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

Backport-PR-URL: #22567
PR-URL: #22383
Fixes: #22157
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
blattersturm pushed a commit to citizenfx/node that referenced this issue Nov 3, 2018
This method is crucial for Runtime.evaluate protocol command with
timeout flag. At least Chrome DevTools frontend uses this method for
every execution in console.

PR-URL: nodejs#22383
Fixes: nodejs#22157
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. inspector Issues and PRs related to the V8 inspector protocol v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants