c#: throw RpcException for failed network sends instead of InvalidOperationException #7925
Conversation
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test. |
1 similar comment
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test. |
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test. |
1 similar comment
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test. |
|
Thanks, but I think that a main issue with changing the exception type here is backwards compatibility. There does seem to be a lot of support in favor of making the breaking exception type change though, and I am thinking that a related change could be something to consider for the 1.1 release. |
|
Ahh, makes total sense. I'd love to throw my support behind a more considered change for this to be part of 1.1 as well. |
|
@imiuka, thanks for the pull request. I think the change you are suggesting is a step in right direction - and I agree with the resulting behavior in the situation in question. Unfortunately, the change needs to be a bit more complicated than that. With your version of the change, you are artificially emitting the StatusCode.Unavailable, which is not necessarily the status code with which the RPC is going to finish - there are other error statuses that you can obtain in described situation. Also, you need to guarantee that once you receive StatusCode.Unavailable from an attempted stream write, you will also receive the same status after awaiting other operations on the same call (e.g. a streaming read or unary response) - there should really be one and only one StatusCode for each remote call. So, the problem you are describing is real and we need to fix it - but in a different way. I think issue #7223 describes the problem really well, so if you don't mind, I will continue the discussion on this topic there. I will also close this PR for now with a promise to work on this issue in the nearest future. |
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
Currently, gRPC throws an
InvalidOperationExceptionwhen it's network sends fail. This communicates the wrong semantics (that you're doing something wrong), and also makes it difficult to handle error scenarios where we actually attempted an invalid operation (like reading trailers before trailers are available).Because
InvalidOperationExceptionhere communicates the wrong semantics, we can't be sure if anInvalidOperationExceptionoccurred because we performed an invalid operation, or because of a transient error, without testing for the string "failed send" or "error sending status" which makes for some messy error handling code. If we instead throw anRpcException, clients can easily know that the issue occurred within the RPC layer, rather than failing a contract on its part.This pull request changes them to throw
RpcExceptions withUNAVAILABLEstatus, but I'd also consideredINTERNAL. I feel that unavailable communicates the potential transient nature of communication errors better than internal does (where something is "very broken").Hope this is reasonable, or you have a better solution. Thanks for your time!