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

HttpServerRequest::receiveContent() never emits any value nor completes when HTTP/1.1 TLS Upgrade (RFC-2817) kicks in #3538

Closed
reta opened this issue Dec 9, 2024 · 3 comments · Fixed by #3540
Assignees
Labels
type/bug A general bug
Milestone

Comments

@reta
Copy link
Contributor

reta commented Dec 9, 2024

We have run into the issue with latest Apache HttpClient5 5.4.1 / Apache HttpCore5 5.3.1 as a client (Reactor Netty as the HTTP server with HTTP/1.1 and H2C protocols) that now tries HTTP/1.1 TLS Upgrade (RFC-2817) by default, see please [1] for context. The HttpServerRequest::receiveContent() never emits any value nor completes since no next / completion callbacks are called.

I have traced this problem to HttpServerOperations, specifically to this block of code https://github.com/reactor/reactor-netty/blob/main/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java#L812:

                      if (isFullHttpRequest) {
				FullHttpRequest request = (FullHttpRequest) msg;
				if (request.content().readableBytes() > 0) {
					super.onInboundNext(ctx, msg);
				}
				else {
					request.release();
				}
				if (isHttp2()) {
					//force auto read to enable more accurate close selection now inbound is done
					channel().config().setAutoRead(true);
					onInboundComplete();
				}
			}

In case of upgrade flow (which happens only for GET/OPTIONS/HEAD), the channel pipeline involves HttpObjectAggregator and emits the FullHttpRequest instance (precisely, HttpObjectAggregator.AggregatedFullHttpRequest) with empty content, which deviates from non-upgrade flow that emits DefaultHttpRequest, followed by last content. As the result, onInboundNext is not called, and because the upgrade did not switch protocols and stayed on HTTP/1.1, onInboundComplete is also not called.

[1] https://github.com/apache/httpcomponents-client/pull/542/files

Expected Behavior

The HttpServerRequest::receiveContent() should complete with empty result.

Actual Behavior

The HttpServerRequest::receiveContent() hangs.

Steps to Reproduce

I have published a full reproducer here [2], it is very small but has pom.xml + client + server (easier to start with).

[2] https://github.com/reta/httpclient5-upgrade-reproducer

Possible Solution

I assume that calling onInboundComplete() would be the right thing to do here, since there is no content and inbound request is processed.

Your Environment

  • Reactor version(s) used: 1.2.24
  • Other relevant libraries versions (eg. netty, ...): Apache HttpClient5 5.4.1, Apache HttpCore5 5.3.1
  • JVM version (java -version): 11 / 17 / 21
  • OS and version (eg. uname -a): any
@reta reta added status/need-triage A new issue that still need to be evaluated as a whole type/bug A general bug labels Dec 9, 2024
@reta reta changed the title `HttpServerRequest::receiveContent() never emits any value nor completes when HTTP/1.1 TLS Upgrade (RFC-2817) kicks in HttpServerRequest::receiveContent() never emits any value nor completes when HTTP/1.1 TLS Upgrade (RFC-2817) kicks in Dec 9, 2024
@reta
Copy link
Contributor Author

reta commented Dec 9, 2024

@violetagg I have an idea how to fix the problem, but would really appreciate the guidance if it makes sense, happy to help in any case. Thank you.

@violetagg violetagg self-assigned this Dec 10, 2024
@violetagg violetagg removed the status/need-triage A new issue that still need to be evaluated as a whole label Dec 10, 2024
@violetagg
Copy link
Member

@reta I think your analysis is correct. Would you like to provide a PR?

@reta
Copy link
Contributor Author

reta commented Dec 10, 2024

@reta I think your analysis is correct. Would you like to provide a PR?

Absolutely, on it, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug
Projects
None yet
2 participants