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

Fix grpc status headers #2973

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

FliegenKLATSCH
Copy link

Fixes #2961

Headers are only set, if they are received.
In case the status comes with the first header and is an error response.setComplete() is called to set endStream=true and prevent an empty DATA frame being sent. This would lead to Received unexpected EOS on DATA frame from server errors otherwise on the grpc client side.

@pivotal-cla
Copy link

@FliegenKLATSCH Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@FliegenKLATSCH Thank you for signing the Contributor License Agreement!

@spencergibb
Copy link
Member

Can you add a test? @abelsromero @Albertoimpl can you review

@FliegenKLATSCH
Copy link
Author

@spencergibb Any hints on how I could set trailing headers for a test case? Doesn't seem to be easy :/
(The patch is running in our production environment since half a year, without any issue.)

@spencergibb
Copy link
Member

I don't. Let's wait for my co-workers tagged above to respond

@Albertoimpl
Copy link
Member

Thanks for the ping @spencergibb, and thanks a lot @FliegenKLATSCH for the contribution and for wanting to improve our tests around it.
To add custom headers and trailers we can add to our integration-test application a custom interceptor.

For headers, it should be something like this: https://github.com/grpc/grpc-java/blob/master/examples/src/main/java/io/grpc/examples/header/HeaderServerInterceptor.java

For trailers what you see in the close: https://github.com/grpc/grpc-java/blob/master/interop-testing/src/main/java/io/grpc/testing/integration/TestServiceImpl.java#L587-L590

@FliegenKLATSCH
Copy link
Author

Thank you! Ping for review @Albertoimpl @abelsromero

@Albertoimpl
Copy link
Member

These assertions should cover the case, thanks @FliegenKLATSCH!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong grpc-status header
5 participants