Merge pull request #9127 from soltanmm-google/if-you-rely-only-on-yourself-fewer-things-might-go-wrong-but-fewer-things-might-go-right

Don't leak Py exceptions without calling gRPC core

10 files changed, 41 insertions, 33 deletions

diff --git a/src/python/grpcio/commands.py b/src/python/grpcio/commands.py
index e09f922591..d813df5f44 100644
--- a/src/python/grpcio/commands.py
+++ b/src/python/grpcio/commands.py
@@ -271,12 +271,12 @@ class BuildExt(build_ext.build_ext):
         compiler = self.compiler.compiler_type
         if compiler in BuildExt.C_OPTIONS:
             for extension in self.extensions:
-                extension.extra_compile_args += list(BuildExt.C_OPTIONS[
-                    compiler])
+                extension.extra_compile_args += list(
+                    BuildExt.C_OPTIONS[compiler])
         if compiler in BuildExt.LINK_OPTIONS:
             for extension in self.extensions:
-                extension.extra_link_args += list(BuildExt.LINK_OPTIONS[
-                    compiler])
+                extension.extra_link_args += list(
+                    BuildExt.LINK_OPTIONS[compiler])
         if not check_and_update_cythonization(self.extensions):
             self.extensions = try_cythonize(self.extensions)
         try:
@@ -284,8 +284,8 @@ class BuildExt(build_ext.build_ext):
         except Exception as error:
             formatted_exception = traceback.format_exc()
             support.diagnose_build_ext_error(self, error, formatted_exception)
-            raise CommandError("Failed `build_ext` step:\n{}".format(
-                formatted_exception))
+            raise CommandError(
+                "Failed `build_ext` step:\n{}".format(formatted_exception))
 
 
 class Gather(setuptools.Command):
diff --git a/src/python/grpcio/grpc/_channel.py b/src/python/grpcio/grpc/_channel.py
index 77412236cc..5a8a3d487a 100644
--- a/src/python/grpcio/grpc/_channel.py
+++ b/src/python/grpcio/grpc/_channel.py
@@ -842,8 +842,8 @@ def _poll_connectivity(state, channel, initial_try_to_connect):
     connectivity = channel.check_connectivity_state(try_to_connect)
     with state.lock:
         state.connectivity = (
-            _common.CYGRPC_CONNECTIVITY_STATE_TO_CHANNEL_CONNECTIVITY[
-                connectivity])
+            _common.
+            CYGRPC_CONNECTIVITY_STATE_TO_CHANNEL_CONNECTIVITY[connectivity])
         callbacks = tuple(callback
                           for callback, unused_but_known_to_be_none_connectivity
                           in state.callbacks_and_connectivities)
diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/credentials.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/credentials.pyx.pxi
index 04872b9c09..4d988192df 100644
--- a/src/python/grpcio/grpc/_cython/_cygrpc/credentials.pyx.pxi
+++ b/src/python/grpcio/grpc/_cython/_cygrpc/credentials.pyx.pxi
@@ -29,6 +29,8 @@
 
 cimport cpython
 
+import traceback
+
 
 cdef class ChannelCredentials:
 
@@ -138,15 +140,22 @@ cdef class AuthMetadataContext:
 cdef void plugin_get_metadata(
     void *state, grpc_auth_metadata_context context,
     grpc_credentials_plugin_metadata_cb cb, void *user_data) with gil:
+  called_flag = [False]
   def python_callback(
       Metadata metadata, grpc_status_code status,
       bytes error_details):
     cb(user_data, metadata.c_metadata_array.metadata,
        metadata.c_metadata_array.count, status, error_details)
+    called_flag[0] = True
   cdef CredentialsMetadataPlugin self = <CredentialsMetadataPlugin>state
   cdef AuthMetadataContext cy_context = AuthMetadataContext()
   cy_context.context = context
