Revert "Implemented conditional shutdown"

4 years ago · 7a0f16e805
parent 0c1ca7b71d
commit 7a0f16e805
7 changed files with 113 additions and 191 deletions
--- 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)

--- 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
--- 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<ApplicationCallbackExecCtx*>(
-        gpr_tls_get(&callback_exec_ctx_));
-  }
-
  static void Set(ApplicationCallbackExecCtx* exec_ctx, uintptr_t flags) {
-    if (Get() == nullptr) {
+    if (reinterpret_cast<ApplicationCallbackExecCtx*>(
+            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<ApplicationCallbackExecCtx*>(
+        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<ApplicationCallbackExecCtx*>(
+               gpr_tls_get(&callback_exec_ctx_)) != nullptr;
+  }

 private:
  uintptr_t flags_{0u};
--- a/src/core/lib/surface/init.cc
+++ b/src/core/lib/surface/init.cc
@ -18,8 +18,6 @@

 #include <grpc/support/port_platform.h>

-#include "src/core/lib/surface/init.h"
-
 #include <limits.h>
 #include <memory.h>

@ -28,7 +26,6 @@
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>
 #include <grpc/support/time.h>
-
 #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();
  }
 }

--- 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 = [
--- 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 <grpc/grpc.h>
 #include <grpc/support/log.h>
 #include <grpc/support/time.h>
-#include <gtest/gtest.h>

-#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;
 }
--- 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,