src: remove AsyncScope and AsyncCallbackScope

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>
nodejs · Nov 17, 2019 · 0072a8eddfe4bb444856c1aea09ed72bf6d359d5 · 0072a8e
1 parent e3371f0
commit 0072a8eddfe4bb444856c1aea09ed72bf6d359d5
Showing with 37 additions and 68 deletions.

+8 −5 src/api/callback.cc

+0 −14 src/async_wrap-inl.h

+0 −13 src/async_wrap.h

+0 −8 src/env-inl.h

+0 −14 src/env.h

+13 −7 src/node_http_parser.cc

+10 −4 src/node_internals.h

+6 −3 src/stream_pipe.cc
diff --git a/src/api/callback.cc b/src/api/callback.cc
@@ -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,
@@ -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;
@@ -74,6 +76,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
 
 InternalCallbackScope::~InternalCallbackScope() {
   Close();
+  env_->PopAsyncCallbackScope();
 }
 
 void InternalCallbackScope::Close() {
@@ -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;
   }
 

diff --git a/src/async_wrap-inl.h b/src/async_wrap-inl.h
@@ -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,

diff --git a/src/async_wrap.h b/src/async_wrap.h
@@ -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,
@@ -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:

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -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_;
 }

diff --git a/src/env.h b/src/env.h
@@ -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();

diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
@@ -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;
 
@@ -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;

diff --git a/src/node_internals.h b/src/node_internals.h
@@ -204,17 +204,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();
 
@@ -225,8 +231,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;

diff --git a/src/stream_pipe.cc b/src/stream_pipe.cc
@@ -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).
@@ -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;
@@ -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();
 }