From 7a0f16e8055647d25e786f2463b41d15721a63af Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Tue, 8 Sep 2020 15:21:14 -0700 Subject: [PATCH] Revert "Implemented conditional shutdown" --- CMakeLists.txt | 71 ++++++++----------- build_autogenerated.yaml | 27 ++++--- src/core/lib/iomgr/exec_ctx.h | 18 +++-- src/core/lib/surface/init.cc | 35 +++------ test/core/surface/BUILD | 3 - test/core/surface/init_test.cc | 102 ++++++++------------------- tools/run_tests/generated/tests.json | 48 ++++++------- 7 files changed, 113 insertions(+), 191 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4b04d25ac50..f07899ee8b5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -606,6 +606,7 @@ if(gRPC_BUILD_TESTS) if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX) add_dependencies(buildtests_c httpscli_test) endif() + add_dependencies(buildtests_c init_test) add_dependencies(buildtests_c inproc_callback_test) add_dependencies(buildtests_c invalid_call_argument_test) add_dependencies(buildtests_c json_token_test) @@ -835,7 +836,6 @@ if(gRPC_BUILD_TESTS) add_dependencies(buildtests_cxx health_service_end2end_test) add_dependencies(buildtests_cxx http2_client) add_dependencies(buildtests_cxx hybrid_end2end_test) - add_dependencies(buildtests_cxx init_test) add_dependencies(buildtests_cxx initial_settings_frame_bad_client_test) add_dependencies(buildtests_cxx interop_client) add_dependencies(buildtests_cxx interop_server) @@ -5987,6 +5987,36 @@ endif() endif() if(gRPC_BUILD_TESTS) +add_executable(init_test + test/core/surface/init_test.cc +) + +target_include_directories(init_test + PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_CURRENT_SOURCE_DIR}/include + ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} + ${_gRPC_RE2_INCLUDE_DIR} + ${_gRPC_SSL_INCLUDE_DIR} + ${_gRPC_UPB_GENERATED_DIR} + ${_gRPC_UPB_GRPC_GENERATED_DIR} + ${_gRPC_UPB_INCLUDE_DIR} + ${_gRPC_ZLIB_INCLUDE_DIR} +) + +target_link_libraries(init_test + ${_gRPC_ALLTARGETS_LIBRARIES} + grpc_test_util + grpc + gpr + address_sorting + upb +) + + +endif() +if(gRPC_BUILD_TESTS) + add_executable(inproc_callback_test test/core/end2end/inproc_callback_test.cc ) @@ -11868,45 +11898,6 @@ target_link_libraries(hybrid_end2end_test ) -endif() -if(gRPC_BUILD_TESTS) - -add_executable(init_test - test/core/surface/init_test.cc - third_party/googletest/googletest/src/gtest-all.cc - third_party/googletest/googlemock/src/gmock-all.cc -) - -target_include_directories(init_test - PRIVATE - ${CMAKE_CURRENT_SOURCE_DIR} - ${CMAKE_CURRENT_SOURCE_DIR}/include - ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} - ${_gRPC_RE2_INCLUDE_DIR} - ${_gRPC_SSL_INCLUDE_DIR} - ${_gRPC_UPB_GENERATED_DIR} - ${_gRPC_UPB_GRPC_GENERATED_DIR} - ${_gRPC_UPB_INCLUDE_DIR} - ${_gRPC_ZLIB_INCLUDE_DIR} - third_party/googletest/googletest/include - third_party/googletest/googletest - third_party/googletest/googlemock/include - third_party/googletest/googlemock - ${_gRPC_PROTO_GENS_DIR} -) - -target_link_libraries(init_test - ${_gRPC_PROTOBUF_LIBRARIES} - ${_gRPC_ALLTARGETS_LIBRARIES} - grpc_test_util - grpc - gpr - address_sorting - upb - ${_gRPC_GFLAGS_LIBRARIES} -) - - endif() if(gRPC_BUILD_TESTS) diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 7ef16a33376..62be71458c8 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -3576,6 +3576,19 @@ targets: - linux - posix - mac +- name: init_test + build: test + language: c + headers: [] + src: + - test/core/surface/init_test.cc + deps: + - grpc_test_util + - grpc + - gpr + - address_sorting + - upb + uses_polling: false - name: inproc_callback_test build: test language: c @@ -6158,20 +6171,6 @@ targets: - gpr - address_sorting - upb -- name: init_test - gtest: true - build: test - language: c++ - headers: [] - src: - - test/core/surface/init_test.cc - deps: - - grpc_test_util - - grpc - - gpr - - address_sorting - - upb - uses_polling: false - name: initial_settings_frame_bad_client_test gtest: true build: test diff --git a/src/core/lib/iomgr/exec_ctx.h b/src/core/lib/iomgr/exec_ctx.h index 6b4e184cad3..0079946b4ab 100644 --- a/src/core/lib/iomgr/exec_ctx.h +++ b/src/core/lib/iomgr/exec_ctx.h @@ -331,15 +331,9 @@ class ApplicationCallbackExecCtx { } } - uintptr_t Flags() { return flags_; } - - static ApplicationCallbackExecCtx* Get() { - return reinterpret_cast( - gpr_tls_get(&callback_exec_ctx_)); - } - static void Set(ApplicationCallbackExecCtx* exec_ctx, uintptr_t flags) { - if (Get() == nullptr) { + if (reinterpret_cast( + gpr_tls_get(&callback_exec_ctx_)) == nullptr) { if (!(GRPC_APP_CALLBACK_EXEC_CTX_FLAG_IS_INTERNAL_THREAD & flags)) { grpc_core::Fork::IncExecCtxCount(); } @@ -352,7 +346,8 @@ class ApplicationCallbackExecCtx { functor->internal_success = is_success; functor->internal_next = nullptr; - ApplicationCallbackExecCtx* ctx = Get(); + auto* ctx = reinterpret_cast( + gpr_tls_get(&callback_exec_ctx_)); if (ctx->head_ == nullptr) { ctx->head_ = functor; @@ -369,7 +364,10 @@ class ApplicationCallbackExecCtx { /** Global shutdown for ApplicationCallbackExecCtx. Called by init. */ static void GlobalShutdown(void) { gpr_tls_destroy(&callback_exec_ctx_); } - static bool Available() { return Get() != nullptr; } + static bool Available() { + return reinterpret_cast( + gpr_tls_get(&callback_exec_ctx_)) != nullptr; + } private: uintptr_t flags_{0u}; diff --git a/src/core/lib/surface/init.cc b/src/core/lib/surface/init.cc index ff5b074ca34..88a69495c01 100644 --- a/src/core/lib/surface/init.cc +++ b/src/core/lib/surface/init.cc @@ -18,8 +18,6 @@ #include -#include "src/core/lib/surface/init.h" - #include #include @@ -28,7 +26,6 @@ #include #include #include - #include "src/core/lib/channel/channel_stack.h" #include "src/core/lib/channel/channelz_registry.h" #include "src/core/lib/channel/connected_channel.h" @@ -40,7 +37,6 @@ #include "src/core/lib/http/parser.h" #include "src/core/lib/iomgr/call_combiner.h" #include "src/core/lib/iomgr/combiner.h" -#include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/iomgr/executor.h" #include "src/core/lib/iomgr/iomgr.h" #include "src/core/lib/iomgr/resource_quota.h" @@ -51,6 +47,7 @@ #include "src/core/lib/surface/call.h" #include "src/core/lib/surface/channel_init.h" #include "src/core/lib/surface/completion_queue.h" +#include "src/core/lib/surface/init.h" #include "src/core/lib/surface/lame_client.h" #include "src/core/lib/surface/server.h" #include "src/core/lib/transport/bdp_estimator.h" @@ -214,29 +211,15 @@ void grpc_shutdown_internal(void* /*ignored*/) { void grpc_shutdown(void) { GRPC_API_TRACE("grpc_shutdown(void)", 0, ()); grpc_core::MutexLock lock(&g_init_mu); - if (--g_initializations == 0) { - grpc_core::ApplicationCallbackExecCtx* acec = - grpc_core::ApplicationCallbackExecCtx::Get(); - if (!grpc_iomgr_is_any_background_poller_thread() && - (acec == nullptr || - (acec->Flags() & GRPC_APP_CALLBACK_EXEC_CTX_FLAG_IS_INTERNAL_THREAD) == - 0)) { - // just run clean-up when this is called on non-executor thread. - gpr_log(GPR_DEBUG, "grpc_shutdown starts clean-up now"); - g_shutting_down = true; - grpc_shutdown_internal_locked(); - } else { - // spawn a detached thread to do the actual clean up in case we are - // currently in an executor thread. - gpr_log(GPR_DEBUG, "grpc_shutdown spawns clean-up thread"); - g_initializations++; - g_shutting_down = true; - grpc_core::Thread cleanup_thread( - "grpc_shutdown", grpc_shutdown_internal, nullptr, nullptr, - grpc_core::Thread::Options().set_joinable(false).set_tracked(false)); - cleanup_thread.Start(); - } + g_initializations++; + g_shutting_down = true; + // spawn a detached thread to do the actual clean up in case we are + // currently in an executor thread. + grpc_core::Thread cleanup_thread( + "grpc_shutdown", grpc_shutdown_internal, nullptr, nullptr, + grpc_core::Thread::Options().set_joinable(false).set_tracked(false)); + cleanup_thread.Start(); } } diff --git a/test/core/surface/BUILD b/test/core/surface/BUILD index 39a86cc5c77..91f42cca637 100644 --- a/test/core/surface/BUILD +++ b/test/core/surface/BUILD @@ -79,9 +79,6 @@ grpc_cc_test( grpc_cc_test( name = "init_test", srcs = ["init_test.cc"], - external_deps = [ - "gtest", - ], language = "C++", uses_polling = False, deps = [ diff --git a/test/core/surface/init_test.cc b/test/core/surface/init_test.cc index 4862e3af9f8..0bed855970d 100644 --- a/test/core/surface/init_test.cc +++ b/test/core/surface/init_test.cc @@ -16,22 +16,14 @@ * */ -#include "src/core/lib/surface/init.h" - #include #include #include -#include -#include "src/core/lib/iomgr/exec_ctx.h" +#include "src/core/lib/surface/init.h" #include "test/core/util/test_config.h" -static int g_plugin_state; - -static void plugin_init(void) { g_plugin_state = 1; } -static void plugin_destroy(void) { g_plugin_state = 2; } -static bool plugin_is_intialized(void) { return g_plugin_state == 1; } -static bool plugin_is_destroyed(void) { return g_plugin_state == 2; } +static int g_flag; static void test(int rounds) { int i; @@ -41,13 +33,7 @@ static void test(int rounds) { for (i = 0; i < rounds; i++) { grpc_shutdown(); } - EXPECT_FALSE(grpc_is_initialized()); -} - -TEST(Init, test) { - test(1); - test(2); - test(3); + grpc_maybe_wait_for_async_shutdown(); } static void test_blocking(int rounds) { @@ -58,87 +44,55 @@ static void test_blocking(int rounds) { for (i = 0; i < rounds; i++) { grpc_shutdown_blocking(); } - EXPECT_FALSE(grpc_is_initialized()); -} - -TEST(Init, blocking) { - test_blocking(1); - test_blocking(2); - test_blocking(3); -} - -TEST(Init, shutdown_with_thread) { - grpc_init(); - { - grpc_core::ApplicationCallbackExecCtx callback_exec_ctx( - GRPC_APP_CALLBACK_EXEC_CTX_FLAG_IS_INTERNAL_THREAD); - grpc_shutdown(); - } - grpc_maybe_wait_for_async_shutdown(); - EXPECT_FALSE(grpc_is_initialized()); } -TEST(Init, mixed) { +static void test_mixed(void) { grpc_init(); grpc_init(); grpc_shutdown(); grpc_init(); grpc_shutdown(); grpc_shutdown(); - EXPECT_FALSE(grpc_is_initialized()); -} - -TEST(Init, mixed_with_thread) { - grpc_init(); - { - grpc_core::ApplicationCallbackExecCtx callback_exec_ctx( - GRPC_APP_CALLBACK_EXEC_CTX_FLAG_IS_INTERNAL_THREAD); - grpc_init(); - grpc_shutdown(); - grpc_init(); - grpc_shutdown(); - grpc_shutdown(); - } grpc_maybe_wait_for_async_shutdown(); - EXPECT_FALSE(grpc_is_initialized()); } -TEST(Init, plugin) { +static void plugin_init(void) { g_flag = 1; } +static void plugin_destroy(void) { g_flag = 2; } + +static void test_plugin() { + grpc_register_plugin(plugin_init, plugin_destroy); grpc_init(); - EXPECT_TRUE(plugin_is_intialized()); + GPR_ASSERT(g_flag == 1); grpc_shutdown_blocking(); - EXPECT_TRUE(plugin_is_destroyed()); - EXPECT_FALSE(grpc_is_initialized()); + GPR_ASSERT(g_flag == 2); } -TEST(Init, repeatedly) { - for (int i = 0; i < 10; i++) { +static void test_repeatedly() { + for (int i = 0; i < 1000; i++) { grpc_init(); - { - grpc_core::ApplicationCallbackExecCtx callback_exec_ctx( - GRPC_APP_CALLBACK_EXEC_CTX_FLAG_IS_INTERNAL_THREAD); - grpc_shutdown(); - } + grpc_shutdown(); } grpc_maybe_wait_for_async_shutdown(); - EXPECT_FALSE(grpc_is_initialized()); } -TEST(Init, repeatedly_blocking) { - for (int i = 0; i < 10; i++) { +static void test_repeatedly_blocking() { + for (int i = 0; i < 1000; i++) { grpc_init(); - { - grpc_core::ApplicationCallbackExecCtx callback_exec_ctx( - GRPC_APP_CALLBACK_EXEC_CTX_FLAG_IS_INTERNAL_THREAD); - grpc_shutdown_blocking(); - } + grpc_shutdown_blocking(); } - EXPECT_FALSE(grpc_is_initialized()); } int main(int argc, char** argv) { grpc::testing::TestEnvironment env(argc, argv); - ::testing::InitGoogleTest(&argc, argv); - grpc_register_plugin(plugin_init, plugin_destroy); - return RUN_ALL_TESTS(); + test(1); + test(2); + test(3); + test_blocking(1); + test_blocking(2); + test_blocking(3); + test_mixed(); + test_plugin(); + test_repeatedly(); + test_repeatedly_blocking(); + return 0; } diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index 9a60821042a..1da86dbc749 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -1607,6 +1607,30 @@ ], "uses_polling": true }, + { + "args": [], + "benchmark": false, + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": false, + "language": "c", + "name": "init_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "uses_polling": false + }, { "args": [], "benchmark": false, @@ -4645,30 +4669,6 @@ ], "uses_polling": true }, - { - "args": [], - "benchmark": false, - "ci_platforms": [ - "linux", - "mac", - "posix", - "windows" - ], - "cpu_cost": 1.0, - "exclude_configs": [], - "exclude_iomgrs": [], - "flaky": false, - "gtest": true, - "language": "c++", - "name": "init_test", - "platforms": [ - "linux", - "mac", - "posix", - "windows" - ], - "uses_polling": false - }, { "args": [], "benchmark": false,