Skip to content

Commit

Permalink
src: implement v8::Platform::CallDelayedOnWorkerThread
Browse files Browse the repository at this point in the history
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: #22383
Fixes: #22157
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
alexkozy committed Aug 21, 2018
1 parent f1d3f97 commit b1e2612
Show file tree
Hide file tree
Showing 3 changed files with 148 additions and 1 deletion.
124 changes: 123 additions & 1 deletion src/node_platform.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include "node_internals.h"

#include "env-inl.h"
#include "debug_utils.h"
#include "util.h"
#include <algorithm>

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 21, 2018

Member

May be a silly question but this PR says implement v8::Platform::CallDelayedOnWorkerThread but I don't see CallDelayedOnWorkerThread mentioned in the code changes.

Is this PR implementing something equivalent to CallDelayedOnWorkerThread or did it get renamed?

This comment has been minimized.

Copy link
@devsnek

devsnek Aug 21, 2018

Member

CallDelayedOnWorkerThread calls PostDelayedTask. technically this implements PostDelayedTask but the actual API is CallDelayedOnWorkerThread.

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 21, 2018

Member

Is this recreating PostDelayedTask from elsewhere in V8 or is PostDelayedTask an interface expected by V8?

This comment has been minimized.

Copy link
@alexkozy

alexkozy Aug 21, 2018

Author Member

NodePlatform::CallDelayedOnWorkerThread calls WorkerThreadsTaskRunner::PostDelayedTask in node_platform.cc file. I implemented WorkerThreadsTaskRunner::PostDelayedTask, WorkerThreadsTaskRunner looks like a good place for this method to me.

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 21, 2018

Member

Thanks @ak239!

I'm trying to wrap my head around this. RE: The comment here:

On Inspector side we call postTask on current platform, most likely current platform is incorrect.

Is this something that's implemented in node_platform-only or something the devtools/inspect expect from whatever platform they're interacting with?

This comment has been minimized.

Copy link
@devsnek

devsnek Aug 21, 2018

Member

v8 inspector internals expect CallDelayedOnWorkerThread

This comment has been minimized.

Copy link
@alexkozy

alexkozy Aug 21, 2018

Author Member

Runtime.evaluate supports timeout flag. When it is passed, inspector tries to kill current execution after timeout, so we expect CallDelayedOnWorkerThread to be implemented. We need to schedule this timer on worker thread to be sure that it is not blocked by current JavaScript execution.

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 21, 2018

Member

Ah, thank you @ak239 @devsnek for the details!

@ak239 This means there's another CallDelayedOnWorkerThread implemented by V8 somewhere for Chromium? Is it possible to share the implementation of CallDelayedOnWorkerThread between the two without having to create a one-off in node_platform?

This comment has been minimized.

Copy link
@alexkozy

alexkozy Aug 21, 2018

Author Member

Unfortunately it is not possible to share. V8 does not have implementation by itself, CallDelayedOnWorkerThread is implemented on Chromium side, most likely it depends on a lot of Chromium internal stuff that we do not want to bring to Node.js.
(for reference, Chromium implementation of posting tasks is here).

Expand Down Expand Up @@ -29,7 +30,127 @@ static void PlatformWorkerThread(void* data) {

} // namespace

