Merge pull request #17978 from yashykt/interceptorcleanup1

Global Interceptor Registration allowed only once

include/grpcpp/impl/codegen/client_interceptor.h

@@ -172,17 +172,16 @@ class ClientRpcInfo {
 // PLEASE DO NOT USE THIS. ALWAYS PREFER PER CHANNEL INTERCEPTORS OVER A GLOBAL
 // INTERCEPTOR. IF USAGE IS ABSOLUTELY NECESSARY, PLEASE READ THE SAFETY NOTES.
 // Registers a global client interceptor factory object, which is used for all
-// RPCs made in this process.  If the argument is nullptr, the global
+// RPCs made in this process. The application is responsible for maintaining the
-// interceptor factory is deregistered. The application is responsible for
+// life of the object while gRPC operations are in progress. The global
-// maintaining the life of the object while gRPC operations are in progress. It
+// interceptor factory should only be registered once at the start of the
-// is unsafe to try to register/deregister if any gRPC operation is in progress.
+// process before any gRPC operations have begun.
-// For safety, it is in the best interests of the developer to register the
-// global interceptor factory once at the start of the process before any gRPC
-// operations have begun. Deregistration is optional since gRPC does not
-// maintain any references to the object.
 void RegisterGlobalClientInterceptorFactory(
     ClientInterceptorFactoryInterface* factory);
 
+// For testing purposes only
+void TestOnlyResetGlobalClientInterceptorFactory();
+
 }  // namespace experimental
 }  // namespace grpc
 

src/cpp/client/client_interceptor.cc

@@ -28,7 +28,17 @@ experimental::ClientInterceptorFactoryInterface*
 namespace experimental {
 void RegisterGlobalClientInterceptorFactory(
     ClientInterceptorFactoryInterface* factory) {
+  if (internal::g_global_client_interceptor_factory != nullptr) {
+    GPR_ASSERT(false &&
+               "It is illegal to call RegisterGlobalClientInterceptorFactory "
+               "multiple times.");
+  }
   internal::g_global_client_interceptor_factory = factory;
 }
+
+// For testing purposes only.
+void TestOnlyResetGlobalClientInterceptorFactory() {
+  internal::g_global_client_interceptor_factory = nullptr;
+}
 }  // namespace experimental
 }  // namespace grpc

test/cpp/end2end/client_interceptors_end2end_test.cc

@@ -894,9 +894,7 @@ TEST_F(ClientGlobalInterceptorEnd2endTest, DummyGlobalInterceptor) {
   MakeCall(channel);
   // Make sure all 20 dummy interceptors were run with the global interceptor
   EXPECT_EQ(DummyInterceptor::GetNumTimesRun(), 21);
-  // Reset the global interceptor. This is again 'safe' because there are no
+  experimental::TestOnlyResetGlobalClientInterceptorFactory();
-  // other ongoing gRPC operations
-  experimental::RegisterGlobalClientInterceptorFactory(nullptr);
 }
 
 TEST_F(ClientGlobalInterceptorEnd2endTest, LoggingGlobalInterceptor) {
@@ -920,9 +918,7 @@ TEST_F(ClientGlobalInterceptorEnd2endTest, LoggingGlobalInterceptor) {
   MakeCall(channel);
   // Make sure all 20 dummy interceptors were run
   EXPECT_EQ(DummyInterceptor::GetNumTimesRun(), 20);
-  // Reset the global interceptor. This is again 'safe' because there are no
+  experimental::TestOnlyResetGlobalClientInterceptorFactory();
-  // other ongoing gRPC operations
-  experimental::RegisterGlobalClientInterceptorFactory(nullptr);
 }
 
 TEST_F(ClientGlobalInterceptorEnd2endTest, HijackingGlobalInterceptor) {
@@ -946,9 +942,7 @@ TEST_F(ClientGlobalInterceptorEnd2endTest, HijackingGlobalInterceptor) {
   MakeCall(channel);
   // Make sure all 20 dummy interceptors were run
   EXPECT_EQ(DummyInterceptor::GetNumTimesRun(), 20);
-  // Reset the global interceptor. This is again 'safe' because there are no
+  experimental::TestOnlyResetGlobalClientInterceptorFactory();
-  // other ongoing gRPC operations
-  experimental::RegisterGlobalClientInterceptorFactory(nullptr);
 }
 
 }  // namespace