From 08e55e302bb39bf491e0f71619d494f62b24428e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 3 Nov 2019 13:28:34 +0100 Subject: [PATCH] src: remove AsyncScope and AsyncCallbackScope Reduce the number of different scopes we use for async callbacks. PR-URL: https://github.com/nodejs/node/pull/30236 Reviewed-By: Franziska Hinkelmann Reviewed-By: Gus Caplan Reviewed-By: David Carlier Reviewed-By: James M Snell --- src/api/callback.cc | 13 ++++++++----- src/async_wrap-inl.h | 14 -------------- src/async_wrap.h | 13 ------------- src/env-inl.h | 8 -------- src/env.h | 14 -------------- src/node_http_parser_impl.h | 20 +++++++++++++------- src/node_internals.h | 14 ++++++++++---- src/stream_pipe.cc | 9 ++++++--- 8 files changed, 37 insertions(+), 68 deletions(-) diff --git a/src/api/callback.cc b/src/api/callback.cc index bced9bb7abe919..238bf49a54f76d 100644 --- 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, @@ -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 index 53972493052f5e..e3e48666e4fbd6 100644 --- 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 AsyncWrap::MakeCallback( const v8::Local symbol, int argc, diff --git a/src/async_wrap.h b/src/async_wrap.h index 2651b5a054d554..dd82497a259243 100644 --- 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 resource, @@ -200,18 +199,6 @@ class AsyncWrap : public BaseObject { static v8::Local GetOwner(Environment* env, v8::Local 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 index d61ae8f8df1d17..79708080d1e634 100644 --- 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 index eb7c4dc5a80bd9..66077560e1f552 100644 --- a/src/env.h +++ b/src/env.h @@ -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(); diff --git a/src/node_http_parser_impl.h b/src/node_http_parser_impl.h index 1b411dee9c86bd..cc42ef1e77819e 100644 --- a/src/node_http_parser_impl.h +++ b/src/node_http_parser_impl.h @@ -334,10 +334,13 @@ class Parser : public AsyncWrap, public StreamListener { argv[A_UPGRADE] = Boolean::New(env()->isolate(), parser_.upgrade); - AsyncCallbackScope callback_scope(env()); - - MaybeLocal head_response = - MakeCallback(cb.As(), arraysize(argv), argv); + MaybeLocal head_response; + { + InternalCallbackScope callback_scope( + this, InternalCallbackScope::kSkipTaskQueues); + head_response = cb.As()->Call( + env()->context(), object(), arraysize(argv), argv); + } int64_t val; @@ -405,9 +408,12 @@ class Parser : public AsyncWrap, public StreamListener { if (!cb->IsFunction()) return 0; - AsyncCallbackScope callback_scope(env()); - - MaybeLocal r = MakeCallback(cb.As(), 0, nullptr); + MaybeLocal r; + { + InternalCallbackScope callback_scope( + this, InternalCallbackScope::kSkipTaskQueues); + r = cb.As()->Call(env()->context(), object(), 0, nullptr); + } if (r.IsEmpty()) { got_exception_ = true; diff --git a/src/node_internals.h b/src/node_internals.h index a53d75546e3850..ddf784e957ada4 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -206,17 +206,23 @@ v8::MaybeLocal 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 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(); @@ -227,8 +233,8 @@ class InternalCallbackScope { Environment* env_; async_context async_context_; v8::Local 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 index be7fc882ea5c8a..832a20d324f0ea 100644 --- 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(); }