From 91d64a3774bfe83f1e3efb51fef2409fcff45b5b Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 21 Mar 2023 08:04:11 -0700 Subject: [PATCH] Revert "[EventEngine] Return correct value EventEngine::IsWorkerThread and use this value in completion queue handling to correctly offload work" (#32656) Reverts grpc/grpc#32637 Apologies for not catching this before submission. I continue to think that IsWorkerThread as an EventEngine member function is a poor API choice: it implies that the thread is a thread for the referenced event engine, and that's not what we're returning, and also rarely useful. If we want this concept in the API it should be a static function with a way of registering a thread into the set. I however firmly believe we'll do better long term if we abandon the concept of being able to identify from outside who created a thread and instead solve problems in other ways. --- src/core/lib/event_engine/cf_engine/cf_engine.cc | 2 +- src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc | 2 +- src/core/lib/event_engine/posix_engine/posix_engine.cc | 4 +--- src/core/lib/event_engine/windows/windows_engine.cc | 4 +--- src/core/lib/surface/completion_queue.cc | 8 +------- 5 files changed, 5 insertions(+), 15 deletions(-) diff --git a/src/core/lib/event_engine/cf_engine/cf_engine.cc b/src/core/lib/event_engine/cf_engine/cf_engine.cc index 196488f1906..4a33d84d005 100644 --- a/src/core/lib/event_engine/cf_engine/cf_engine.cc +++ b/src/core/lib/event_engine/cf_engine/cf_engine.cc @@ -82,7 +82,7 @@ bool CFEventEngine::CancelConnect(ConnectionHandle /* handle */) { grpc_core::Crash("unimplemented"); } -bool CFEventEngine::IsWorkerThread() { return executor_->IsThreadPoolThread(); } +bool CFEventEngine::IsWorkerThread() { grpc_core::Crash("unimplemented"); } std::unique_ptr CFEventEngine::GetDNSResolver( const DNSResolver::ResolverOptions& /* options */) { diff --git a/src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc b/src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc index b66029b247b..0d117229424 100644 --- a/src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc +++ b/src/core/lib/event_engine/posix_engine/ev_epoll1_linux.cc @@ -349,7 +349,7 @@ void Epoll1EventHandle::HandleShutdownInternal(absl::Status why, } } write_closure_->SetShutdown(why); - error_closure_->SetShutdown(why); + write_closure_->SetShutdown(why); } } diff --git a/src/core/lib/event_engine/posix_engine/posix_engine.cc b/src/core/lib/event_engine/posix_engine/posix_engine.cc index 995a8a2fddb..31a7cf6ed6f 100644 --- a/src/core/lib/event_engine/posix_engine/posix_engine.cc +++ b/src/core/lib/event_engine/posix_engine/posix_engine.cc @@ -487,9 +487,7 @@ std::unique_ptr PosixEventEngine::GetDNSResolver( grpc_core::Crash("unimplemented"); } -bool PosixEventEngine::IsWorkerThread() { - return executor_->IsThreadPoolThread(); -} +bool PosixEventEngine::IsWorkerThread() { grpc_core::Crash("unimplemented"); } bool PosixEventEngine::CancelConnect(EventEngine::ConnectionHandle handle) { #ifdef GRPC_POSIX_SOCKET_TCP diff --git a/src/core/lib/event_engine/windows/windows_engine.cc b/src/core/lib/event_engine/windows/windows_engine.cc index 6dd3f10b67e..86167ee255f 100644 --- a/src/core/lib/event_engine/windows/windows_engine.cc +++ b/src/core/lib/event_engine/windows/windows_engine.cc @@ -181,9 +181,7 @@ std::unique_ptr WindowsEventEngine::GetDNSResolver( grpc_core::Crash("unimplemented"); } -bool WindowsEventEngine::IsWorkerThread() { - return executor_->IsThreadPoolThread(); -} +bool WindowsEventEngine::IsWorkerThread() { grpc_core::Crash("unimplemented"); } void WindowsEventEngine::OnConnectCompleted( std::shared_ptr state) { diff --git a/src/core/lib/surface/completion_queue.cc b/src/core/lib/surface/completion_queue.cc index c68a6039d2f..aaa0caf811b 100644 --- a/src/core/lib/surface/completion_queue.cc +++ b/src/core/lib/surface/completion_queue.cc @@ -25,7 +25,6 @@ #include #include #include -#include #include #include #include @@ -35,7 +34,6 @@ #include "absl/strings/str_format.h" #include "absl/strings/str_join.h" -#include #include #include #include @@ -45,7 +43,6 @@ #include "src/core/lib/debug/stats.h" #include "src/core/lib/debug/stats_data.h" -#include "src/core/lib/event_engine/default_event_engine.h" #include "src/core/lib/gpr/spinlock.h" #include "src/core/lib/gprpp/atomic_utils.h" #include "src/core/lib/gprpp/debug_location.h" @@ -861,11 +858,8 @@ static void cq_end_op_for_callback( // 2. The callback is marked inlineable and there is an ACEC available // 3. We are already running in a background poller thread (which always has // an ACEC available at the base of the stack). - // 4. We are running in an event engine thread with ACEC available. auto* functor = static_cast(tag); - if (((internal || functor->inlineable || - grpc_event_engine::experimental::GetDefaultEventEngine() - ->IsWorkerThread()) && + if (((internal || functor->inlineable) && grpc_core::ApplicationCallbackExecCtx::Available()) || grpc_iomgr_is_any_background_poller_thread()) { grpc_core::ApplicationCallbackExecCtx::Enqueue(functor, (error.ok()));