Paper-over Python errors

This reads directly off of the slices rather than ref'ing and unref'ing
them. There's likely some silliness w.r.t. interned slices and
references to them outliving their associated call objects, but we are
just ignoring that for now.

5 files changed, 56 insertions, 19 deletions

diff --git a/.gitignore b/.gitignore
index 3112445176..06a3d25c3d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -31,7 +31,7 @@ coverage
 # python compiled objects
 *.pyc
 
-#eclipse project files
+# eclipse project files
 .cproject
 .project
 .settings
@@ -110,3 +110,6 @@ bazel-genfiles
 bazel-grpc
 bazel-out
 bazel-testlogs
+
+# Debug output
+gdb.txt
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 a258ba4063..2b5cce88a4 100644
--- a/src/python/grpcio/grpc/_cython/_cygrpc/completion_queue.pyx.pxi
+++ b/src/python/grpcio/grpc/_cython/_cygrpc/completion_queue.pyx.pxi
@@ -70,6 +70,8 @@ cdef class CompletionQueue:
         operation_call = tag.operation_call
         request_call_details = tag.request_call_details
         request_metadata = tag.request_metadata
+        if request_metadata is not None:
+          request_metadata._claim_slice_ownership()
         batch_operations = tag.batch_operations
         if tag.is_new_request:
           # Stuff in the tag not explicitly handled by us needs to live through
@@ -91,7 +93,7 @@ cdef class CompletionQueue:
       c_deadline = gpr_inf_future(GPR_CLOCK_REALTIME)
       if deadline is not None:
         c_deadline = deadline.c_time
-      
+
       while True:
         c_timeout = gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), c_increment)
         if gpr_time_cmp(c_timeout, c_deadline) > 0:
@@ -100,7 +102,7 @@ cdef class CompletionQueue:
           self.c_completion_queue, c_timeout, NULL)
         if event.type != GRPC_QUEUE_TIMEOUT or gpr_time_cmp(c_timeout, c_deadline) == 0:
           break;
-        
+
         # Handle any signals
         with gil:
           cpython.PyErr_CheckSignals()
diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/records.pxd.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/records.pxd.pxi
index 870da51fa8..3644b7cdae 100644
--- a/src/python/grpcio/grpc/_cython/_cygrpc/records.pxd.pxi
+++ b/src/python/grpcio/grpc/_cython/_cygrpc/records.pxd.pxi
@@ -77,6 +77,8 @@ cdef class Slice:
   cdef void _assign_slice(self, grpc_slice new_slice) nogil
   @staticmethod
   cdef Slice from_slice(grpc_slice slice)
+  @staticmethod
+  cdef bytes bytes_from_slice(grpc_slice slice)
 
 
 cdef class ByteBuffer:
@@ -113,7 +115,10 @@ cdef class Metadatum:
 cdef class Metadata:
 
   cdef grpc_metadata_array c_metadata_array
+  cdef bint owns_metadata_slices
   cdef object metadata
+  cdef void _claim_slice_ownership(self) nogil
+  cdef void _drop_slice_ownership(self) nogil
 
 
 cdef class Operation:
diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi
index b7a75cd97a..30e7b9657a 100644
--- a/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi
+++ b/src/python/grpcio/grpc/_cython/_cygrpc/records.pyx.pxi
@@ -189,11 +189,11 @@ cdef class CallDetails:
 
   @property
   def method(self):
-    return Slice.from_slice(self.c_details.method).bytes()
+    return Slice.bytes_from_slice(self.c_details.method)
 
   @property
   def host(self):
-    return Slice.from_slice(self.c_details.host).bytes()
+    return Slice.bytes_from_slice(self.c_details.host)
 
   @property
   def deadline(self):
@@ -251,12 +251,16 @@ cdef class Slice:
     self._assign_slice(slice)
     return self
 
-  def bytes(self):
+  @staticmethod
+  cdef bytes bytes_from_slice(grpc_slice slice):
     with nogil:
-      pointer = grpc_slice_start_ptr(self.c_slice)
-      length = grpc_slice_length(self.c_slice)
+      pointer = grpc_slice_start_ptr(slice)
+      length = grpc_slice_length(slice)
     return (<char *>pointer)[:length]
 
+  def bytes(self):
+    return Slice.bytes_from_slice(self.c_slice)
+
   def __dealloc__(self):
     with nogil:
       grpc_slice_unref(self.c_slice)
@@ -466,13 +470,14 @@ cdef class _MetadataIterator:
 cdef class Metadata:
 
   def __cinit__(self, metadata):
-    grpc_init()
+    with nogil:
+      grpc_init()
+      grpc_metadata_array_init(&self.c_metadata_array)
+    self.owns_metadata_slices = False
     self.metadata = list(metadata)
-    for metadatum in metadata:
+    for metadatum in self.metadata:
       if not isinstance(metadatum, Metadatum):
         raise TypeError("expected list of Metadatum")
-    with nogil:
-      grpc_metadata_array_init(&self.c_metadata_array)
     self.c_metadata_array.count = len(self.metadata)
     self.c_metadata_array.capacity = len(self.metadata)
     with nogil:
@@ -484,23 +489,43 @@ cdef class Metadata:
           (<Metadatum>self.metadata[i]).c_metadata)
 
   def __dealloc__(self):
-    # 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
-    grpc_metadata_array_destroy(&self.c_metadata_array)
-    grpc_shutdown()
+    with nogil:
+      self._drop_slice_ownership()
+      # 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
+      grpc_metadata_array_destroy(&self.c_metadata_array)
+      grpc_shutdown()
 
   def __len__(self):
     return self.c_metadata_array.count
 
   def __getitem__(self, size_t i):
+    if i >= self.c_metadata_array.count:
+      raise IndexError
     return Metadatum(
-        key=Slice.from_slice(self.c_metadata_array.metadata[i].key).bytes(),
-        value=Slice.from_slice(self.c_metadata_array.metadata[i].value).bytes())
+        key=Slice.bytes_from_slice(self.c_metadata_array.metadata[i].key),
+        value=Slice.bytes_from_slice(self.c_metadata_array.metadata[i].value))
 
   def __iter__(self):
     return _MetadataIterator(self)
 
+  cdef void _claim_slice_ownership(self) nogil:
+    if self.owns_metadata_slices:
+      return
+    for i in range(self.c_metadata_array.count):
+      grpc_slice_ref(self.c_metadata_array.metadata[i].key)
+      grpc_slice_ref(self.c_metadata_array.metadata[i].value)
+    self.owns_metadata_slices = True
+
+  cdef void _drop_slice_ownership(self) nogil:
+    if not self.owns_metadata_slices:
+      return
+    for i in range(self.c_metadata_array.count):
+      grpc_slice_unref(self.c_metadata_array.metadata[i].key)
+      grpc_slice_unref(self.c_metadata_array.metadata[i].value)
+    self.owns_metadata_slices = False
+
 
 cdef class Operation:
 
diff --git a/src/python/grpcio/grpc/_server.py b/src/python/grpcio/grpc/_server.py
index 5223712dfa..75f661340c 100644
--- a/src/python/grpcio/grpc/_server.py
+++ b/src/python/grpcio/grpc/_server.py
@@ -576,6 +576,8 @@ def _handle_with_method_handler(rpc_event, method_handler, thread_pool):
 
 
 def _handle_call(rpc_event, generic_handlers, thread_pool):
+  if not rpc_event.success:
+    return None
   if rpc_event.request_call_details.method is not None:
     method_handler = _find_method_handler(rpc_event, generic_handlers)
     if method_handler is None: