Add some Unit tests to Abstract Client Stream #841
Conversation
| try { | ||
| setDecompressor(messageEncoding); | ||
| } catch (IllegalArgumentException e) { | ||
| inboundTransportError(Status.INVALID_ARGUMENT.withDescription(e.getMessage())); |
There was a problem hiding this comment.
This looks like it should be INTERNAL, since the application didn't specify an invalid argument. Instead, either the grpc library on client or server has a bug.
There was a problem hiding this comment.
inboundTransportError() doesn't actually kill the stream. And yes, that means that the other usage of inboundTransportError() in this file is broken. It looks like they should probably call cancel() instead now-a-days.
There was a problem hiding this comment.
This is invalid input, since the remote host tried to use a message encoding that the local one does not support.
cancel() cannot be called, since the javadoc says it can only be called from the application, not the transport. If there is some sort of inboundStreamError (just enough to kill the call) that would be the bets choice IMO.
There was a problem hiding this comment.
Most status codes are directional. INVALID_ARGUMENT is defined as "Client specified an invalid argument." In this case the server specified something that the client doesn't support; that isn't INVALID_ARGUMENT.
Hmm... I'm thinking for now call both sendCancel() and inboundTransportError() and add a TODO to improve the situation. sendCancel() is delayed with Netty. I'm not confident about that solution, but everywhere I look I see potential problems, and that seems the least risky.
There was a problem hiding this comment.
Updated it to use INTERNAL.
|
@carl-mastrangelo looks good! Just a few minor comments. |
| @@ -0,0 +1,254 @@ | |||
| package io.grpc.internal; | |||
There was a problem hiding this comment.
Ah, my old nemesis. Added.
|
@carl-mastrangelo LGTM |
|
PTAL |
| setDecompressor(messageEncoding); | ||
| } catch (IllegalArgumentException e) { | ||
| // Don't use INVALID_ARGUMENT since that is for servers to send clients. | ||
| Status status = Status.INTERNAL.withDescription(e.getMessage()).withCause(e); |
There was a problem hiding this comment.
We haven't generally been copying the message from caused exceptions. However, we should really be providing more context here anyway, so can we replace e.getMessage() with a more clear message. Maybe something like: "Unable to decompress messages from server".
|
@carl-mastrangelo, LGTM, but I had one comment. |
|
Thanks, submitting. |
carl-mastrangelo
force-pushed
the
bzipper
branch
from
da2030d
to
701bbe5
Compare
No description provided.