Skip to content

Commit

Permalink
[filter-test] Allow running grpc_init safely (grpc#34567)
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
ctiller and ctiller authored Oct 3, 2023
1 parent df1976b commit b581a24
Show file tree
Hide file tree
Showing 7 changed files with 40 additions and 28 deletions.
8 changes: 7 additions & 1 deletion src/core/lib/iomgr/timer_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions src/core/lib/iomgr/timer_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 1 addition & 4 deletions test/core/filters/client_auth_filter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
7 changes: 1 addition & 6 deletions test/core/filters/client_authority_filter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
36 changes: 25 additions & 11 deletions test/core/filters/filter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "test/core/filters/filter_test.h"

#include <algorithm>
#include <chrono>
#include <memory>
#include <queue>

Expand All @@ -24,8 +25,11 @@
#include "absl/types/optional.h"
#include "gtest/gtest.h"

#include <grpc/grpc.h>

#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"
Expand All @@ -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 {

///////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -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<FuzzingEventEngine>(
options, fuzzing_event_engine::Actions());
});
event_engine_ =
std::dynamic_pointer_cast<FuzzingEventEngine>(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);
}

Expand Down
5 changes: 3 additions & 2 deletions test/core/filters/filter_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<grpc_event_engine::experimental::FuzzingEventEngine>
event_engine_;
};

template <typename Filter>
Expand Down
5 changes: 1 addition & 4 deletions test/core/filters/filter_test_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

0 comments on commit b581a24

Please sign in to comment.