From 62d895bc77ec9a06659ad66bddc7930eb3ce24fe Mon Sep 17 00:00:00 2001 From: Masood Malekghassemi Date: Thu, 12 Jan 2017 17:16:48 -0800 Subject: 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. --- .gitignore | 5 +- .../grpc/_cython/_cygrpc/completion_queue.pyx.pxi | 6 ++- .../grpcio/grpc/_cython/_cygrpc/records.pxd.pxi | 5 ++ .../grpcio/grpc/_cython/_cygrpc/records.pyx.pxi | 57 ++++++++++++++++------ src/python/grpcio/grpc/_server.py | 2 + 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 (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: (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: -- cgit v1.2.3