From 5a5adfb1b953ae0b6894b7345931dd5e51a6fe6e Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 26 Aug 2022 15:09:56 -0700 Subject: [PATCH] [fixit] Ensure ordering between iomgr and event engine shutdown (#30764) * [fixit] Terminate event engine before iomgr * guard against self-joins * build * fix api_fuzzer * Automated change: Fix sanity tests * add test Co-authored-by: ctiller --- BUILD | 3 +++ .../lib/event_engine/posix_engine/timer_manager.cc | 6 ++++++ .../lib/event_engine/posix_engine/timer_manager.h | 2 ++ src/core/lib/surface/init.cc | 5 +++++ .../fuzzing_event_engine/fuzzing_event_engine.cc | 3 +++ test/core/surface/init_test.cc | 14 ++++++++++++++ 6 files changed, 33 insertions(+) diff --git a/BUILD b/BUILD index 2f97f97d3ce..56d78d8019f 100644 --- a/BUILD +++ b/BUILD @@ -428,6 +428,7 @@ grpc_cc_library( "grpc_trace", "http_connect_handshaker", "iomgr_timer", + "posix_event_engine_timer_manager", "slice", "tcp_connect_handshaker", ], @@ -493,6 +494,7 @@ grpc_cc_library( "grpc_trace", "http_connect_handshaker", "iomgr_timer", + "posix_event_engine_timer_manager", "slice", "tcp_connect_handshaker", ], @@ -2360,6 +2362,7 @@ grpc_cc_library( "forkable", "gpr", "gpr_codegen", + "gpr_tls", "posix_event_engine_timer", "time", ], diff --git a/src/core/lib/event_engine/posix_engine/timer_manager.cc b/src/core/lib/event_engine/posix_engine/timer_manager.cc index 70d91df681f..8a0524622c4 100644 --- a/src/core/lib/event_engine/posix_engine/timer_manager.cc +++ b/src/core/lib/event_engine/posix_engine/timer_manager.cc @@ -32,8 +32,11 @@ #include #include +#include "src/core/lib/gpr/tls.h" #include "src/core/lib/gprpp/thd.h" +static GPR_THREAD_LOCAL(bool) g_timer_thread = false; + namespace grpc_event_engine { namespace posix_engine { @@ -196,6 +199,7 @@ void TimerManager::MainLoop() { } void TimerManager::RunThread(void* arg) { + g_timer_thread = true; std::unique_ptr thread(static_cast(arg)); thread->self->Run(std::move(thread->thread)); } @@ -208,6 +212,8 @@ void TimerManager::Run(grpc_core::Thread thread) { if (thread_count_ == 0) cv_threadcount_.Signal(); } +bool TimerManager::IsTimerManagerThread() { return g_timer_thread; } + TimerManager::TimerManager() : host_(this) { timer_list_ = absl::make_unique(&host_); grpc_core::MutexLock lock(&mu_); diff --git a/src/core/lib/event_engine/posix_engine/timer_manager.h b/src/core/lib/event_engine/posix_engine/timer_manager.h index 22048bb8805..5467205a301 100644 --- a/src/core/lib/event_engine/posix_engine/timer_manager.h +++ b/src/core/lib/event_engine/posix_engine/timer_manager.h @@ -60,6 +60,8 @@ class TimerManager final : public grpc_event_engine::experimental::Forkable { void PostforkParent() override; void PostforkChild() override; + static bool IsTimerManagerThread(); + private: struct RunThreadArgs { TimerManager* self; diff --git a/src/core/lib/surface/init.cc b/src/core/lib/surface/init.cc index b0b22992b46..14be131e405 100644 --- a/src/core/lib/surface/init.cc +++ b/src/core/lib/surface/init.cc @@ -37,7 +37,9 @@ #include "src/core/lib/channel/channel_stack_builder.h" #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/debug/trace.h" +#include "src/core/lib/event_engine/default_event_engine.h" #include "src/core/lib/event_engine/forkable.h" +#include "src/core/lib/event_engine/posix_engine/timer_manager.h" #include "src/core/lib/gprpp/fork.h" #include "src/core/lib/gprpp/sync.h" #include "src/core/lib/gprpp/thd.h" @@ -179,6 +181,7 @@ void grpc_shutdown_internal_locked(void) } } } + grpc_event_engine::experimental::ResetDefaultEventEngine(); grpc_iomgr_shutdown(); gpr_timers_global_destroy(); grpc_tracer_shutdown(); @@ -208,6 +211,8 @@ void grpc_shutdown(void) { grpc_core::ApplicationCallbackExecCtx* acec = grpc_core::ApplicationCallbackExecCtx::Get(); if (!grpc_iomgr_is_any_background_poller_thread() && + !grpc_event_engine::posix_engine::TimerManager:: + IsTimerManagerThread() && (acec == nullptr || (acec->Flags() & GRPC_APP_CALLBACK_EXEC_CTX_FLAG_IS_INTERNAL_THREAD) == 0)) { diff --git a/test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.cc b/test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.cc index 04767976d94..18f20247b27 100644 --- a/test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.cc +++ b/test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.cc @@ -99,6 +99,9 @@ gpr_timespec FuzzingEventEngine::NowAsTimespec(gpr_clock_type clock_type) { } gpr_timespec FuzzingEventEngine::GlobalNowImpl(gpr_clock_type clock_type) { + if (g_fuzzing_event_engine == nullptr) { + return gpr_inf_future(clock_type); + } GPR_ASSERT(g_fuzzing_event_engine != nullptr); grpc_core::MutexLock lock(&g_fuzzing_event_engine->mu_); return g_fuzzing_event_engine->NowAsTimespec(clock_type); diff --git a/test/core/surface/init_test.cc b/test/core/surface/init_test.cc index 3e6ceb3060c..84db32e8ed3 100644 --- a/test/core/surface/init_test.cc +++ b/test/core/surface/init_test.cc @@ -20,10 +20,13 @@ #include +#include "absl/synchronization/notification.h" + #include #include #include +#include "src/core/lib/event_engine/default_event_engine.h" #include "src/core/lib/iomgr/exec_ctx.h" #include "test/core/util/test_config.h" @@ -137,6 +140,17 @@ TEST(Init, repeatedly_blocking) { EXPECT_FALSE(grpc_is_initialized()); } +TEST(Init, TimerManagerHoldsLastInit) { + grpc_init(); + absl::Notification n; + grpc_event_engine::experimental::GetDefaultEventEngine()->RunAfter( + std::chrono::seconds(1), [&n] { + grpc_shutdown(); + n.Notify(); + }); + n.WaitForNotification(); +} + int main(int argc, char** argv) { grpc::testing::TestEnvironment env(&argc, argv); ::testing::InitGoogleTest(&argc, argv);