Skip to content

grpc: Attempt to read trailers always when parsing grpc exception [DO NOT MERGE] - #3185

Closed
DoumanAsh wants to merge 4 commits into
square:masterfrom
DoumanAsh:fix_grpc_parser_missing_trailers_on_io_exception
Closed

grpc: Attempt to read trailers always when parsing grpc exception [DO NOT MERGE]#3185
DoumanAsh wants to merge 4 commits into
square:masterfrom
DoumanAsh:fix_grpc_parser_missing_trailers_on_io_exception

Conversation

@DoumanAsh

@DoumanAshDoumanAsh commented Nov 25, 2024

Copy link
Copy Markdown

This should prevent missing grpc-status in case of inability to read message correctly

Unless this is changed, any failure to read body will result in generic IO exception when grpc-status is in trailers

@DoumanAsh

Copy link
Copy Markdown
Author

I've been notified of #3087

Actually that makes sense if body read fails, but it doesn't make sense if body is just empty so I'm not sure if this PR is worth it

@oldergodoldergod left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Do you think you could write a test in GrpcClientTest ?

@DoumanAsh

Copy link
Copy Markdown
Author

Will do 👌

@DoumanAshDoumanAsh changed the title grpc: Attempt to read trailers always when parsing grpc exceptiongrpc: Attempt to read trailers always when parsing grpc exception [DO NOT MERGE]Nov 26, 2024
@DoumanAsh

DoumanAsh commented Nov 26, 2024

Copy link
Copy Markdown
Author

I tried to simulate situation that we encountered in wild but I think it doesn't go correctly through this part
https://github.com/square/wire/blob/master/wire-grpc-client/src/jvmMain/kotlin/com/squareup/wire/internal/RealGrpcCall.kt#L107-L109

For some reason even when I send empty body, it successfully passes despite exception supposed to be present
I'm not very familiar with internals but I guess this test suite doesn't simulate real client behavior

@DoumanAsh

DoumanAsh commented Dec 12, 2024

Copy link
Copy Markdown
Author

This is fundamentally not correct approach as trailers() might not be available due to how buggy okhttp library is
Correct approach would be to re-write grpc call to correctly handle empty response as distinct case instead of failing to read body

For reference: when trailers are not ready you would get exception

java.lang.IllegalStateException: too early; can't read the trailers yet

@DoumanAsh
DoumanAsh deleted the fix_grpc_parser_missing_trailers_on_io_exception branch December 12, 2024 08:39
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@DoumanAsh@oldergod