Streamline metadata in gRPC Python #13672
Conversation
nathanielmanistaatgoogle
force-pushed
the
12531
branch
from
ef7d5f5
to
f4eee19
Compare
There was a problem hiding this comment.
Only blocking comment is the one about using [0] for dereferencing.
Could I also request that you just run a simple client server in a loop for 10 minutes with extremely large initial/trailing metadata on every call? It seems like we have the who-owns-what-c-structure right, but we have been bitten by metadata leaks enough that I'm cautious.
| logging.exception('Huh? Metadata was "%s"!', metadata) | ||
|
|
||
|
|
||
| cdef void _un_c_metadata(int count, grpc_metadata *c_metadata): |
There was a problem hiding this comment.
Can we call this delete_c_metadata? I needed to look at the function to get a sense of what it did.
There was a problem hiding this comment.
I thought I had a good joke going, but... okay. How's _release_c_metadata?
| else: | ||
| metadatum_count = len(metadata) | ||
| if metadatum_count == 0: | ||
| c_count[0] = 0 |
There was a problem hiding this comment.
I assume this ([0]) is a workaround an issue with the * operator? Can you link to a bug if it is a known cython issue?
There was a problem hiding this comment.
I feel similarly but I don't think it's a workaround. I think it might actually be the way Cython code is supposed to be written; Cython flatly suggests doing so with the message a starred assignment target must be in a list or tuple - maybe you meant to use an index assignment: var[0] = ... when I try to write it with the * operator.
There was a problem hiding this comment.
I just looked at the docs, you are right this is how Cython wants to dereference things...
| _Metadatum = collections.namedtuple('_Metadatum', ('key', 'value',)) | ||
|
|
||
|
|
||
| cdef void _c_metadata(metadata, size_t *c_count, grpc_metadata **c_metadata): |
There was a problem hiding this comment.
Nit: I'm used to seeing c functions that take an "output array of variable length" have the arguments ordered func(arg, char** buf, int* length)
There was a problem hiding this comment.
Thanks for noticing; I think gRPC Core consistently uses that ordering as well. Changed both for this function and its inverse.
nathanielmanistaatgoogle
force-pushed
the
12531
branch
from
f4eee19
to
5fe1fe7
Compare
There was a problem hiding this comment.
I'm similarly cautious and I believe I have the leak-driving code left over from pull request 12466. I'll give it a run in the morning.
| else: | ||
| metadatum_count = len(metadata) | ||
| if metadatum_count == 0: | ||
| c_count[0] = 0 |
There was a problem hiding this comment.
I feel similarly but I don't think it's a workaround. I think it might actually be the way Cython code is supposed to be written; Cython flatly suggests doing so with the message a starred assignment target must be in a list or tuple - maybe you meant to use an index assignment: var[0] = ... when I try to write it with the * operator.
| _Metadatum = collections.namedtuple('_Metadatum', ('key', 'value',)) | ||
|
|
||
|
|
||
| cdef void _c_metadata(metadata, size_t *c_count, grpc_metadata **c_metadata): |
There was a problem hiding this comment.
Thanks for noticing; I think gRPC Core consistently uses that ordering as well. Changed both for this function and its inverse.
| logging.exception('Huh? Metadata was "%s"!', metadata) | ||
|
|
||
|
|
||
| cdef void _un_c_metadata(int count, grpc_metadata *c_metadata): |
There was a problem hiding this comment.
I thought I had a good joke going, but... okay. How's _release_c_metadata?
nathanielmanistaatgoogle
force-pushed
the
12531
branch
from
5fe1fe7
to
80516e8
Compare
|
I have now gone through the leak sanity check (I deliberately introduce a leak, ran my leak script, observed a leak, removed my deliberately-introduced leak, ran the script again, and observed no leak). |
No description provided.