class WorkerThreadsTaskRunner::DelayedTaskScheduler {
public:
explicit DelayedTaskScheduler(TaskQueue<Task>* tasks)
: pending_worker_tasks_(tasks) {}

std::unique_ptr<uv_thread_t> Start() {
auto start_thread = [](void* data) {
static_cast<DelayedTaskScheduler*>(data)->Run();
};
std::unique_ptr<uv_thread_t> t { new uv_thread_t() };
uv_sem_init(&ready_, 0);
CHECK_EQ(0, uv_thread_create(t.get(), start_thread, this));
uv_sem_wait(&ready_);
uv_sem_destroy(&ready_);
return t;
}

void PostDelayedTask(std::unique_ptr<Task> task, double delay_in_seconds) {
tasks_.Push(std::unique_ptr<Task>(new ScheduleTask(this, std::move(task),
delay_in_seconds)));
uv_async_send(&flush_tasks_);
}

void Stop() {
tasks_.Push(std::unique_ptr<Task>(new StopTask(this)));
uv_async_send(&flush_tasks_);
}

private:
void Run() {
TRACE_EVENT_METADATA1("__metadata", "thread_name", "name",
"WorkerThreadsTaskRunner::DelayedTaskScheduler");
loop_.data = this;
CHECK_EQ(0, uv_loop_init(&loop_));
flush_tasks_.data = this;
CHECK_EQ(0, uv_async_init(&loop_, &flush_tasks_, FlushTasks));
uv_sem_post(&ready_);

uv_run(&loop_, UV_RUN_DEFAULT);
CheckedUvLoopClose(&loop_);
}

static void FlushTasks(uv_async_t* flush_tasks) {
DelayedTaskScheduler* scheduler =
ContainerOf(&DelayedTaskScheduler::loop_, flush_tasks->loop);
while (std::unique_ptr<Task> task = scheduler->tasks_.Pop())
task->Run();
}

class StopTask : public Task {
public:
explicit StopTask(DelayedTaskScheduler* scheduler): scheduler_(scheduler) {}

void Run() override {
std::vector<uv_timer_t*> timers;
for (uv_timer_t* timer : scheduler_->timers_)
timers.push_back(timer);
for (uv_timer_t* timer : timers)
scheduler_->TakeTimerTask(timer);
uv_close(reinterpret_cast<uv_handle_t*>(&scheduler_->flush_tasks_),
[](uv_handle_t* handle) {});
}

private:
DelayedTaskScheduler* scheduler_;
};

class ScheduleTask : public Task {
public:
ScheduleTask(DelayedTaskScheduler* scheduler,
std::unique_ptr<Task> task,
double delay_in_seconds)
: scheduler_(scheduler),
task_(std::move(task)),
delay_in_seconds_(delay_in_seconds) {}

void Run() override {
uint64_t delay_millis =
static_cast<uint64_t>(delay_in_seconds_ + 0.5) * 1000;
std::unique_ptr<uv_timer_t> timer(new uv_timer_t());
CHECK_EQ(0, uv_timer_init(&scheduler_->loop_, timer.get()));
timer->data = task_.release();
CHECK_EQ(0, uv_timer_start(timer.get(), RunTask, delay_millis, 0));
scheduler_->timers_.insert(timer.release());
}

private:
DelayedTaskScheduler* scheduler_;
std::unique_ptr<Task> task_;
double delay_in_seconds_;
};

static void RunTask(uv_timer_t* timer) {
DelayedTaskScheduler* scheduler =
ContainerOf(&DelayedTaskScheduler::loop_, timer->loop);
scheduler->pending_worker_tasks_->Push(scheduler->TakeTimerTask(timer));
}

std::unique_ptr<Task> TakeTimerTask(uv_timer_t* timer) {
std::unique_ptr<Task> task(static_cast<Task*>(timer->data));
uv_timer_stop(timer);
uv_close(reinterpret_cast<uv_handle_t*>(timer), [](uv_handle_t* handle) {
delete reinterpret_cast<uv_timer_t*>(handle);
});
timers_.erase(timer);
return task;
}

uv_sem_t ready_;
TaskQueue<v8::Task>* pending_worker_tasks_;

TaskQueue<v8::Task> tasks_;
uv_loop_t loop_;
uv_async_t flush_tasks_;
std::unordered_set<uv_timer_t*> timers_;
};

WorkerThreadsTaskRunner::WorkerThreadsTaskRunner(int thread_pool_size) {
delayed_task_scheduler_.reset(
new DelayedTaskScheduler(&pending_worker_tasks_));
threads_.push_back(delayed_task_scheduler_->Start());
for (int i = 0; i < thread_pool_size; i++) {
std::unique_ptr<uv_thread_t> t { new uv_thread_t() };
if (uv_thread_create(t.get(), PlatformWorkerThread,
Expand All @@ -46,7 +167,7 @@ void WorkerThreadsTaskRunner::PostTask(std::unique_ptr<Task> task) {

void WorkerThreadsTaskRunner::PostDelayedTask(std::unique_ptr<v8::Task> task,
double delay_in_seconds) {
UNREACHABLE();
delayed_task_scheduler_->PostDelayedTask(std::move(task), delay_in_seconds);
}

void WorkerThreadsTaskRunner::BlockingDrain() {
Expand All @@ -55,6 +176,7 @@ void WorkerThreadsTaskRunner::BlockingDrain() {

void WorkerThreadsTaskRunner::Shutdown() {
pending_worker_tasks_.Stop();
delayed_task_scheduler_->Stop();
for (size_t i = 0; i < threads_.size(); i++) {
CHECK_EQ(0, uv_thread_join(threads_[i].get()));
}
Expand Down
4 changes: 4 additions & 0 deletions src/node_platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ class WorkerThreadsTaskRunner {

private:
TaskQueue<v8::Task> pending_worker_tasks_;

class DelayedTaskScheduler;
std::unique_ptr<DelayedTaskScheduler> delayed_task_scheduler_;

std::vector<std::unique_ptr<uv_thread_t>> threads_;
};

Expand Down
21 changes: 21 additions & 0 deletions test/sequential/test-inspector-runtime-evaluate-with-timeout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Flags: --expose-internals
'use strict';

const common = require('../common');
common.skipIfInspectorDisabled();

(async function test() {
const { strictEqual } = require('assert');
const { Session } = require('inspector');
const { promisify } = require('util');

const session = new Session();
session.connect();
session.post = promisify(session.post);
const result = await session.post('Runtime.evaluate', {
expression: 'for(;;);',
timeout: 0
}).catch((e) => e);
strictEqual(result.message, 'Execution was terminated');
session.disconnect();
})();

1 comment on commit b1e2612

@alexkozy
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nodejs/release could you share estimated timeframe for next Node 10 release? How can I help to make it happens faster?
This commit is crucial for DevTools console and ndb, without it as soon as user tries to execute anything in console - node process crashes.

Please sign in to comment.