-  self.plugin_callback(cy_context, python_callback)
+  try:
+    self.plugin_callback(cy_context, python_callback)
+  except Exception as error:
+    if not called_flag[0]:
+      cb(user_data, Metadata([]).c_metadata_array.metadata,
+         0, StatusCode.unknown, traceback.format_exc().encode())
 
 cdef void plugin_destroy_c_plugin_state(void *state) with gil:
   cpython.Py_DECREF(<CredentialsMetadataPlugin>state)
diff --git a/src/python/grpcio_tests/commands.py b/src/python/grpcio_tests/commands.py
index 845b7f598c..af0ffe3475 100644
--- a/src/python/grpcio_tests/commands.py
+++ b/src/python/grpcio_tests/commands.py
@@ -121,8 +121,8 @@ class BuildProtoModules(setuptools.Command):
                 '--grpc_python_out={}'.format(PROTO_STEM),
             ] + [path]
             if protoc.main(command) != 0:
-                sys.stderr.write('warning: Command:\n{}\nFailed'.format(
-                    command))
+                sys.stderr.write(
+                    'warning: Command:\n{}\nFailed'.format(command))
 
         # Generated proto directories dont include __init__.py, but
         # these are needed for python package resolution
diff --git a/src/python/grpcio_tests/tests/_loader.py b/src/python/grpcio_tests/tests/_loader.py
index 42cf9ab4ca..165bc53fb7 100644
--- a/src/python/grpcio_tests/tests/_loader.py
+++ b/src/python/grpcio_tests/tests/_loader.py
@@ -116,5 +116,5 @@ def iterate_suite_cases(suite):
         elif isinstance(item, unittest.TestCase):
             yield item
         else:
-            raise ValueError('unexpected suite item of type {}'.format(
-                type(item)))
+            raise ValueError(
+                'unexpected suite item of type {}'.format(type(item)))
diff --git a/src/python/grpcio_tests/tests/_runner.py b/src/python/grpcio_tests/tests/_runner.py
index 59964b271c..1138a2279d 100644
--- a/src/python/grpcio_tests/tests/_runner.py
+++ b/src/python/grpcio_tests/tests/_runner.py
@@ -196,8 +196,8 @@ class Runner(object):
         # Run the tests
         result.startTestRun()
         for augmented_case in augmented_cases:
-            sys.stdout.write('Running       {}\n'.format(augmented_case.case.id(
-            )))
+            sys.stdout.write(
+                'Running       {}\n'.format(augmented_case.case.id()))
             sys.stdout.flush()
             case_thread = threading.Thread(
                 target=augmented_case.case.run, args=(result,))
diff --git a/src/python/grpcio_tests/tests/interop/methods.py b/src/python/grpcio_tests/tests/interop/methods.py
index e1f8722168..bdb258591e 100644
--- a/src/python/grpcio_tests/tests/interop/methods.py
+++ b/src/python/grpcio_tests/tests/interop/methods.py
@@ -428,8 +428,8 @@ def _compute_engine_creds(stub, args):
 
 
 def _oauth2_auth_token(stub, args):
-    json_key_filename = os.environ[
-        oauth2client_client.GOOGLE_APPLICATION_CREDENTIALS]
+    json_key_filename = os.environ[oauth2client_client.
+                                   GOOGLE_APPLICATION_CREDENTIALS]
     wanted_email = json.load(open(json_key_filename, 'rb'))['client_email']
     response = _large_unary_common_behavior(stub, True, True, None)
     if wanted_email != response.username:
@@ -441,8 +441,8 @@ def _oauth2_auth_token(stub, args):
 
 
 def _jwt_token_creds(stub, args):
-    json_key_filename = os.environ[
-        oauth2client_client.GOOGLE_APPLICATION_CREDENTIALS]
+    json_key_filename = os.environ[oauth2client_client.
+                                   GOOGLE_APPLICATION_CREDENTIALS]
     wanted_email = json.load(open(json_key_filename, 'rb'))['client_email']
     response = _large_unary_common_behavior(stub, True, False, None)
     if wanted_email != response.username:
@@ -451,11 +451,10 @@ def _jwt_token_creds(stub, args):
 
 
 def _per_rpc_creds(stub, args):
