Skip to content

Commit

Permalink
src: prepare platform for upstream V8 changes
Browse files Browse the repository at this point in the history
V8 platform tasks may schedule other tasks (both background and
foreground), and may perform asynchronous operations like
resolving Promises.

To address that:

- Run the task queue drain call inside a callback scope.
  This makes sure asynchronous operations inside it, like
  resolving promises, lead to the microtask queue and any
  subsequent operations not being silently forgotten.
- Move the task queue drain call before `EmitBeforeExit()`
  and only run `EmitBeforeExit()` if there is no new event
  loop work.
- Account for possible new foreground tasks scheduled by
  background tasks in `DrainBackgroundTasks()`.

PR-URL: #15428
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matthew Loring <mattloring@google.com>
  • Loading branch information
addaleax committed Sep 21, 2017
1 parent 01c680b commit f27b5e4
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 37 deletions.
45 changes: 16 additions & 29 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1339,30 +1339,6 @@ void AddPromiseHook(v8::Isolate* isolate, promise_hook_func fn, void* arg) {
env->AddPromiseHook(fn, arg);
}

class InternalCallbackScope {
public:
InternalCallbackScope(Environment* env,
Local<Object> object,
const async_context& asyncContext);
~InternalCallbackScope();
void Close();

inline bool Failed() const { return failed_; }
inline void MarkAsFailed() { failed_ = true; }
inline bool IsInnerMakeCallback() const {
return callback_scope_.in_makecallback();
}

private:
Environment* env_;
async_context async_context_;
v8::Local<v8::Object> object_;
Environment::AsyncCallbackScope callback_scope_;
bool failed_ = false;
bool pushed_ids_ = false;
bool closed_ = false;
};

