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

Handle header with errors and endStream = true #10384

Merged
merged 8 commits into from
Jul 20, 2023

Conversation

larry-safran
Copy link
Contributor

We catch a number of errors in onHeadersRead where we return an error to the caller and then return without completing any more logic. When endStream is true, onDataRead which is dependent on values set at the end of the onHeadersRead method which was leading to NPEs.

Fixes #10364

…operty for streamKey (header processing identified an error).
@@ -679,12 +685,14 @@ void returnProcessedBytes(Http2Stream http2Stream, int bytes) {

private void closeStreamWhenDone(ChannelPromise promise, int streamId) throws Http2Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... All callers to this method have the TransportState, but it is hidden behind the StreamIdHolder interface. In the future we should probably just have the callers pass the object in, but that'd require some changes to the commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would break encapsulation. Since the public method is using StreamIdHolder we would have to check if the type was TransportState and have alternate logic based upon whether it was or not which might be more efficient, but would also be more complicated.

Copy link
Member

Choose a reason for hiding this comment

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

No, we'd just expose TransportState directly from the commands. The commands are just for Netty*Stream and Netty*Handler to communicate.

netty/src/main/java/io/grpc/netty/NettyServerHandler.java Outdated Show resolved Hide resolved
…axage (unless maxAge is < 2 minutes) for the RLS configuration from proto.
…r than maxage (unless maxAge is < 2 minutes) for the RLS configuration from proto."

This reverts commit aa87130.
@larry-safran larry-safran merged commit 864d30c into grpc:master Jul 20, 2023
@larry-safran larry-safran deleted the is_10364 branch July 20, 2023 22:32
@larry-safran larry-safran added the TODO:backport PR needs to be backported. Removed after backport complete label Jul 24, 2023
larry-safran added a commit to larry-safran/grpc-java that referenced this pull request Jul 24, 2023
* Eliminate NPE by skipping further processing when stream is defined, but doesn't have a property for streamKey (header processing identified an error)

Fixes grpc#10364

* Add unit test for missing content type
larry-safran added a commit that referenced this pull request Jul 24, 2023
* Eliminate NPE by skipping further processing when stream is defined, but doesn't have a property for streamKey (header processing identified an error)

Fixes #10364

* Add unit test for missing content type
@larry-safran larry-safran removed the TODO:backport PR needs to be backported. Removed after backport complete label Jul 24, 2023
larry-safran added a commit that referenced this pull request Jul 24, 2023
* Eliminate NPE by skipping further processing when stream is defined, but doesn't have a property for streamKey (header processing identified an error)

Fixes #10364

* Add unit test for missing content type
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GET requests trigger NullPointerExceptions
2 participants