-    json_key_filename = os.environ[
-        oauth2client_client.GOOGLE_APPLICATION_CREDENTIALS]
+    json_key_filename = os.environ[oauth2client_client.
+                                   GOOGLE_APPLICATION_CREDENTIALS]
     wanted_email = json.load(open(json_key_filename, 'rb'))['client_email']
-    credentials = oauth2client_client.GoogleCredentials.get_application_default(
-    )
+    credentials = oauth2client_client.GoogleCredentials.get_application_default()
     scoped_credentials = credentials.create_scoped([args.oauth_scope])
     # TODO(https://github.com/grpc/grpc/issues/6799): Eliminate this last
     # remaining use of the Beta API.
diff --git a/src/python/grpcio_tests/tests/qps/worker_server.py b/src/python/grpcio_tests/tests/qps/worker_server.py
index 1deb7ed698..ca1a777611 100644
--- a/src/python/grpcio_tests/tests/qps/worker_server.py
+++ b/src/python/grpcio_tests/tests/qps/worker_server.py
@@ -102,8 +102,8 @@ class WorkerServer(services_pb2.WorkerServiceServicer):
                 'grpc.testing.BenchmarkService', method_implementations)
             server.add_generic_rpc_handlers((handler,))
         else:
-            raise Exception('Unsupported server type {}'.format(
-                config.server_type))
+            raise Exception(
+                'Unsupported server type {}'.format(config.server_type))
 
         if config.HasField('security_params'):  # Use SSL
             server_creds = grpc.ssl_server_credentials((
@@ -171,8 +171,8 @@ class WorkerServer(services_pb2.WorkerServiceServicer):
             else:
                 raise Exception('Async streaming client not supported')
         else:
-            raise Exception('Unsupported client type {}'.format(
-                config.client_type))
+            raise Exception(
+                'Unsupported client type {}'.format(config.client_type))
 
         # In multi-channel tests, we split the load across all channels
         load_factor = float(config.client_channels)
diff --git a/src/python/grpcio_tests/tests/unit/_cython/cygrpc_test.py b/src/python/grpcio_tests/tests/unit/_cython/cygrpc_test.py
index a4d5015aa4..b4efe87730 100644
--- a/src/python/grpcio_tests/tests/unit/_cython/cygrpc_test.py
+++ b/src/python/grpcio_tests/tests/unit/_cython/cygrpc_test.py
@@ -214,8 +214,8 @@ class ServerClientMixin(object):
                 self.assertTrue(event.success)
                 self.assertIs(tag, event.tag)
             except Exception as error:
-                raise Exception("Error in '{}': {}".format(description,
-                                                           error.message))
+                raise Exception(
+                    "Error in '{}': {}".format(description, error.message))
             return event
 
         return test_utilities.SimpleFuture(performer)
diff --git a/src/python/grpcio_tests/tests/unit/_empty_message_test.py b/src/python/grpcio_tests/tests/unit/_empty_message_test.py
index 4588688ea6..155173807f 100644
--- a/src/python/grpcio_tests/tests/unit/_empty_message_test.py
+++ b/src/python/grpcio_tests/tests/unit/_empty_message_test.py
@@ -122,13 +122,13 @@ class EmptyMessageTest(unittest.TestCase):
                                  list(response_iterator))
 
     def testStreamUnary(self):
-        response = self._channel.stream_unary(_STREAM_UNARY)(iter(
-            [_REQUEST] * test_constants.STREAM_LENGTH))
+        response = self._channel.stream_unary(_STREAM_UNARY)(
+            iter([_REQUEST] * test_constants.STREAM_LENGTH))
         self.assertEqual(_RESPONSE, response)
 
     def testStreamStream(self):
-        response_iterator = self._channel.stream_stream(_STREAM_STREAM)(iter(
-            [_REQUEST] * test_constants.STREAM_LENGTH))
+        response_iterator = self._channel.stream_stream(_STREAM_STREAM)(
+            iter([_REQUEST] * test_constants.STREAM_LENGTH))
         self.assertSequenceEqual([_RESPONSE] * test_constants.STREAM_LENGTH,
                                  list(response_iterator))