Hold onto the GIL in __dealloc__ functions

When a child thread triggers __dealloc__ as part of a thread being
joined, the thread is already considered to be "joined", and so
releasing the GIL can allow the main thread to proceed to exit,
which introduces shutdown race conditions/memory leaks.

6 files changed, 15 insertions, 28 deletions

diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/call.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/call.pyx.pxi
index 6570dcdb85..ba60986143 100644
--- a/src/python/grpcio/grpc/_cython/_cygrpc/call.pyx.pxi
+++ b/src/python/grpcio/grpc/_cython/_cygrpc/call.pyx.pxi
@@ -105,8 +105,7 @@ cdef class Call:
 
   def __dealloc__(self):
     if self.c_call != NULL:
-      with nogil:
-        grpc_call_destroy(self.c_call)
+      grpc_call_destroy(self.c_call)
 
   # The object *should* always be valid from Python. Used for debugging.
   @property
diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/channel.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/channel.pyx.pxi
index 1406696510..5416401431 100644
--- a/src/python/grpcio/grpc/_cython/_cygrpc/channel.pyx.pxi
+++ b/src/python/grpcio/grpc/_cython/_cygrpc/channel.pyx.pxi
@@ -102,5 +102,4 @@ cdef class Channel:
 
   def __dealloc__(self):
     if self.c_channel != NULL:
-      with nogil:
-        grpc_channel_destroy(self.c_channel)
+      grpc_channel_destroy(self.c_channel)
diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/completion_queue.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/completion_queue.pyx.pxi
index 90266516fe..5955021ceb 100644
--- a/src/python/grpcio/grpc/_cython/_cygrpc/completion_queue.pyx.pxi
+++ b/src/python/grpcio/grpc/_cython/_cygrpc/completion_queue.pyx.pxi
@@ -118,18 +118,14 @@ cdef class CompletionQueue:
 
   def __dealloc__(self):
     cdef gpr_timespec c_deadline
-    with nogil:
-      c_deadline = gpr_inf_future(GPR_CLOCK_REALTIME)
+    c_deadline = gpr_inf_future(GPR_CLOCK_REALTIME)
     if self.c_completion_queue != NULL:
       # Ensure shutdown
       if not self.is_shutting_down:
-        with nogil:
-          grpc_completion_queue_shutdown(self.c_completion_queue)
-      # Pump the queue
+        grpc_completion_queue_shutdown(self.c_completion_queue)
+      # Pump the queue (All outstanding calls should have been cancelled)
       while not self.is_shutdown:
-        with nogil:
-          event = grpc_completion_queue_next(
-              self.c_completion_queue, c_deadline, NULL)
+        event = grpc_completion_queue_next(
+            self.c_completion_queue, c_deadline, NULL)
         self._interpret_event(event)
-      with nogil:
-        grpc_completion_queue_destroy(self.c_completion_queue)
+      grpc_completion_queue_destroy(self.c_completion_queue)
diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/credentials.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/credentials.pyx.pxi
index b24e69243e..035ac49a8b 100644
--- a/src/python/grpcio/grpc/_cython/_cygrpc/credentials.pyx.pxi
+++ b/src/python/grpcio/grpc/_cython/_cygrpc/credentials.pyx.pxi
@@ -46,8 +46,7 @@ cdef class ChannelCredentials:
 
   def __dealloc__(self):
     if self.c_credentials != NULL:
-      with nogil:
-        grpc_channel_credentials_release(self.c_credentials)
+      grpc_channel_credentials_release(self.c_credentials)
 
 
 cdef class CallCredentials:
@@ -64,8 +63,7 @@ cdef class CallCredentials:
 
   def __dealloc__(self):
     if self.c_credentials != NULL:
-      with nogil:
-        grpc_call_credentials_release(self.c_credentials)
+      grpc_call_credentials_release(self.c_credentials)
 
 
 cdef class ServerCredentials:
@@ -76,8 +74,7 @@ cdef class ServerCredentials:
 
   def __dealloc__(self):
     if self.c_credentials != NULL:
-      with nogil:
-        grpc_server_credentials_release(self.c_credentials)
+      grpc_server_credentials_release(self.c_credentials)
 
 
 cdef class CredentialsMetadataPlugin:
diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi
index b39b2f08de..54b3d00dfc 100644
--- a/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi
+++ b/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi
@@ -287,8 +287,7 @@ cdef class ByteBuffer:
 
   def __dealloc__(self):
     if self.c_byte_buffer != NULL:
-      with nogil:
-        grpc_byte_buffer_destroy(self.c_byte_buffer)
+      grpc_byte_buffer_destroy(self.c_byte_buffer)
 
 
 cdef class SslPemKeyCertPair:
@@ -420,8 +419,7 @@ cdef class Metadata:
     # this frees the allocated memory for the grpc_metadata_array (although
     # it'd be nice if that were documented somewhere...)
     # TODO(atash): document this in the C core
-    with nogil:
-      grpc_metadata_array_destroy(&self.c_metadata_array)
+    grpc_metadata_array_destroy(&self.c_metadata_array)
 
   def __len__(self):
     return self.c_metadata_array.count
@@ -530,8 +528,7 @@ cdef class Operation:
     # Python. The remaining one(s) are primitive fields filled in by GRPC core.
     # This means that we need to clean up after receive_status_on_client.
     if self.c_op.type == GRPC_OP_RECV_STATUS_ON_CLIENT:
-      with nogil:
-        gpr_free(self._received_status_details)
+      gpr_free(self._received_status_details)
 
 def operation_send_initial_metadata(Metadata metadata, int flags):
   cdef Operation op = Operation()
diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi
index 3e03b6efe1..4f2d51b03f 100644
--- a/src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi
+++ b/src/python/grpcio/grpc/_cython/_cygrpc/server.pyx.pxi
@@ -171,5 +171,4 @@ cdef class Server:
         # much but repeatedly release the GIL and wait
         while not self.is_shutdown:
           time.sleep(0)
-      with nogil:
-        grpc_server_destroy(self.c_server)
+      grpc_server_destroy(self.c_server)