fix Exception throw for invalid channel args

* unit test included
* throw ValueError exception from Cython to Python
* prevent the deconstruction method from failing when Channel initialization failed

4 files changed, 24 insertions, 5 deletions

diff --git a/src/python/grpcio/grpc/_channel.py b/src/python/grpcio/grpc/_channel.py
index 3494c9b15a..eeeb4ddb33 100644
--- a/src/python/grpcio/grpc/_channel.py
+++ b/src/python/grpcio/grpc/_channel.py
@@ -981,4 +981,7 @@ class Channel(grpc.Channel):
         # then deletion of this grpc._channel.Channel instance can be made to
         # effect closure of the underlying cygrpc.Channel instance.
         cygrpc.fork_unregister_channel(self)
-        _moot(self._connectivity_state)
+        # This prevent the failed-at-initializing object removal from failing.
+        # Though the __init__ failed, the removal will still trigger __del__.
+        if hasattr(self, "_connectivity_state"):
+            _moot(self._connectivity_state)
diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/arguments.pxd.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/arguments.pxd.pxi
index 6cb1bc0c05..e0e068e452 100644
--- a/src/python/grpcio/grpc/_cython/_cygrpc/arguments.pxd.pxi
+++ b/src/python/grpcio/grpc/_cython/_cygrpc/arguments.pxd.pxi
@@ -32,7 +32,7 @@ cdef class _ArgumentProcessor:
 
   cdef grpc_arg c_argument
 
-  cdef void c(self, argument, grpc_arg_pointer_vtable *vtable, references)
+  cdef void c(self, argument, grpc_arg_pointer_vtable *vtable, references) except *
 
 
 cdef class _ArgumentsProcessor:
@@ -42,5 +42,5 @@ cdef class _ArgumentsProcessor:
   cdef readonly list _references
   cdef grpc_channel_args _c_arguments
 
-  cdef grpc_channel_args *c(self, grpc_arg_pointer_vtable *vtable)
+  cdef grpc_channel_args *c(self, grpc_arg_pointer_vtable *vtable) except *
   cdef un_c(self)
diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/arguments.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/arguments.pyx.pxi
index 2239e26b32..b7a4277ff6 100644
--- a/src/python/grpcio/grpc/_cython/_cygrpc/arguments.pyx.pxi
+++ b/src/python/grpcio/grpc/_cython/_cygrpc/arguments.pyx.pxi
@@ -52,7 +52,7 @@ cdef grpc_arg _unwrap_grpc_arg(tuple wrapped_arg):
 
 cdef class _ArgumentProcessor:
 
-  cdef void c(self, argument, grpc_arg_pointer_vtable *vtable, references):
+  cdef void c(self, argument, grpc_arg_pointer_vtable *vtable, references) except *:
     key, value = argument
     cdef bytes encoded_key = _encode(key)
     if encoded_key is not key:
@@ -89,7 +89,7 @@ cdef class _ArgumentsProcessor:
     self._argument_processors = []
     self._references = []
 
-  cdef grpc_channel_args *c(self, grpc_arg_pointer_vtable *vtable):
+  cdef grpc_channel_args *c(self, grpc_arg_pointer_vtable *vtable) except *:
     self._c_arguments.arguments_length = len(self._arguments)
     if self._c_arguments.arguments_length == 0:
       return NULL
diff --git a/src/python/grpcio_tests/tests/unit/_channel_args_test.py b/src/python/grpcio_tests/tests/unit/_channel_args_test.py
index 869c2f4d2f..dd1d2969a2 100644
--- a/src/python/grpcio_tests/tests/unit/_channel_args_test.py
+++ b/src/python/grpcio_tests/tests/unit/_channel_args_test.py
@@ -33,6 +33,14 @@ TEST_CHANNEL_ARGS = (
     ('arg6', TestPointerWrapper()),
 )
 
+INVALID_TEST_CHANNEL_ARGS = [
+    {
+        'foo': 'bar'
+    },
+    (('key',),),
+    'str',
+]
+
 
 class ChannelArgsTest(unittest.TestCase):
 
@@ -44,6 +52,14 @@ class ChannelArgsTest(unittest.TestCase):
             futures.ThreadPoolExecutor(max_workers=1),
             options=TEST_CHANNEL_ARGS)
 
+    def test_invalid_client_args(self):
+        for invalid_arg in INVALID_TEST_CHANNEL_ARGS:
+            self.assertRaises(
+                ValueError,
+                grpc.insecure_channel,
+                'localhost:8080',
+                options=invalid_arg)
+
 
 if __name__ == '__main__':
     unittest.main(verbosity=2)