Skip to content

Commit

Permalink
Reviewer comments
Browse files Browse the repository at this point in the history
  • Loading branch information
yashykt committed Jan 28, 2025
1 parent eb6d642 commit b758bde
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 20 deletions.
21 changes: 2 additions & 19 deletions test/cpp/end2end/xds/xds_end2end_test_lib.cc
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,8 @@ const char XdsEnd2endTest::kRequestMessage[] = "Live long and prosper.";

XdsEnd2endTest::XdsEnd2endTest(
std::shared_ptr<ServerCredentials> balancer_credentials)
: balancer_(CreateAndStartBalancer("Default Balancer",
: event_engine_scope_(grpc_event_engine::experimental::CreateEventEngine()),
balancer_(CreateAndStartBalancer("Default Balancer",
std::move(balancer_credentials))) {
// Initialize default client-side xDS resources.
default_listener_ = XdsResourceUtils::DefaultListener();
Expand All @@ -399,17 +400,6 @@ XdsEnd2endTest::XdsEnd2endTest(
default_server_listener_ = XdsResourceUtils::DefaultServerListener();
}

XdsEnd2endTest::~XdsEnd2endTest() {
// We need to make sure that all work from the current test is done before the
// next test starts. Without this, we run into errors such as
// https://github.com/grpc/grpc/issues/38588. Note that we are using all refs
// to current event engine going down to 0 as a proxy for checking that all
// work from the current test is done, but this is not a guaranteed way
// since there might still be threads that are not using event engine but are
// still accessing/modifying the system state.
grpc_event_engine::experimental::ShutdownDefaultEventEngine();
}

void XdsEnd2endTest::TearDown() {
ShutdownAllBackends();
balancer_->Shutdown();
Expand All @@ -422,13 +412,6 @@ void XdsEnd2endTest::TearDown() {
remove(bootstrap_file_);
gpr_free(bootstrap_file_);
}
// Destruct members so that we can make sure no EventEngine ref is being held
backends_.clear();
stub2_.reset();
stub1_.reset();
stub_.reset();
channel_.reset();
balancer_.reset();
}

std::unique_ptr<XdsEnd2endTest::BalancerServerThread>
Expand Down
14 changes: 13 additions & 1 deletion test/cpp/end2end/xds/xds_end2end_test_lib.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include "envoy/config/rbac/v3/rbac.pb.h"
#include "envoy/extensions/filters/http/rbac/v3/rbac.pb.h"
#include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h"
#include "src/core/lib/event_engine/default_event_engine.h"
#include "src/core/lib/security/credentials/fake/fake_credentials.h"
#include "src/core/lib/security/security_connector/ssl_utils.h"
#include "src/cpp/server/secure_server_credentials.h"
Expand Down Expand Up @@ -450,7 +451,6 @@ class XdsEnd2endTest : public ::testing::TestWithParam<XdsTestType>,
// If balancer_credentials is null, it defaults to fake credentials.
explicit XdsEnd2endTest(
std::shared_ptr<ServerCredentials> balancer_credentials = nullptr);
~XdsEnd2endTest();

void SetUp() override { InitClient(); }
void TearDown() override;
Expand Down Expand Up @@ -1036,6 +1036,18 @@ class XdsEnd2endTest : public ::testing::TestWithParam<XdsTestType>,
static std::shared_ptr<ServerCredentials> CreateMtlsServerCredentials();
static std::shared_ptr<ServerCredentials> CreateTlsServerCredentials();

// event_engine_scope_ always has to be at the top of the list to make sure
// that all other objects are destroyed before this and other event engine
// refs are released.
// We are using DefaultEventEngineScope to make sure that all work from the
// current test is done before the next test starts. Without this, we run into
// errors such as https://github.com/grpc/grpc/issues/38588. Note that we are
// using all refs to current event engine going down to 0 as a proxy for
// checking that all work from the current test is done, but this is not a
// guaranteed way since there might still be threads that are not using event
// engine but are still accessing/modifying the system state.
grpc_event_engine::experimental::DefaultEventEngineScope event_engine_scope_;

std::unique_ptr<BalancerServerThread> balancer_;

std::shared_ptr<Channel> channel_;
Expand Down

0 comments on commit b758bde

Please sign in to comment.