HTTP2 GET request sends an empty DATA frame where ReactorClientHttpRequest is involved

[asw12]

Expected Behavior

With HttpClientHandler#handler populated (as is the case with spring-web version 6.2.0 when using reactor-netty for the RestClient, this would be org.springframework.http.client.ReactorClientHttpRequest), when sending a GET request with HTTP2, the reactor-netty client should send a HEADERS frame with the END_STREAM flag set, and no DATA frame for most standard requests.

Actual Behavior

The GET request results in an HTTP2 stream where the HEADERS frame does not have the END_STREAM flag set, and there is a subsequent empty DATA frame that follows.

This is in violation of RFC9113, which does not allow for DATA frames to follow:
https://datatracker.ietf.org/doc/html/rfc9113#name-simple-request

Steps to Reproduce

Set a breakpoint on io.netty.handler.codec.http2.DefaultHttp2DataFrame, then make a request:

@Autowired
RestClient.Builder builder;
...
RestClient client = builder.get()
    .uri(endpoint) // arbitrary endpoint that supports H2
    .retrieve()
    .body(String.class);

"reactor-http-nio-4@7834" daemon prio=5 tid=0x21 nid=NA runnable
  java.lang.Thread.State: RUNNABLE
	  at io.netty.handler.codec.http2.DefaultHttp2DataFrame.<init>(DefaultHttp2DataFrame.java:72)
	  at io.netty.handler.codec.http2.DefaultHttp2DataFrame.<init>(DefaultHttp2DataFrame.java:60)
	  at io.netty.handler.codec.http2.Http2StreamFrameToHttpObjectCodec.encodeLastContent(Http2StreamFrameToHttpObjectCodec.java:131)
	  at io.netty.handler.codec.http2.Http2StreamFrameToHttpObjectCodec.encode(Http2StreamFrameToHttpObjectCodec.java:186)
	  at io.netty.handler.codec.http2.Http2StreamFrameToHttpObjectCodec.encode(Http2StreamFrameToHttpObjectCodec.java:59)
	  at io.netty.handler.codec.MessageToMessageCodec$2.encode(MessageToMessageCodec.java:85)
	  at io.netty.handler.codec.MessageToMessageEncoder.write(MessageToMessageEncoder.java:90)
	  at io.netty.handler.codec.MessageToMessageCodec.write(MessageToMessageCodec.java:130)
	  at io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:891)
	  at io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:875)
	  at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:984)
	  at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:868)

I would not expect this breakpoint to be hit, because no DATA frame should be sent.

Possible Solution

In my study of this problem, I'm interpreting this as a reactor-netty bug rather than a spring-web defect. Even though this technically works (no GET DATA frames) when using HttpClient by itself (as say, in the Http2Tests), I don't see a viable way for spring-web's ReactorClientHttpRequest to tap into this correct default behaviour.

Where this branching point between working / non-working behaviour lies is in

reactor-netty/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientConnect.java

Line 594 in 9a5a672

return handler != null ? handler.apply(ch, ch) : ch.send();

In the working case, the package-protected ch.send() method is called, which attempts to mark the headers and body as set simultaneously

reactor-netty/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientOperations.java

Lines 891 to 898 in 9a5a672

	final Mono<Void> send() {
	if (!channel().isActive()) {
	return Mono.error(AbortedException.beforeSend());
	}
	return FutureMono.deferFuture(() -> markSentHeaderAndBody() ?
	channel().writeAndFlush(newFullBodyMessage(Unpooled.EMPTY_BUFFER)) :
	channel().newSucceededFuture());
	}

In the other case (handler != null), in calling handler.apply(ch, ch), it's essentially up to ReactorClientHttpRequest to find this same default behaviour, where we have:

"reactor-http-nio-2@7832" daemon prio=5 tid=0x1f nid=NA runnable
  java.lang.Thread.State: RUNNABLE
	  at org.springframework.http.client.ReactorClientHttpRequest.send(ReactorClientHttpRequest.java:148)
	  at org.springframework.http.client.ReactorClientHttpRequest.lambda$executeInternal$0(ReactorClientHttpRequest.java:124)
	  at org.springframework.http.client.ReactorClientHttpRequest$$Lambda$989/0x0000000801141418.apply(Unknown Source:-1)
	  at reactor.netty.http.client.HttpClientConnect$HttpClientHandler.requestWithBody(HttpClientConnect.java:594)
	  at reactor.netty.http.client.HttpClientConnect$HttpIOHandlerObserver.lambda$onStateChange$0(HttpClientConnect.java:434)

https://github.com/spring-projects/spring-framework/blob/5024bb72279188f1d0dcfe19e8abdd4bdb9887c8/spring-web/src/main/java/org/springframework/http/client/ReactorClientHttpRequest.java#L142-L149

As this basically returns the ch as-is, this results in the ch used as a subscriber, which eventually leads to HttpOperations#then() being called. This "default" behaviour has different semantics (it checks for the "Content-Length" header, which doesn't really apply to HTTP2), resulting in a DefaultHttpRequest being generated rather than a DefaultFullHttpRequest.

I would think that the intent of ReactorClientHttpRequest's body == null ... return outbound logic is to try and reuse the default behaviour, which in this case would be .send() rather than .then(). But send is a protected method, and so the ReactorClientHttpRequest over in spring-web can't really access this -- this is why I see this as a reactor-netty bug rather than a spring-web bug.

At that point, would it be reasonable to suggest that reactor-netty take what's in the HttpClientOperations.send(), and change it to be an @Override for then()? I'm not sure I understand if there's a reason for there to be two "defaults", in a sense, here.

Your Environment

reactor-netty 1.2.0
spring-web 6.2.0
spring-boot 3.4.0's out-of-the-box configuration, with the only exception being

@Bean
ClientHttpRequestFactoryBuilder<?> clientHttpRequestFactoryBuilder() {
	return ClientHttpRequestFactoryBuilder.reactor()
			.withHttpClientCustomizer(httpClient -> httpClient
					.protocol(HttpProtocol.HTTP11, HttpProtocol.H2));
}

to enable the H2 protocol for the autoconfigured RestClient.Builder.

[violetagg]

@asw12 Thanks for the detailed description and the provided test case! This will be fixed in the next release.