CallbackScope::CallbackScope(Isolate* isolate,
Local<Object> object,
async_context asyncContext)
Expand All @@ -1381,17 +1357,21 @@ CallbackScope::~CallbackScope() {

InternalCallbackScope::InternalCallbackScope(Environment* env,
Local<Object> object,
const async_context& asyncContext)
const async_context& asyncContext,
ResourceExpectation expect)
: env_(env),
async_context_(asyncContext),
object_(object),
callback_scope_(env) {
CHECK(!object.IsEmpty());
if (expect == kRequireResource) {
CHECK(!object.IsEmpty());
}

HandleScope handle_scope(env->isolate());
// If you hit this assertion, you forgot to enter the v8::Context first.
CHECK_EQ(env->context(), env->isolate()->GetCurrentContext());

if (env->using_domains()) {
if (env->using_domains() && !object_.IsEmpty()) {
DomainEnter(env, object_);
}

Expand All @@ -1413,6 +1393,7 @@ InternalCallbackScope::~InternalCallbackScope() {
void InternalCallbackScope::Close() {
if (closed_) return;
closed_ = true;
HandleScope handle_scope(env_->isolate());

if (pushed_ids_)
env_->async_hooks()->pop_ids(async_context_.async_id);
Expand All @@ -1423,7 +1404,7 @@ void InternalCallbackScope::Close() {
AsyncWrap::EmitAfter(env_, async_context_.async_id);
}

if (env_->using_domains()) {
if (env_->using_domains() && !object_.IsEmpty()) {
DomainExit(env_, object_);
}

Expand Down Expand Up @@ -1463,6 +1444,7 @@ MaybeLocal<Value> InternalMakeCallback(Environment* env,
int argc,
Local<Value> argv[],
async_context asyncContext) {
CHECK(!recv.IsEmpty());
InternalCallbackScope scope(env, recv, asyncContext);
if (scope.Failed()) {
return Undefined(env->isolate());
Expand Down Expand Up @@ -4726,9 +4708,14 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
do {
uv_run(env.event_loop(), UV_RUN_DEFAULT);

v8_platform.DrainVMTasks();

more = uv_loop_alive(env.event_loop());
if (more)
continue;

EmitBeforeExit(&env);

v8_platform.DrainVMTasks();
// Emit `beforeExit` if the loop became alive either after emitting
// event, or after running some callbacks.
more = uv_loop_alive(env.event_loop());
Expand Down
28 changes: 28 additions & 0 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,36 @@ v8::MaybeLocal<v8::Value> InternalMakeCallback(
v8::Local<v8::Value> argv[],
async_context asyncContext);

class InternalCallbackScope {
public:
// Tell the constructor whether its `object` parameter may be empty or not.
enum ResourceExpectation { kRequireResource, kAllowEmptyResource };
InternalCallbackScope(Environment* env,
v8::Local<v8::Object> object,
const async_context& asyncContext,
ResourceExpectation expect = kRequireResource);
~InternalCallbackScope();
void Close();

inline bool Failed() const { return failed_; }
inline void MarkAsFailed() { failed_ = true; }
inline bool IsInnerMakeCallback() const {
return callback_scope_.in_makecallback();
}

private:
Environment* env_;
async_context async_context_;
v8::Local<v8::Object> object_;
Environment::AsyncCallbackScope callback_scope_;
bool failed_ = false;
bool pushed_ids_ = false;
bool closed_ = false;
};

} // namespace node


#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#endif // SRC_NODE_INTERNALS_H_
30 changes: 23 additions & 7 deletions src/node_platform.cc
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
#include "node_platform.h"
#include "node_internals.h"

#include "util.h"

namespace node {

using v8::HandleScope;
using v8::Isolate;
using v8::Local;
using v8::Object;
using v8::Platform;
using v8::Task;
using v8::TracingController;
Expand Down Expand Up @@ -63,22 +67,33 @@ size_t NodePlatform::NumberOfAvailableBackgroundThreads() {
return threads_.size();
}

static void RunForegroundTask(uv_timer_t* handle) {
Task* task = static_cast<Task*>(handle->data);
static void RunForegroundTask(Task* task) {
Isolate* isolate = Isolate::GetCurrent();
HandleScope scope(isolate);
Environment* env = Environment::GetCurrent(isolate);
InternalCallbackScope cb_scope(env, Local<Object>(), { 0, 0 },
InternalCallbackScope::kAllowEmptyResource);
task->Run();
delete task;
}

static void RunForegroundTask(uv_timer_t* handle) {
Task* task = static_cast<Task*>(handle->data);
RunForegroundTask(task);
uv_close(reinterpret_cast<uv_handle_t*>(handle), [](uv_handle_t* handle) {
delete reinterpret_cast<uv_timer_t*>(handle);
});
}

void NodePlatform::DrainBackgroundTasks() {
FlushForegroundTasksInternal();
background_tasks_.BlockingDrain();
while (FlushForegroundTasksInternal())
background_tasks_.BlockingDrain();
}

void NodePlatform::FlushForegroundTasksInternal() {
bool NodePlatform::FlushForegroundTasksInternal() {
bool did_work = false;
while (auto delayed = foreground_delayed_tasks_.Pop()) {
did_work = true;
uint64_t delay_millis =
static_cast<uint64_t>(delayed->second + 0.5) * 1000;
uv_timer_t* handle = new uv_timer_t();
Expand All @@ -91,9 +106,10 @@ void NodePlatform::FlushForegroundTasksInternal() {
delete delayed;
}
while (Task* task = foreground_tasks_.Pop()) {
task->Run();
delete task;
did_work = true;
RunForegroundTask(task);
}
return did_work;
}

void NodePlatform::CallOnBackgroundThread(Task* task,
Expand Down
3 changes: 2 additions & 1 deletion src/node_platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ class NodePlatform : public v8::Platform {
virtual ~NodePlatform() {}

void DrainBackgroundTasks();
void FlushForegroundTasksInternal();
// Returns true iff work was dispatched or executed.
bool FlushForegroundTasksInternal();
void Shutdown();

// v8::Platform implementation.
Expand Down

0 comments on commit f27b5e4

Please sign in to comment.