From b581a24a4c273757d70cdeb70359d0ceb571c6bd Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 3 Oct 2023 10:46:31 -0700 Subject: [PATCH] [filter-test] Allow running grpc_init safely (#34567) Previously it turns out it was not safe to run grpc_init in a filter test - we'd end up mixing event engine implementations, and causing undefined behavior at grpc_shutdown. This change makes it safe and fixes a test internally that's flaking at 70% right now (b/302986486). --------- Co-authored-by: ctiller --- src/core/lib/iomgr/timer_manager.cc | 8 ++++- src/core/lib/iomgr/timer_manager.h | 2 ++ test/core/filters/client_auth_filter_test.cc | 5 +-- .../filters/client_authority_filter_test.cc | 7 +--- test/core/filters/filter_test.cc | 36 +++++++++++++------ test/core/filters/filter_test.h | 5 +-- test/core/filters/filter_test_test.cc | 5 +-- 7 files changed, 40 insertions(+), 28 deletions(-) diff --git a/src/core/lib/iomgr/timer_manager.cc b/src/core/lib/iomgr/timer_manager.cc index d7d8dfb53dd..408e6452bd5 100644 --- a/src/core/lib/iomgr/timer_manager.cc +++ b/src/core/lib/iomgr/timer_manager.cc @@ -41,6 +41,8 @@ extern grpc_core::TraceFlag grpc_timer_check_trace; static gpr_mu g_mu; // are we multi-threaded static bool g_threaded; +// should we start multi-threaded +static bool g_start_threaded = true; // cv to wait until a thread is needed static gpr_cv g_cv_wait; // cv for notification when threading ends @@ -309,7 +311,7 @@ void grpc_timer_manager_init(void) { g_has_timed_waiter = false; g_timed_waiter_deadline = grpc_core::Timestamp::InfFuture(); - start_threads(); + if (g_start_threaded) start_threads(); } static void stop_threads(void) { @@ -351,6 +353,10 @@ void grpc_timer_manager_set_threading(bool enabled) { } } +void grpc_timer_manager_set_start_threaded(bool enabled) { + g_start_threaded = enabled; +} + void grpc_kick_poller(void) { gpr_mu_lock(&g_mu); g_kicked = true; diff --git a/src/core/lib/iomgr/timer_manager.h b/src/core/lib/iomgr/timer_manager.h index 6dd4fd31b04..e5f0b120adb 100644 --- a/src/core/lib/iomgr/timer_manager.h +++ b/src/core/lib/iomgr/timer_manager.h @@ -32,6 +32,8 @@ void grpc_timer_manager_shutdown(void); // enable/disable threading - must be called after grpc_timer_manager_init and // before grpc_timer_manager_shutdown void grpc_timer_manager_set_threading(bool enabled); +// enable/disable threading - must be called before first grpc init +void grpc_timer_manager_set_start_threaded(bool enabled); // explicitly perform one tick of the timer system - for when threading is // disabled void grpc_timer_manager_tick(void); diff --git a/test/core/filters/client_auth_filter_test.cc b/test/core/filters/client_auth_filter_test.cc index 7e22c6d40a1..f8343bf8282 100644 --- a/test/core/filters/client_auth_filter_test.cc +++ b/test/core/filters/client_auth_filter_test.cc @@ -139,8 +139,5 @@ TEST_F(ClientAuthFilterTest, RewritesInvalidStatusFromCallCreds) { int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); - grpc_init(); - int retval = RUN_ALL_TESTS(); - grpc_shutdown(); - return retval; + return RUN_ALL_TESTS(); } diff --git a/test/core/filters/client_authority_filter_test.cc b/test/core/filters/client_authority_filter_test.cc index 8998b75cf7a..06a438fec91 100644 --- a/test/core/filters/client_authority_filter_test.cc +++ b/test/core/filters/client_authority_filter_test.cc @@ -71,10 +71,5 @@ TEST_F(ClientAuthorityFilterTest, int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); - // TODO(ctiller): promise_based_call currently demands to instantiate an event - // engine which needs grpc to be initialized. - grpc_init(); - int r = RUN_ALL_TESTS(); - grpc_shutdown(); - return r; + return RUN_ALL_TESTS(); } diff --git a/test/core/filters/filter_test.cc b/test/core/filters/filter_test.cc index 738545212aa..cb2ec168b80 100644 --- a/test/core/filters/filter_test.cc +++ b/test/core/filters/filter_test.cc @@ -15,6 +15,7 @@ #include "test/core/filters/filter_test.h" #include +#include #include #include @@ -24,8 +25,11 @@ #include "absl/types/optional.h" #include "gtest/gtest.h" +#include + #include "src/core/lib/channel/call_finalization.h" #include "src/core/lib/channel/context.h" +#include "src/core/lib/event_engine/default_event_engine.h" #include "src/core/lib/gprpp/crash.h" #include "src/core/lib/iomgr/timer_manager.h" #include "src/core/lib/promise/activity.h" @@ -38,6 +42,9 @@ #include "src/core/lib/slice/slice.h" #include "test/core/event_engine/fuzzing_event_engine/fuzzing_event_engine.pb.h" +using grpc_event_engine::experimental::FuzzingEventEngine; +using grpc_event_engine::experimental::GetDefaultEventEngine; + namespace grpc_core { /////////////////////////////////////////////////////////////////////////////// @@ -415,20 +422,27 @@ void FilterTestBase::Call::FinishNextFilter(ServerMetadataHandle md) { /////////////////////////////////////////////////////////////////////////////// // FilterTestBase -FilterTestBase::FilterTestBase() - : event_engine_( - []() { - grpc_timer_manager_set_threading(false); - grpc_event_engine::experimental::FuzzingEventEngine::Options - options; - return options; - }(), - fuzzing_event_engine::Actions()) {} +FilterTestBase::FilterTestBase() { + grpc_event_engine::experimental::SetEventEngineFactory([]() { + FuzzingEventEngine::Options options; + options.max_delay_run_after = std::chrono::milliseconds(500); + options.max_delay_write = std::chrono::milliseconds(50); + return std::make_unique( + options, fuzzing_event_engine::Actions()); + }); + event_engine_ = + std::dynamic_pointer_cast(GetDefaultEventEngine()); + grpc_timer_manager_set_start_threaded(false); + grpc_init(); +} -FilterTestBase::~FilterTestBase() { event_engine_.UnsetGlobalHooks(); } +FilterTestBase::~FilterTestBase() { + grpc_shutdown(); + event_engine_->UnsetGlobalHooks(); +} void FilterTestBase::Step() { - event_engine_.TickUntilIdle(); + event_engine_->TickUntilIdle(); ::testing::Mock::VerifyAndClearExpectations(&events); } diff --git a/test/core/filters/filter_test.h b/test/core/filters/filter_test.h index 74acd71b5fa..796b0a22213 100644 --- a/test/core/filters/filter_test.h +++ b/test/core/filters/filter_test.h @@ -206,13 +206,14 @@ class FilterTestBase : public ::testing::Test { ~FilterTestBase() override; grpc_event_engine::experimental::EventEngine* event_engine() { - return &event_engine_; + return event_engine_.get(); } void Step(); private: - grpc_event_engine::experimental::FuzzingEventEngine event_engine_; + std::shared_ptr + event_engine_; }; template diff --git a/test/core/filters/filter_test_test.cc b/test/core/filters/filter_test_test.cc index 8ffa907b429..030ba4914a6 100644 --- a/test/core/filters/filter_test_test.cc +++ b/test/core/filters/filter_test_test.cc @@ -246,8 +246,5 @@ TEST_F(NoOpFilterTest, CanProcessServerToClientMessage) { int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); - grpc_init(); - int r = RUN_ALL_TESTS(); - grpc_shutdown(); - return r; + return RUN_ALL_TESTS(); }