Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gRPC-client should fail responses without grpc-status code #934

Merged
merged 5 commits into from Feb 12, 2020

Conversation

@idelpivnitskiy
Copy link
Member

idelpivnitskiy commented Feb 7, 2020

Motivation:

gRPC protocol requires server to send grpc-status code as part
of the response. It may come in headers (when there is no
payload body) or in trailers. Current implementation considers
responses without grpc-status as legit responses.

Modifications:

  • Ensure that the response object contains grpc-status code;
  • Tests to verify that client throws an exception when server
    does not send grpc-status;

Result:

gRPC-client throws an exception when server does not send
grpc-status.

Motivation:

gRPC protocol requires server to send grpc-status code as part
of the response. It may come in headers (when there is no
payload body) or in trailers. Current implementation considers
responses without `grpc-status` as legit responses.

Modifications:

- Ensure that the response object contains `grpc-status` code;
- Tests to verify that client throws an exception when server
does not send `grpc-status`;

Result:

gRPC-client throws an exception when server does not send
`grpc-status`.
@idelpivnitskiy idelpivnitskiy requested a review from NiteshKant Feb 7, 2020
}).filter(o -> !(o instanceof HttpHeaders)).map(o -> (Buffer) o));

response.transformRaw(ENSURE_GRPC_STATUS_RECEIVED);
return deserializer.deserialize(headers, response.payloadBodyAndTrailers()

This comment has been minimized.

Copy link
@idelpivnitskiy

idelpivnitskiy Feb 7, 2020

Author Member

This helps to catch the case when server does not send trailers at all.
In this case payloadBodyAndTrailers() returns only Buffers. transformRaw helps to create empty trailers and catch that there is no grpc-status.
I did not add a comment here, because the test will fail if this changes, but I can add it if it will be useful.

This comment has been minimized.

Copy link
@NiteshKant

NiteshKant Feb 11, 2020

Member

This makes sense. If you wish to avoid calling transformRaw() (and create trailers); you can use a liftSync() to intercept onComplete() (you would still have to use filter() like before).

This comment has been minimized.

Copy link
@idelpivnitskiy

idelpivnitskiy Feb 11, 2020

Author Member

Good idea, I will keep it with transformRaw for now and we can optimize if it show ups as a problem later.

Copy link
Member

NiteshKant left a comment

Minor comments, then LGTM

}).filter(o -> !(o instanceof HttpHeaders)).map(o -> (Buffer) o));

response.transformRaw(ENSURE_GRPC_STATUS_RECEIVED);
return deserializer.deserialize(headers, response.payloadBodyAndTrailers()

This comment has been minimized.

Copy link
@NiteshKant

NiteshKant Feb 11, 2020

Member

This makes sense. If you wish to avoid calling transformRaw() (and create trailers); you can use a liftSync() to intercept onComplete() (you would still have to use filter() like before).

idelpivnitskiy and others added 4 commits Feb 11, 2020
…cUtils.java


Address comments

Co-Authored-By: Nitesh Kant <nitesh_kant@apple.com>
@idelpivnitskiy

This comment has been minimized.

Copy link
Member Author

idelpivnitskiy commented Feb 12, 2020

Build failed due to some docker issue:

ERROR: for servicetalk-java8-prb_runtime-setup_1 Cannot start service runtime-setup: network servicetalk-java8-prb_default not found

@servicetalk-bot test this please

@idelpivnitskiy idelpivnitskiy merged commit 349f2ca into apple:master Feb 12, 2020
3 checks passed
3 checks passed
pull request validation (jdk11) Build finished.
Details
pull request validation (jdk8) Build finished.
Details
pull request validation (quality) Build finished.
Details
@idelpivnitskiy idelpivnitskiy deleted the idelpivnitskiy:grpc-require-trailers branch Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.