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

src: remove AsyncScope, AsyncCallbackScope, RunNextTicksNative #30236

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions src/api/callback.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,22 +34,25 @@ CallbackScope::~CallbackScope() {
delete private_;
}

InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap)
InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap, int flags)
: InternalCallbackScope(async_wrap->env(),
async_wrap->object(),
{ async_wrap->get_async_id(),
async_wrap->get_trigger_async_id() }) {}
async_wrap->get_trigger_async_id() },
flags) {}

InternalCallbackScope::InternalCallbackScope(Environment* env,
Local<Object> object,
const async_context& asyncContext,
ResourceExpectation expect)
int flags)
: env_(env),
async_context_(asyncContext),
object_(object),
callback_scope_(env) {
CHECK_IMPLIES(expect == kRequireResource, !object.IsEmpty());
skip_hooks_(flags & kSkipAsyncHooks),
skip_task_queues_(flags & kSkipTaskQueues) {
CHECK_IMPLIES(!(flags & kAllowEmptyResource), !object.IsEmpty());
CHECK_NOT_NULL(env);
env->PushAsyncCallbackScope();

if (!env->can_call_into_js()) {
failed_ = true;
Expand All @@ -60,7 +63,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
// If you hit this assertion, you forgot to enter the v8::Context first.
CHECK_EQ(Environment::GetCurrent(env->isolate()), env);

if (asyncContext.async_id != 0) {
if (asyncContext.async_id != 0 && !skip_hooks_) {
// No need to check a return value because the application will exit if
// an exception occurs.
AsyncWrap::EmitBefore(env, asyncContext.async_id);
Expand All @@ -73,6 +76,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,

InternalCallbackScope::~InternalCallbackScope() {
Close();
env_->PopAsyncCallbackScope();
}

void InternalCallbackScope::Close() {
Expand All @@ -89,11 +93,11 @@ void InternalCallbackScope::Close() {

if (failed_) return;

if (async_context_.async_id != 0) {
if (async_context_.async_id != 0 && !skip_hooks_) {
AsyncWrap::EmitAfter(env_, async_context_.async_id);
}

if (env_->async_callback_scope_depth() > 1) {
if (env_->async_callback_scope_depth() > 1 || skip_task_queues_) {
return;
}

Expand Down
14 changes: 0 additions & 14 deletions src/async_wrap-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,6 @@ inline double AsyncWrap::get_trigger_async_id() const {
}


inline AsyncWrap::AsyncScope::AsyncScope(AsyncWrap* wrap)
: wrap_(wrap) {
Environment* env = wrap->env();
if (env->async_hooks()->fields()[AsyncHooks::kBefore] == 0) return;
EmitBefore(env, wrap->get_async_id());
}

inline AsyncWrap::AsyncScope::~AsyncScope() {
Environment* env = wrap_->env();
if (env->async_hooks()->fields()[AsyncHooks::kAfter] == 0) return;
EmitAfter(env, wrap_->get_async_id());
}


inline v8::MaybeLocal<v8::Value> AsyncWrap::MakeCallback(
const v8::Local<v8::String> symbol,
int argc,
Expand Down
13 changes: 0 additions & 13 deletions src/async_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ class AsyncWrap : public BaseObject {
inline ProviderType set_provider_type(ProviderType provider);

inline double get_async_id() const;

inline double get_trigger_async_id() const;

void AsyncReset(v8::Local<v8::Object> resource,
Expand Down Expand Up @@ -200,18 +199,6 @@ class AsyncWrap : public BaseObject {
static v8::Local<v8::Object> GetOwner(Environment* env,
v8::Local<v8::Object> obj);

// This is a simplified version of InternalCallbackScope that only runs
// the `before` and `after` hooks. Only use it when not actually calling
// back into JS; otherwise, use InternalCallbackScope.
class AsyncScope {
public:
explicit inline AsyncScope(AsyncWrap* wrap);
~AsyncScope();

private:
AsyncWrap* wrap_ = nullptr;
};

bool IsDoneInitializing() const override;

private:
Expand Down
8 changes: 0 additions & 8 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -211,14 +211,6 @@ Environment* Environment::ForAsyncHooks(AsyncHooks* hooks) {
return ContainerOf(&Environment::async_hooks_, hooks);
}

inline AsyncCallbackScope::AsyncCallbackScope(Environment* env) : env_(env) {
env_->PushAsyncCallbackScope();
}

inline AsyncCallbackScope::~AsyncCallbackScope() {
env_->PopAsyncCallbackScope();
}

inline size_t Environment::async_callback_scope_depth() const {
return async_callback_scope_depth_;
}
Expand Down
14 changes: 0 additions & 14 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -721,20 +721,6 @@ class AsyncHooks : public MemoryRetainer {
void grow_async_ids_stack();
};

class AsyncCallbackScope {
public:
AsyncCallbackScope() = delete;
explicit AsyncCallbackScope(Environment* env);
~AsyncCallbackScope();
AsyncCallbackScope(const AsyncCallbackScope&) = delete;
AsyncCallbackScope& operator=(const AsyncCallbackScope&) = delete;
AsyncCallbackScope(AsyncCallbackScope&&) = delete;
AsyncCallbackScope& operator=(AsyncCallbackScope&&) = delete;

private:
Environment* env_;
};

class ImmediateInfo : public MemoryRetainer {
public:
inline AliasedUint32Array& fields();
Expand Down
9 changes: 2 additions & 7 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -406,13 +406,8 @@ MaybeLocal<Value> StartExecution(Environment* env, const char* main_script_id) {
->GetFunction(env->context())
.ToLocalChecked()};

Local<Value> result;
if (!ExecuteBootstrapper(env, main_script_id, &parameters, &arguments)
.ToLocal(&result) ||
!task_queue::RunNextTicksNative(env)) {
return MaybeLocal<Value>();
}
return scope.Escape(result);
return scope.EscapeMaybe(
ExecuteBootstrapper(env, main_script_id, &parameters, &arguments));
}

MaybeLocal<Value> StartMainThreadExecution(Environment* env) {
Expand Down
20 changes: 13 additions & 7 deletions src/node_http_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -310,10 +310,13 @@ class Parser : public AsyncWrap, public StreamListener {

argv[A_UPGRADE] = Boolean::New(env()->isolate(), parser_.upgrade);

AsyncCallbackScope callback_scope(env());

MaybeLocal<Value> head_response =
MakeCallback(cb.As<Function>(), arraysize(argv), argv);
MaybeLocal<Value> head_response;
{
InternalCallbackScope callback_scope(
this, InternalCallbackScope::kSkipTaskQueues);
head_response = cb.As<Function>()->Call(
env()->context(), object(), arraysize(argv), argv);
}

int64_t val;

Expand Down Expand Up @@ -379,9 +382,12 @@ class Parser : public AsyncWrap, public StreamListener {
if (!cb->IsFunction())
return 0;

AsyncCallbackScope callback_scope(env());

MaybeLocal<Value> r = MakeCallback(cb.As<Function>(), 0, nullptr);
MaybeLocal<Value> r;
{
InternalCallbackScope callback_scope(
this, InternalCallbackScope::kSkipTaskQueues);
r = cb.As<Function>()->Call(env()->context(), object(), 0, nullptr);
}

if (r.IsEmpty()) {
got_exception_ = true;
Expand Down
21 changes: 16 additions & 5 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -199,14 +199,24 @@ v8::MaybeLocal<v8::Value> InternalMakeCallback(

class InternalCallbackScope {
public:
// Tell the constructor whether its `object` parameter may be empty or not.
enum ResourceExpectation { kRequireResource, kAllowEmptyResource };
enum Flags {
kNoFlags = 0,
// Tell the constructor whether its `object` parameter may be empty or not.
addaleax marked this conversation as resolved.
Show resolved Hide resolved
kAllowEmptyResource = 1,
// Indicates whether 'before' and 'after' hooks should be skipped.
kSkipAsyncHooks = 2,
// Indicates whether nextTick and microtask queues should be skipped.
// This should only be used when there is no call into JS in this scope.
// (The HTTP parser also uses it for some weird backwards
// compatibility issues, but it shouldn't.)
kSkipTaskQueues = 4
};
InternalCallbackScope(Environment* env,
v8::Local<v8::Object> object,
const async_context& asyncContext,
ResourceExpectation expect = kRequireResource);
int flags = kNoFlags);
// Utility that can be used by AsyncWrap classes.
explicit InternalCallbackScope(AsyncWrap* async_wrap);
explicit InternalCallbackScope(AsyncWrap* async_wrap, int flags = 0);
~InternalCallbackScope();
void Close();

Expand All @@ -217,7 +227,8 @@ class InternalCallbackScope {
Environment* env_;
async_context async_context_;
v8::Local<v8::Object> object_;
AsyncCallbackScope callback_scope_;
bool skip_hooks_;
bool skip_task_queues_;
bool failed_ = false;
bool pushed_ids_ = false;
bool closed_ = false;
Expand Down
10 changes: 7 additions & 3 deletions src/node_main_instance.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ using v8::HandleScope;
using v8::Isolate;
using v8::Local;
using v8::Locker;
using v8::Object;
using v8::SealHandleScope;

NodeMainInstance::NodeMainInstance(Isolate* isolate,
Expand Down Expand Up @@ -111,10 +112,13 @@ int NodeMainInstance::Run() {

if (exit_code == 0) {
{
AsyncCallbackScope callback_scope(env.get());
env->async_hooks()->push_async_ids(1, 0);
InternalCallbackScope callback_scope(
env.get(),
Local<Object>(),
{ 1, 0 },
InternalCallbackScope::kAllowEmptyResource |
InternalCallbackScope::kSkipAsyncHooks);
LoadEnvironment(env.get());
env->async_hooks()->pop_async_id(1);
}

env->set_trace_sync_io(env->options()->trace_sync_io);
Expand Down
9 changes: 0 additions & 9 deletions src/node_process.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,6 @@ v8::Maybe<bool> ProcessEmitDeprecationWarning(Environment* env,
v8::MaybeLocal<v8::Object> CreateProcessObject(Environment* env);
void PatchProcessObject(const v8::FunctionCallbackInfo<v8::Value>& args);

namespace task_queue {
// Handle any nextTicks added in the first tick of the program.
// We use the native version here for once so that any microtasks
// created by the main module is then handled from C++, and
// the call stack of the main script does not show up in the async error
// stack trace.
bool RunNextTicksNative(Environment* env);
} // namespace task_queue

} // namespace node
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
#endif // SRC_NODE_PROCESS_H_
16 changes: 0 additions & 16 deletions src/node_task_queue.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,6 @@ static void EnqueueMicrotask(const FunctionCallbackInfo<Value>& args) {
isolate->EnqueueMicrotask(args[0].As<Function>());
}

// Should be in sync with runNextTicks in internal/process/task_queues.js
bool RunNextTicksNative(Environment* env) {
OnScopeLeave weakref_cleanup([&]() { env->RunWeakRefCleanup(); });

TickInfo* tick_info = env->tick_info();
if (!tick_info->has_tick_scheduled() && !tick_info->has_rejection_to_warn())
MicrotasksScope::PerformCheckpoint(env->isolate());
if (!tick_info->has_tick_scheduled() && !tick_info->has_rejection_to_warn())
return true;

Local<Function> callback = env->tick_callback_function();
CHECK(!callback.IsEmpty());
return !callback->Call(env->context(), env->process_object(), 0, nullptr)
.IsEmpty();
}

static void RunMicrotasks(const FunctionCallbackInfo<Value>& args) {
MicrotasksScope::PerformCheckpoint(args.GetIsolate());
}
Expand Down
11 changes: 7 additions & 4 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -273,17 +273,20 @@ void Worker::Run() {
inspector_started = true;
#endif
HandleScope handle_scope(isolate_);
AsyncCallbackScope callback_scope(env_.get());
env_->async_hooks()->push_async_ids(1, 0);
InternalCallbackScope callback_scope(
env_.get(),
Local<Object>(),
{ 1, 0 },
InternalCallbackScope::kAllowEmptyResource |
InternalCallbackScope::kSkipAsyncHooks);

if (!env_->RunBootstrapping().IsEmpty()) {
CreateEnvMessagePort(env_.get());
if (is_stopped()) return;
Debug(this, "Created message port for worker %llu", thread_id_);
USE(StartExecution(env_.get(), "internal/main/worker_thread"));
}

env_->async_hooks()->pop_async_id(1);

Debug(this, "Loaded environment for worker %llu", thread_id_);
}

Expand Down
9 changes: 6 additions & 3 deletions src/stream_pipe.cc
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ void StreamPipe::ReadableListener::OnStreamRead(ssize_t nread,
const uv_buf_t& buf_) {
StreamPipe* pipe = ContainerOf(&StreamPipe::readable_listener_, this);
AllocatedBuffer buf(pipe->env(), buf_);
AsyncScope async_scope(pipe);
if (nread < 0) {
// EOF or error; stop reading and pass the error to the previous listener
// (which might end up in JS).
Expand Down Expand Up @@ -162,7 +161,9 @@ void StreamPipe::WritableListener::OnStreamAfterWrite(WriteWrap* w,
StreamPipe* pipe = ContainerOf(&StreamPipe::writable_listener_, this);
pipe->is_writing_ = false;
if (pipe->is_eof_) {
AsyncScope async_scope(pipe);
HandleScope handle_scope(pipe->env()->isolate());
InternalCallbackScope callback_scope(pipe,
InternalCallbackScope::kSkipTaskQueues);
pipe->ShutdownWritable();
pipe->Unpipe();
return;
Expand Down Expand Up @@ -206,7 +207,9 @@ void StreamPipe::WritableListener::OnStreamWantsWrite(size_t suggested_size) {
pipe->wanted_data_ = suggested_size;
if (pipe->is_reading_ || pipe->is_closed_)
return;
AsyncScope async_scope(pipe);
HandleScope handle_scope(pipe->env()->isolate());
InternalCallbackScope callback_scope(pipe,
InternalCallbackScope::kSkipTaskQueues);
pipe->is_reading_ = true;
pipe->source()->ReadStart();
}
Expand Down