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

Add root cause exception when throwing PrematureCloseException #2937

Merged
merged 3 commits into from
Oct 18, 2023

Conversation

pderop
Copy link
Member

@pderop pderop commented Oct 17, 2023

Reactor Netty’s HttpClient relies on Netty’s Http2FrameCodecBuilder. This codec, by default, enforces the MAX_HEADER_LIST_SIZE found in the SETTINGS sent by the server, meaning that If a request’s header list size exceeds the SETTINGS MAX_HEADER_LIST_SIZE limit, then HpackEncoder throws a HeaderListSizeException. Consequently, the request is not sent, and the client receives a PrematureCloseException error without any details.

Let's just add more specific root cause to the PrematureCloseException in order to make it meaningful in the case the request header size exceeds MAX_HEADER_LIST_SIZE (or if whatever Netty Http2 Codec exceptions are caught while writing the request out).

Example:

reactor.netty.http.client.PrematureCloseException: Connection prematurely closed BEFORE response
Caused by: io.netty.handler.codec.http2.Http2Exception$HeaderListSizeException: Header size exceeded max allowed size (1024)

Fixes #2927

@pderop pderop added the type/enhancement A general enhancement label Oct 17, 2023
@pderop pderop added this to the 1.0.39 milestone Oct 17, 2023
@pderop pderop self-assigned this Oct 17, 2023
@pderop
Copy link
Member Author

pderop commented Oct 17, 2023

@violetagg , can you take a look ? I don't think the failures from macos-native and windows-2029 are related.

PS: I have set the label to enhancement because this is not a BUG fix per se, but I however have targeted the PR to 1.0.x because I think this simple improvement is worth it.

thanks.

@pderop pderop requested a review from violetagg October 17, 2023 14:13
Copy link
Member

@violetagg violetagg left a comment

Choose a reason for hiding this comment

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

Thanks

@pderop pderop merged commit 0e28135 into reactor:1.0.x Oct 18, 2023
7 of 9 checks passed
@pderop
Copy link
Member Author

pderop commented Oct 18, 2023

@violetagg , thanks for your review.

@pderop pderop deleted the issue-2927 branch October 18, 2023 14:58
pderop added a commit that referenced this pull request Oct 18, 2023
pderop added a commit that referenced this pull request Oct 18, 2023
@pderop
Copy link
Member Author

pderop commented Oct 18, 2023

The Merge done in the netty5 branch is not working, and needs to be completed with another PR, and for the moment, the testIssue2927_H2C and testIssue2927 tests are disabled in netty5 branch.

The problem in netty5 is that when the HeaderListSizeException is thrown, then:

  1. first, the HttpClientOperations.onInboundClose() is invoked -> here, no unprocessedOutboundError, so the PrematureCloseException is logged without the root cause exception.
  2. and right after, the ChannelOperations.onError(Throwable t) method is called .. but this too late, because the HttpClientOperations.onInboundClose() method has already logged the PrematureCloseExeption without the HeaderListSizeException root cause exception.

@violetagg violetagg changed the title Add root cause exception when throwing PrematureCloseException Add root cause exception when throwing PrematureCloseException Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PrematureCloseException from exceeding header limit after H1 upgrade to H2
2 participants