Skip to content

Commit

Permalink
src: remove AsyncScope and AsyncCallbackScope
Browse files Browse the repository at this point in the history
Reduce the number of different scopes we use for async callbacks.

PR-URL: #30236
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax authored and MylesBorins committed Jan 10, 2020
1 parent f70d518 commit f080239
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 68 deletions.
13 changes: 8 additions & 5 deletions src/api/callback.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,12 @@ 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,
Expand All @@ -47,10 +48,11 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
: env_(env),
async_context_(asyncContext),
object_(object),
callback_scope_(env),
skip_hooks_(flags & kSkipAsyncHooks) {
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 @@ -74,6 +76,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,

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

void InternalCallbackScope::Close() {
Expand All @@ -94,7 +97,7 @@ void InternalCallbackScope::Close() {
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 @@ -723,20 +723,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
20 changes: 13 additions & 7 deletions src/node_http_parser_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -321,10 +321,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 @@ -392,9 +395,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
14 changes: 10 additions & 4 deletions src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -206,17 +206,23 @@ v8::MaybeLocal<v8::Value> InternalMakeCallback(
class InternalCallbackScope {
public:
enum Flags {
kNoFlags = 0,
// Tell the constructor whether its `object` parameter may be empty or not.
kAllowEmptyResource = 1,
// Indicates whether 'before' and 'after' hooks should be skipped.
kSkipAsyncHooks = 2
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,
int flags = 0);
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 @@ -227,8 +233,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
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

0 comments on commit f080239

Please sign in to comment.