grpc: always check value of returned grpc::Status.#6917
grpc: always check value of returned grpc::Status.#6917PiotrSikora wants to merge 1 commit intoenvoyproxy:masterfrom
Conversation
Fixes Google import, since grpc::Status is internally annotated with warn_unused_result. Signed-off-by: Piotr Sikora <piotrsikora@google.com>
|
cc @jplevyak |
| // lifetime of the Slice(s) exceeds our Buffer::Instance. | ||
| std::vector<grpc::Slice> slices; | ||
| byte_buffer.Dump(&slices); | ||
| RELEASE_ASSERT(byte_buffer.Dump(&slices) == grpc::Status::OK, ""); |
There was a problem hiding this comment.
Please explain what happens in the rc details failure. Should this only happen if the machine is out of memory? If it can otherwise fail we should probably do error handling instead
There was a problem hiding this comment.
@alyssawilk Good call!
It looks like this can fail for various reasons (e.g. uninitialized buffer, decompression errors when input buffer is compressed), but GoogleGrpcUtils::makeBufferInstance() is only used in tests, so I'm not sure how much we care about those...
@jplevyak any plans for this function to be used for real traffic? Perhaps we should move it into tests?
There was a problem hiding this comment.
I can do the error handling. The question is what is most appropriate. Since the ByteBuffer is coming from the low level gRPC library, the most likely causes would be a bug in that library or memory corruption. This would suggest that a RELEASE_ASSERT at that level would be appropriate. WDYT? Alternatively an Exception or at critical log?
There was a problem hiding this comment.
From looking at the gRPC code, it seems that Dump() (well, grpc_byte_buffer_reader_init()) can fail because of decompression errors, which means that if someone sends corrupted gRPC message, then Dump() would return error, so we should treat it as invalid request/response, and not runtime error.
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
See envoyproxy#6917 for details. Signed-off-by: John Plevyak <jplevyak@gmail.com>
|
Closing (incorporated in #6525). |
Fixes Google import, since grpc::Status is internally annotated
with warn_unused_result.
Signed-off-by: Piotr Sikora piotrsikora@google.com