Skip to content

Handle header with errors and endStream = true#10384

Merged
larry-safran merged 8 commits intogrpc:masterfrom
larry-safran:is_10364
Jul 20, 2023
Merged

Handle header with errors and endStream = true#10384
larry-safran merged 8 commits intogrpc:masterfrom
larry-safran:is_10364

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

@larry-safran larry-safran requested a review from ejona86 July 17, 2023 21:40
…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.

…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