From 87c42aab68c817e5a9240ce7fabf2cfc7362cd85 Mon Sep 17 00:00:00 2001 From: AJ Heller Date: Tue, 7 May 2024 11:43:06 -0700 Subject: [PATCH 1/2] [test] Wait for EE to quiesce in RLSEnd2endTest teardown (#36553) Closes #36553 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36553 from drfloob:cleaner-RlsEnd2endTest-shutdown f16b5aac3e5f73154a7944791de61dc050f05ec3 PiperOrigin-RevId: 631496415 --- CMakeLists.txt | 1 + build_autogenerated.yaml | 2 ++ test/cpp/end2end/BUILD | 1 + test/cpp/end2end/rls_end2end_test.cc | 3 +++ 4 files changed, 7 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index c6e292b0373..787db67baa0 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -26337,6 +26337,7 @@ add_executable(rls_end2end_test ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/orca_load_report.grpc.pb.cc ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/orca_load_report.pb.h ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/testing/xds/v3/orca_load_report.grpc.pb.h + test/core/event_engine/event_engine_test_utils.cc test/core/test_util/fake_stats_plugin.cc test/core/test_util/test_lb_policies.cc test/cpp/end2end/rls_end2end_test.cc diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 7e4f95f2932..dd845a6d959 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -17596,6 +17596,7 @@ targets: run: false language: c++ headers: + - test/core/event_engine/event_engine_test_utils.h - test/core/test_util/fake_stats_plugin.h - test/core/test_util/test_lb_policies.h - test/cpp/end2end/counted_service.h @@ -17608,6 +17609,7 @@ targets: - src/proto/grpc/testing/echo_messages.proto - src/proto/grpc/testing/simple_messages.proto - src/proto/grpc/testing/xds/v3/orca_load_report.proto + - test/core/event_engine/event_engine_test_utils.cc - test/core/test_util/fake_stats_plugin.cc - test/core/test_util/test_lb_policies.cc - test/cpp/end2end/rls_end2end_test.cc diff --git a/test/cpp/end2end/BUILD b/test/cpp/end2end/BUILD index 4e0b8ed3e55..3e01fe662f9 100644 --- a/test/cpp/end2end/BUILD +++ b/test/cpp/end2end/BUILD @@ -584,6 +584,7 @@ grpc_cc_test( "//src/proto/grpc/testing:echo_messages_proto", "//src/proto/grpc/testing:echo_proto", "//src/proto/grpc/testing/duplicate:echo_duplicate_proto", + "//test/core/event_engine:event_engine_test_utils", "//test/core/test_util:fake_stats_plugin", "//test/core/test_util:grpc_test_util", "//test/core/test_util:test_lb_policies", diff --git a/test/cpp/end2end/rls_end2end_test.cc b/test/cpp/end2end/rls_end2end_test.cc index 5eb5e41ac3e..189c4c2d34c 100644 --- a/test/cpp/end2end/rls_end2end_test.cc +++ b/test/cpp/end2end/rls_end2end_test.cc @@ -58,6 +58,7 @@ #include "src/proto/grpc/lookup/v1/rls.grpc.pb.h" #include "src/proto/grpc/lookup/v1/rls.pb.h" #include "src/proto/grpc/testing/echo.grpc.pb.h" +#include "test/core/event_engine/event_engine_test_utils.h" #include "test/core/test_util/fake_stats_plugin.h" #include "test/core/test_util/port.h" #include "test/core/test_util/resolve_localhost_ip46.h" @@ -175,6 +176,8 @@ class RlsEnd2endTest : public ::testing::Test { static void TearDownTestSuite() { grpc_shutdown_blocking(); + WaitForSingleOwner( + grpc_event_engine::experimental::GetDefaultEventEngine()); grpc_core::CoreConfiguration::Reset(); } From 641dbfd4c24be9262b1b49a798e7f8116819d820 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 7 May 2024 14:36:28 -0700 Subject: [PATCH 2/2] [build] Add an assertion for no leaks, use it for event engine shutdown (#36520) Add a test utility to assert there are no memory leaks at an arbitrary point in code (when running under ASAN). Use that utility to help diagnose why event engine fails to shut down. .. also moves some preprocessor junk around in build.cc to make that file a little easier to parse Closes #36520 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36520 from ctiller:leaky-boat 540bfca8c6ead54436ea215bd0afec454329f610 PiperOrigin-RevId: 631552535 --- .../event_engine/event_engine_test_utils.cc | 4 ++ test/core/test_util/build.cc | 70 ++++++++++++------- test/core/test_util/build.h | 3 + 3 files changed, 50 insertions(+), 27 deletions(-) diff --git a/test/core/event_engine/event_engine_test_utils.cc b/test/core/event_engine/event_engine_test_utils.cc index cc036ad5d3b..32ab6ac5276 100644 --- a/test/core/event_engine/event_engine_test_utils.cc +++ b/test/core/event_engine/event_engine_test_utils.cc @@ -41,6 +41,7 @@ #include "src/core/lib/gprpp/notification.h" #include "src/core/lib/gprpp/time.h" #include "src/core/lib/resource_quota/memory_quota.h" +#include "test/core/test_util/build.h" // IWYU pragma: no_include @@ -77,7 +78,10 @@ std::string GetNextSendMessage() { } void WaitForSingleOwner(std::shared_ptr engine) { + int n = 0; while (engine.use_count() > 1) { + ++n; + if (n % 100 == 0) AsanAssertNoLeaks(); GRPC_LOG_EVERY_N_SEC(2, GPR_INFO, "engine.use_count() = %ld", engine.use_count()); absl::SleepFor(absl::Milliseconds(100)); diff --git a/test/core/test_util/build.cc b/test/core/test_util/build.cc index 04d39e9574d..36510696681 100644 --- a/test/core/test_util/build.cc +++ b/test/core/test_util/build.cc @@ -12,62 +12,78 @@ // See the License for the specific language governing permissions and // limitations under the License. -bool BuiltUnderValgrind() { -#ifdef RUNNING_ON_VALGRIND - return true; -#else - return false; -#endif -} - -bool BuiltUnderTsan() { +// Define GRPC_BUILD_HAS_ASAN as 1 or 0 depending on if we're building under +// ASAN. #if defined(__has_feature) -#if __has_feature(thread_sanitizer) - return true; +#if __has_feature(address_sanitizer) +#define GRPC_BUILD_HAS_ASAN 1 #else - return false; +#define GRPC_BUILD_HAS_ASAN 0 #endif #else -#ifdef THREAD_SANITIZER - return true; +#ifdef ADDRESS_SANITIZER +#define GRPC_BUILD_HAS_ASAN 1 #else - return false; +#define GRPC_BUILD_HAS_ASAN 0 #endif #endif -} -bool BuiltUnderAsan() { +// Define GRPC_BUILD_HAS_TSAN as 1 or 0 depending on if we're building under +// TSAN. #if defined(__has_feature) -#if __has_feature(address_sanitizer) - return true; +#if __has_feature(thread_sanitizer) +#define GRPC_BUILD_HAS_TSAN 1 #else - return false; +#define GRPC_BUILD_HAS_TSAN 0 #endif #else -#ifdef ADDRESS_SANITIZER - return true; +#ifdef THREAD_SANITIZER +#define GRPC_BUILD_HAS_TSAN 1 #else - return false; +#define GRPC_BUILD_HAS_TSAN 0 #endif #endif -} -bool BuiltUnderMsan() { +// Define GRPC_BUILD_HAS_MSAN as 1 or 0 depending on if we're building under +// MSAN. #if defined(__has_feature) #if __has_feature(memory_sanitizer) - return true; +#define GRPC_BUILD_HAS_MSAN 1 #else - return false; +#define GRPC_BUILD_HAS_MSAN 0 #endif #else #ifdef MEMORY_SANITIZER +#define GRPC_BUILD_HAS_MSAN 1 +#else +#define GRPC_BUILD_HAS_MSAN 0 +#endif +#endif + +#if GRPC_BUILD_HAS_ASAN +#include +#endif + +bool BuiltUnderValgrind() { +#ifdef RUNNING_ON_VALGRIND return true; #else return false; #endif +} + +bool BuiltUnderTsan() { return GRPC_BUILD_HAS_TSAN != 0; } + +bool BuiltUnderAsan() { return GRPC_BUILD_HAS_ASAN != 0; } + +void AsanAssertNoLeaks() { +#if GRPC_BUILD_HAS_ASAN + __lsan_do_leak_check(); #endif } +bool BuiltUnderMsan() { return GRPC_BUILD_HAS_MSAN != 0; } + bool BuiltUnderUbsan() { #ifdef GRPC_UBSAN return true; diff --git a/test/core/test_util/build.h b/test/core/test_util/build.h index 20bc21954ff..8574dfe8d3c 100644 --- a/test/core/test_util/build.h +++ b/test/core/test_util/build.h @@ -30,4 +30,7 @@ bool BuiltUnderMsan(); // Returns whether this is built under UndefinedBehaviorSanitizer bool BuiltUnderUbsan(); +// Force a leak check if built under ASAN. If there are leaks, crash. +void AsanAssertNoLeaks(); + #endif // GRPC_TEST_CORE_TEST_UTIL_BUILD_H