From c71b2f4fb71d2b9c8a0a2f1efa56b26a9bc8fac7 Mon Sep 17 00:00:00 2001
From: Yash Tibrewal <yashkt@google.com>
Date: Thu, 7 Feb 2019 19:36:31 -0800
Subject: [PATCH] Global Interceptor Registration allowed only once

---
 include/grpcpp/impl/codegen/client_interceptor.h     | 12 ++++--------
 src/cpp/client/client_interceptor.cc                 |  5 +++++
 test/cpp/end2end/client_interceptors_end2end_test.cc |  9 ---------
 3 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/include/grpcpp/impl/codegen/client_interceptor.h b/include/grpcpp/impl/codegen/client_interceptor.h
index 7dfe2290a3f..defbeabfb63 100644
--- a/include/grpcpp/impl/codegen/client_interceptor.h
+++ b/include/grpcpp/impl/codegen/client_interceptor.h
@@ -172,14 +172,10 @@ 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
-// interceptor factory is deregistered. The application is responsible for
-// maintaining the life of the object while gRPC operations are in progress. It
-// is unsafe to try to register/deregister if any gRPC operation is in progress.
-// 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.
+// RPCs made in this process. The application is responsible for maintaining the
+// life of the object while gRPC operations are in progress. The global
+// interceptor factory should only be registered once at the start of the
+// process before any gRPC operations have begun.
 void RegisterGlobalClientInterceptorFactory(
     ClientInterceptorFactoryInterface* factory);
 
diff --git a/src/cpp/client/client_interceptor.cc b/src/cpp/client/client_interceptor.cc
index 3a5cac9830f..15ab89c5e67 100644
--- a/src/cpp/client/client_interceptor.cc
+++ b/src/cpp/client/client_interceptor.cc
@@ -28,6 +28,11 @@ 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;
 }
 }  // namespace experimental
diff --git a/test/cpp/end2end/client_interceptors_end2end_test.cc b/test/cpp/end2end/client_interceptors_end2end_test.cc
index 177922f4576..d9099d722b2 100644
--- a/test/cpp/end2end/client_interceptors_end2end_test.cc
+++ b/test/cpp/end2end/client_interceptors_end2end_test.cc
@@ -894,9 +894,6 @@ 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
-  // other ongoing gRPC operations
-  experimental::RegisterGlobalClientInterceptorFactory(nullptr);
 }
 
 TEST_F(ClientGlobalInterceptorEnd2endTest, LoggingGlobalInterceptor) {
@@ -920,9 +917,6 @@ 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
-  // other ongoing gRPC operations
-  experimental::RegisterGlobalClientInterceptorFactory(nullptr);
 }
 
 TEST_F(ClientGlobalInterceptorEnd2endTest, HijackingGlobalInterceptor) {
@@ -946,9 +940,6 @@ 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
-  // other ongoing gRPC operations
-  experimental::RegisterGlobalClientInterceptorFactory(nullptr);
 }
 
 }  // namespace