Give ownership of byte buffers to write ops #12306
Conversation
|
|
Makefile
Outdated
| @@ -105,7 +105,7 @@ CC_asan = clang | |||
| CXX_asan = clang++ | |||
| LD_asan = clang++ | |||
| LDXX_asan = clang++ | |||
| CPPFLAGS_asan = -O0 -fsanitize-coverage=edge -fsanitize=address -fno-omit-frame-pointer -Wno-unused-command-line-argument -DGPR_NO_DIRECT_SYSCALLS | |||
There was a problem hiding this comment.
I was running ASAN tests on an older clang, so I removed this for testing. I've reverted it.
include/grpc++/impl/codegen/call.h
Outdated
| @@ -308,7 +308,11 @@ class CallOpSendMessage { | |||
| template <class M> | |||
| Status CallOpSendMessage::SendMessage(const M& message, WriteOptions options) { | |||
| write_options_ = options; | |||
| return SerializationTraits<M>::Serialize(message, &send_buf_, &own_buf_); | |||
| Status result = SerializationTraits<M>::Serialize(message, &send_buf_, &own_buf_); | |||
| if (own_buf_) { | |||
There was a problem hiding this comment.
Under which situations is own_buf_ true?
There was a problem hiding this comment.
(should we just eliminate this flag from the API?)
There was a problem hiding this comment.
+1 -- removing this option seems like the right thing. I'm not really sure why it's here to begin with.
There was a problem hiding this comment.
I've left own_buf in there because SerializationTraits is part of the public API.
This would be an API breaking change to remove the ability to not free the byte buffer, so I'd rather defer that change.
include/grpc++/impl/codegen/call.h
Outdated
| @@ -308,7 +308,11 @@ class CallOpSendMessage { | |||
| template <class M> | |||
| Status CallOpSendMessage::SendMessage(const M& message, WriteOptions options) { | |||
| write_options_ = options; | |||
| return SerializationTraits<M>::Serialize(message, &send_buf_, &own_buf_); | |||
| Status result = SerializationTraits<M>::Serialize(message, &send_buf_, &own_buf_); | |||
| if (own_buf_) { | |||
There was a problem hiding this comment.
+1 -- removing this option seems like the right thing. I'm not really sure why it's here to begin with.
| @@ -255,10 +255,13 @@ cdef class ByteBuffer: | |||
|
|
|||
| def __cinit__(self, bytes data): | |||
| grpc_init() | |||
| self._own_buffer = True | |||
There was a problem hiding this comment.
Similar comment here as in C++. Is there any case where this attribute will actually be true?
There was a problem hiding this comment.
This would end up being true for write ops. This is no longer used in the current implementation however.
471ee98
to
567e0f1
Compare
|
I've switched up the implementation a bit to address memory leaks. |
|
|
|
If the transport stream op batch structure owns the payload, I'd expect to see some cleanup code in grpc_transport_stream_op_batch_finish_with_failure. |
|
@ctiller |
|
|
|
Please add a comment documenting the semantics somewhere around here: https://github.com/grpc/grpc/blob/master/include/grpc/impl/codegen/grpc_types.h#L514 |
|
|
|
In developing a gRPC HHVM extension, I've come across a memory corruption issue related to this. The issue appears to happen in the following situation:
This behavior is observed in valgrind in the timeout case as follows: |
|
@RyanGordon Do you have a pointer to the HHVM code that you are using that is running into this issue? Specifically, I'm interested in your second bullet point: You shouldn't be calling |
|
@kpayson64 Yes absolutely: This is where we do the completion_queue_next call with the timeout argument: https://github.com/grpc/grpc/pull/11553/files#diff-1a64209204460e9c92631584fb266047R597 If the timeout happens then we call grpc_call_cancel_with_status: https://github.com/grpc/grpc/pull/11553/files#diff-1a64209204460e9c92631584fb266047R576 And then before we start the next call grpc_byte_buffer_destroy gets called: https://github.com/grpc/grpc/pull/11553/files#diff-1a64209204460e9c92631584fb266047R312 I guess I don't know how we would know when the call itself has timed out or how to wait on that differently than what we currently do? |
|
@RyanGordon
If for some reason you need to manually call |
|
@kpayson64 Thank you for explaining that - definitely did not realize the interactions there. Honestly, the only reason why we added a timeout to grpc_completion_queue_next is because during stress testing the |
This implementation does not give ownership of the entire
byte_bufferto the transport, but rather allows the transport to steal the reference to the slices.The reason for this is that the transport only holds on to the slice buffer, which is a field in the byte buffer.
The application is still responsible for calling
grpc_byte_buffer_destroy()but the contents of that buffer is no longer guaranteed.