From 52ec54bea41fa98bba34b9f43a76b2d4bce662fe Mon Sep 17 00:00:00 2001 From: Yousuk Seung Date: Wed, 18 Sep 2024 18:07:27 -0700 Subject: [PATCH 1/2] [ssl] Ensure OPENSSL shuts down after gRPC Ensure OPENSSL global clean up happens after gRPC shutdown completes. OPENSSL registers an exit handler to clean up global objects, which may happen before gRPC removes all references to OPENSSL. --- BUILD | 2 ++ src/core/lib/surface/init.cc | 18 ++++++++++++ src/core/lib/surface/init.h | 5 ++++ src/core/tsi/ssl_transport_security.cc | 9 ++++++ test/core/surface/init_test.cc | 38 ++++++++++++++++++++++++++ 5 files changed, 72 insertions(+) diff --git a/BUILD b/BUILD index c8c1d72af9d86..954d7e4ef6894 100644 --- a/BUILD +++ b/BUILD @@ -571,6 +571,7 @@ grpc_cc_library( external_deps = [ "absl/base:core_headers", "absl/log:log", + "absl/time:time", ], language = "c++", public_hdrs = GRPC_PUBLIC_HDRS, @@ -642,6 +643,7 @@ grpc_cc_library( external_deps = [ "absl/base:core_headers", "absl/log:log", + "absl/time:time", ], language = "c++", public_hdrs = GRPC_PUBLIC_HDRS, diff --git a/src/core/lib/surface/init.cc b/src/core/lib/surface/init.cc index 0adcbb399954a..360766b0673c9 100644 --- a/src/core/lib/surface/init.cc +++ b/src/core/lib/surface/init.cc @@ -20,6 +20,8 @@ #include "absl/base/thread_annotations.h" #include "absl/log/log.h" +#include "absl/time/clock.h" +#include "absl/time/time.h" #include #include @@ -216,3 +218,19 @@ void grpc_maybe_wait_for_async_shutdown(void) { g_shutting_down_cv->Wait(g_init_mu); } } + +bool grpc_wait_for_shutdown_with_timeout(absl::Duration timeout) { + GRPC_TRACE_LOG(api, INFO) << "grpc_wait_for_shutdown_with_timeout()"; + const auto started = absl::Now(); + gpr_once_init(&g_basic_init, do_basic_init); + grpc_core::MutexLock lock(g_init_mu); + while (g_initializations != 0) { + if (g_shutting_down_cv->WaitWithTimeout(g_init_mu, timeout)) { + LOG(ERROR) << "grpc_wait_for_shutdown_with_timeout() timed out."; + return false; + } + } + LOG(INFO) << "grpc_wait_for_shutdown_with_timeout() took " + << absl::Now() - started; + return true; +} diff --git a/src/core/lib/surface/init.h b/src/core/lib/surface/init.h index 0d0035ae30474..9f9a4a468807f 100644 --- a/src/core/lib/surface/init.h +++ b/src/core/lib/surface/init.h @@ -20,6 +20,11 @@ #include +#include "absl/time/time.h" + void grpc_maybe_wait_for_async_shutdown(void); +// Returns false if the timeout expired before fully shut down. +bool grpc_wait_for_shutdown_with_timeout(absl::Duration timeout); + #endif // GRPC_SRC_CORE_LIB_SURFACE_INIT_H diff --git a/src/core/tsi/ssl_transport_security.cc b/src/core/tsi/ssl_transport_security.cc index 3149eb283fedb..d658b39a2f3c2 100644 --- a/src/core/tsi/ssl_transport_security.cc +++ b/src/core/tsi/ssl_transport_security.cc @@ -18,11 +18,13 @@ #include "src/core/tsi/ssl_transport_security.h" +#include #include #include #include +#include "src/core/lib/surface/init.h" #include "src/core/tsi/transport_security_interface.h" // TODO(jboeuf): refactor inet_ntop into a portability header. @@ -189,6 +191,13 @@ static void verified_root_cert_free(void* /*parent*/, void* ptr, static void init_openssl(void) { #if OPENSSL_VERSION_NUMBER >= 0x10100000 OPENSSL_init_ssl(0, nullptr); + // Ensure OPENSSL global clean up happens after gRPC shutdown completes. + // OPENSSL registers an exit handler to clean up global objects, which + // otherwise may happen before gRPC removes all references to OPENSSL. Below + // exit handler is guaranteed to run after OPENSSL's. + std::atexit([](){ + grpc_wait_for_shutdown_with_timeout(absl::Seconds(2)); + }); #else SSL_library_init(); SSL_load_error_strings(); diff --git a/test/core/surface/init_test.cc b/test/core/surface/init_test.cc index fe0ca1e57e3ab..0410731403584 100644 --- a/test/core/surface/init_test.cc +++ b/test/core/surface/init_test.cc @@ -29,6 +29,7 @@ #include #include "src/core/lib/event_engine/default_event_engine.h" +#include "src/core/lib/gprpp/thd.h" #include "src/core/lib/iomgr/exec_ctx.h" #include "test/core/test_util/test_config.h" @@ -115,6 +116,43 @@ TEST(Init, Repeatedly) { EXPECT_FALSE(grpc_is_initialized()); } + +TEST(Init, WaitForShutdownBeforeInit) { + EXPECT_TRUE(grpc_wait_for_shutdown_with_timeout(absl::ZeroDuration())); +} + +TEST(Init, WaitForShutdownAfterShutdown) { + grpc_init(); + grpc_shutdown(); + EXPECT_TRUE(grpc_wait_for_shutdown_with_timeout(absl::ZeroDuration())); +} + +TEST(Init, WaitForShutdownWithTimeout) { + grpc_init(); + grpc_init(); + grpc_shutdown(); + grpc_core::Thread t0( + "init_test", + [](void*) { + EXPECT_FALSE( + grpc_wait_for_shutdown_with_timeout(absl::Seconds(0.5))); + }, + nullptr); + grpc_core::Thread t1( + "init_test", + [](void*) { + EXPECT_TRUE( + grpc_wait_for_shutdown_with_timeout(absl::Seconds(1.5))); + }, + nullptr); + t0.Start(); + t1.Start(); + absl::SleepFor(absl::Seconds(1)); + grpc_shutdown(); + t0.Join(); + t1.Join(); +} + TEST(Init, RepeatedlyBlocking) { for (int i = 0; i < 10; i++) { grpc_init(); From 13967ec5b8d2aee444a2fe2bc29fd9100eae5d28 Mon Sep 17 00:00:00 2001 From: yousukseung <85715732+yousukseung@users.noreply.github.com> Date: Thu, 19 Sep 2024 01:21:38 +0000 Subject: [PATCH 2/2] Automated change: Fix sanity tests --- src/core/lib/surface/init.h | 4 ++-- src/core/tsi/ssl_transport_security.cc | 7 +++---- test/core/surface/init_test.cc | 7 ++----- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/core/lib/surface/init.h b/src/core/lib/surface/init.h index 9f9a4a468807f..804b79c5d5b31 100644 --- a/src/core/lib/surface/init.h +++ b/src/core/lib/surface/init.h @@ -18,10 +18,10 @@ #ifndef GRPC_SRC_CORE_LIB_SURFACE_INIT_H #define GRPC_SRC_CORE_LIB_SURFACE_INIT_H -#include - #include "absl/time/time.h" +#include + void grpc_maybe_wait_for_async_shutdown(void); // Returns false if the timeout expired before fully shut down. diff --git a/src/core/tsi/ssl_transport_security.cc b/src/core/tsi/ssl_transport_security.cc index d658b39a2f3c2..501f309082dc1 100644 --- a/src/core/tsi/ssl_transport_security.cc +++ b/src/core/tsi/ssl_transport_security.cc @@ -18,10 +18,11 @@ #include "src/core/tsi/ssl_transport_security.h" -#include #include #include +#include + #include #include "src/core/lib/surface/init.h" @@ -195,9 +196,7 @@ static void init_openssl(void) { // OPENSSL registers an exit handler to clean up global objects, which // otherwise may happen before gRPC removes all references to OPENSSL. Below // exit handler is guaranteed to run after OPENSSL's. - std::atexit([](){ - grpc_wait_for_shutdown_with_timeout(absl::Seconds(2)); - }); + std::atexit([]() { grpc_wait_for_shutdown_with_timeout(absl::Seconds(2)); }); #else SSL_library_init(); SSL_load_error_strings(); diff --git a/test/core/surface/init_test.cc b/test/core/surface/init_test.cc index 0410731403584..c8a09cc151d54 100644 --- a/test/core/surface/init_test.cc +++ b/test/core/surface/init_test.cc @@ -116,7 +116,6 @@ TEST(Init, Repeatedly) { EXPECT_FALSE(grpc_is_initialized()); } - TEST(Init, WaitForShutdownBeforeInit) { EXPECT_TRUE(grpc_wait_for_shutdown_with_timeout(absl::ZeroDuration())); } @@ -134,15 +133,13 @@ TEST(Init, WaitForShutdownWithTimeout) { grpc_core::Thread t0( "init_test", [](void*) { - EXPECT_FALSE( - grpc_wait_for_shutdown_with_timeout(absl::Seconds(0.5))); + EXPECT_FALSE(grpc_wait_for_shutdown_with_timeout(absl::Seconds(0.5))); }, nullptr); grpc_core::Thread t1( "init_test", [](void*) { - EXPECT_TRUE( - grpc_wait_for_shutdown_with_timeout(absl::Seconds(1.5))); + EXPECT_TRUE(grpc_wait_for_shutdown_with_timeout(absl::Seconds(1.5))); }, nullptr); t0.Start();