Skip to content

Commit

Permalink
Ensure CL/TE headers are handled correctly for 204/205/304 status cod…
Browse files Browse the repository at this point in the history 
…es when server configured with HTTP/2 (#2623)

Fixes #2620
  • Loading branch information
@violetagg
violetagg authored Jan 3, 2023
1 parent d762bc4 commit fcda69a
Show file tree
Hide file tree
Showing 6 changed files with 163 additions and 78 deletions.
15 changes: 11 additions & 4 deletions reactor-netty-http/src/main/java/reactor/netty/http/HttpOperations.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011-2022 VMware, Inc. or its affiliates, All Rights Reserved.
* Copyright (c) 2011-2023 VMware, Inc. or its affiliates, All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -125,10 +125,11 @@ public NettyOutbound send(Publisher<? extends ByteBuf> source) {
ReferenceCountUtil.release(b);
return Mono.error(e);
}
if (HttpUtil.getContentLength(outboundHttpMessage(), -1) == 0) {
if (HttpUtil.getContentLength(outboundHttpMessage(), -1) == 0 ||
isContentAlwaysEmpty()) {
if (log.isDebugEnabled()) {
log.debug(format(channel(), "Dropped HTTP content, " +
"since response has Content-Length: 0 {}"), b);
log.debug(format(channel(), "Dropped HTTP content, since response has " +
"1. [Content-Length: 0] or 2. there must be no content: {}"), b);
}
b.release();
return FutureMono.from(channel().writeAndFlush(newFullBodyMessage(Unpooled.EMPTY_BUFFER)));
Expand Down Expand Up @@ -200,6 +201,10 @@ public Mono<Void> then() {
msg = outboundHttpMessage();
}
}
else if (isContentAlwaysEmpty()) {
markSentBody();
msg = newFullBodyMessage(Unpooled.EMPTY_BUFFER);
}
else {
msg = outboundHttpMessage();
}
Expand Down Expand Up @@ -236,6 +241,8 @@ protected HttpMessageLogFactory httpMessageLogFactory() {

protected abstract void afterMarkSentHeaders();

protected abstract boolean isContentAlwaysEmpty();

protected abstract void onHeadersSent();

protected abstract HttpMessage newFullBodyMessage(ByteBuf body);
Expand Down
7 changes: 6 additions & 1 deletion reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientOperations.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011-2022 VMware, Inc. or its affiliates, All Rights Reserved.
* Copyright (c) 2011-2023 VMware, Inc. or its affiliates, All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -537,6 +537,11 @@ protected void beforeMarkSentHeaders() {
}
}

@Override
protected boolean isContentAlwaysEmpty() {
return false;
}

@Override
protected void onHeadersSent() {
channel().read();
Expand Down
18 changes: 12 additions & 6 deletions reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011-2022 VMware, Inc. or its affiliates, All Rights Reserved.
* Copyright (c) 2011-2023 VMware, Inc. or its affiliates, All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -211,7 +211,9 @@ protected HttpMessage newFullBodyMessage(ByteBuf body) {

if (!HttpMethod.HEAD.equals(method())) {
responseHeaders.remove(HttpHeaderNames.TRANSFER_ENCODING);
if (!HttpResponseStatus.NOT_MODIFIED.equals(status())) {
int code = status().code();
if (!(HttpResponseStatus.NOT_MODIFIED.code() == code ||
HttpResponseStatus.NO_CONTENT.code() == code)) {

if (HttpUtil.getContentLength(nettyResponse, -1) == -1) {
responseHeaders.setInt(HttpHeaderNames.CONTENT_LENGTH, body.readableBytes());
Expand Down Expand Up @@ -637,10 +639,6 @@ protected void onInboundClose() {

@Override
protected void afterMarkSentHeaders() {
if (HttpResponseStatus.NOT_MODIFIED.equals(status())) {
responseHeaders.remove(HttpHeaderNames.TRANSFER_ENCODING)
.remove(HttpHeaderNames.CONTENT_LENGTH);
}
if (compressionPredicate != null && compressionPredicate.test(this, this)) {
compression(true);
}
Expand All @@ -651,6 +649,14 @@ protected void beforeMarkSentHeaders() {
//noop
}

@Override
protected boolean isContentAlwaysEmpty() {
int code = status().code();
return HttpResponseStatus.NOT_MODIFIED.code() == code ||
HttpResponseStatus.NO_CONTENT.code() == code ||
HttpResponseStatus.RESET_CONTENT.code() == code;
}

@Override
protected void onHeadersSent() {
//noop
Expand Down
10 changes: 7 additions & 3 deletions reactor-netty-http/src/main/java/reactor/netty/http/server/HttpTrafficHandler.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011-2022 VMware, Inc. or its affiliates, All Rights Reserved.
* Copyright (c) 2011-2023 VMware, Inc. or its affiliates, All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -466,16 +466,20 @@ boolean shouldKeepAlive() {
*/
static boolean isSelfDefinedMessageLength(HttpResponse response) {
return isContentLengthSet(response) || isTransferEncodingChunked(response) || isMultipart(
response) || isInformational(response) || isNotModified(response);
response) || isInformational(response) || isNotModified(response) || isNoContent(response);
}

static boolean isInformational(HttpResponse response) {
return response.status()
.codeClass() == HttpStatusClass.INFORMATIONAL;
}

static boolean isNoContent(HttpResponse response) {
return HttpResponseStatus.NO_CONTENT.code() == response.status().code();
}

static boolean isNotModified(HttpResponse response) {
return HttpResponseStatus.NOT_MODIFIED.equals(response.status());
return HttpResponseStatus.NOT_MODIFIED.code() == response.status().code();
}

static boolean isMultipart(HttpResponse response) {
Expand Down
127 changes: 126 additions & 1 deletion ...tor-netty-http/src/test/java/reactor/netty/http/HttpResponseStatusCodesHandlingTests.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2017-2021 VMware, Inc. or its affiliates, All Rights Reserved.
* Copyright (c) 2017-2023 VMware, Inc. or its affiliates, All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -15,15 +15,28 @@
*/
package reactor.netty.http;

import io.netty.handler.codec.http.HttpHeaderNames;
import io.netty.handler.codec.http.HttpHeaders;
import io.netty.handler.codec.http.HttpMethod;
import io.netty.handler.codec.http.HttpResponseStatus;
import io.netty.handler.ssl.util.InsecureTrustManagerFactory;
import io.netty.handler.ssl.util.SelfSignedCertificate;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import reactor.core.publisher.Flux;
import reactor.core.publisher.Mono;
import reactor.netty.BaseHttpTest;
import reactor.netty.ByteBufFlux;
import reactor.netty.http.client.HttpClient;
import reactor.netty.http.server.HttpServer;
import reactor.test.StepVerifier;

import java.time.Duration;
import java.util.Arrays;
import java.util.stream.Stream;

/**
* @author Violeta Georgieva
*/
Expand Down Expand Up @@ -51,4 +64,116 @@ void httpStatusCode404IsHandledByTheClient() {
.expectNext(404)
.verifyComplete();
}

@ParameterizedTest
@MethodSource("httpCompatibleCombinations")
void noContentStatusCodes(HttpProtocol[] serverProtocols, HttpProtocol[] clientProtocols) throws Exception {
SelfSignedCertificate ssc = new SelfSignedCertificate();

HttpServer server = createServer().protocol(serverProtocols);
if (Arrays.asList(serverProtocols).contains(HttpProtocol.H2)) {
Http2SslContextSpec serverCtx = Http2SslContextSpec.forServer(ssc.certificate(), ssc.privateKey());
server = server.secure(spec -> spec.sslContext(serverCtx));
}

disposableServer =
server.host("localhost")
.route(r -> r.get("/204-1", (req, res) -> res.status(HttpResponseStatus.NO_CONTENT)
.sendHeaders())
.get("/204-2", (req, res) -> res.status(HttpResponseStatus.NO_CONTENT))
.get("/204-3", (req, res) -> res.status(HttpResponseStatus.NO_CONTENT)
.sendString(Mono.just("/204-3")))
.get("/204-4", (req, res) -> res.status(HttpResponseStatus.NO_CONTENT)
.sendString(Flux.just("/", "204-4")))
.get("/204-5", (req, res) -> res.status(HttpResponseStatus.NO_CONTENT)
.send())
.get("/205-1", (req, res) -> res.status(HttpResponseStatus.RESET_CONTENT)
.sendHeaders())
.get("/205-2", (req, res) -> res.status(HttpResponseStatus.RESET_CONTENT))
.get("/205-3", (req, res) -> res.status(HttpResponseStatus.RESET_CONTENT)
.sendString(Mono.just("/205-3")))
.get("/205-4", (req, res) -> res.status(HttpResponseStatus.RESET_CONTENT)
.sendString(Flux.just("/", "205-4")))
.get("/205-5", (req, res) -> res.status(HttpResponseStatus.RESET_CONTENT)
.send())
.get("/304-1", (req, res) -> res.status(HttpResponseStatus.NOT_MODIFIED)
.sendHeaders())
.get("/304-2", (req, res) -> res.status(HttpResponseStatus.NOT_MODIFIED))
.get("/304-3", (req, res) -> res.status(HttpResponseStatus.NOT_MODIFIED)
.sendString(Mono.just("/304-3")))
.get("/304-4", (req, res) -> res.status(HttpResponseStatus.NOT_MODIFIED)
.sendString(Flux.just("/", "304-4")))
.get("/304-5", (req, res) -> res.status(HttpResponseStatus.NOT_MODIFIED)
.send()))
.bindNow();

HttpClient client = createClient(disposableServer::address).protocol(clientProtocols);
if (Arrays.asList(clientProtocols).contains(HttpProtocol.H2)) {
Http2SslContextSpec clientCtx =
Http2SslContextSpec.forClient()
.configure(builder -> builder.trustManager(InsecureTrustManagerFactory.INSTANCE));
client = client.secure(spec -> spec.sslContext(clientCtx));
}

checkResponse("/204-1", client);
checkResponse("/204-2", client);
checkResponse("/204-3", client);
checkResponse("/204-4", client);
checkResponse("/204-5", client);
checkResponse("/205-1", client);
checkResponse("/205-2", client);
checkResponse("/205-3", client);
checkResponse("/205-4", client);
checkResponse("/205-5", client);
checkResponse("/304-1", client);
checkResponse("/304-2", client);
checkResponse("/304-3", client);
checkResponse("/304-4", client);
checkResponse("/304-5", client);
}

static void checkResponse(String url, HttpClient client) {
client.get()
.uri(url)
.responseSingle((r, buf) ->
Mono.zip(Mono.just(r.status().code()),
Mono.just(r.responseHeaders()),
buf.asString().defaultIfEmpty("NO BODY")))
.as(StepVerifier::create)
.expectNextMatches(t -> {
int code = t.getT1();
HttpHeaders h = t.getT2();
if (code == HttpResponseStatus.NO_CONTENT.code() ||
code == HttpResponseStatus.NOT_MODIFIED.code()) {
return !h.contains(HttpHeaderNames.TRANSFER_ENCODING) &&
!h.contains(HttpHeaderNames.CONTENT_LENGTH) &&
"NO BODY".equals(t.getT3());
}
else if (code == HttpResponseStatus.RESET_CONTENT.code()) {
return !h.contains(HttpHeaderNames.TRANSFER_ENCODING) &&
h.getInt(HttpHeaderNames.CONTENT_LENGTH).equals(0) &&
"NO BODY".equals(t.getT3());
}
else {
return false;
}
})
.expectComplete()
.verify(Duration.ofSeconds(30));
}

static Stream<Arguments> httpCompatibleCombinations() {
return Stream.of(
Arguments.of(new HttpProtocol[]{HttpProtocol.HTTP11}, new HttpProtocol[]{HttpProtocol.HTTP11}),

Arguments.of(new HttpProtocol[]{HttpProtocol.H2}, new HttpProtocol[]{HttpProtocol.H2}),
Arguments.of(new HttpProtocol[]{HttpProtocol.H2}, new HttpProtocol[]{HttpProtocol.H2, HttpProtocol.HTTP11}),
Arguments.of(new HttpProtocol[]{HttpProtocol.H2, HttpProtocol.HTTP11}, new HttpProtocol[]{HttpProtocol.H2}),
Arguments.of(new HttpProtocol[]{HttpProtocol.H2, HttpProtocol.HTTP11}, new HttpProtocol[]{HttpProtocol.H2, HttpProtocol.HTTP11}),

Arguments.of(new HttpProtocol[]{HttpProtocol.H2C}, new HttpProtocol[]{HttpProtocol.H2C}),
Arguments.of(new HttpProtocol[]{HttpProtocol.H2C, HttpProtocol.HTTP11}, new HttpProtocol[]{HttpProtocol.H2C}),
Arguments.of(new HttpProtocol[]{HttpProtocol.H2C, HttpProtocol.HTTP11}, new HttpProtocol[]{HttpProtocol.H2C, HttpProtocol.HTTP11})
);
}
}
64 changes: 1 addition & 63 deletions reactor-netty-http/src/test/java/reactor/netty/http/server/HttpServerTests.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011-2022 VMware, Inc. or its affiliates, All Rights Reserved.
* Copyright (c) 2011-2023 VMware, Inc. or its affiliates, All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -136,7 +136,6 @@
import reactor.util.annotation.Nullable;
import reactor.util.context.Context;
import reactor.util.function.Tuple2;
import reactor.util.function.Tuple3;

import javax.net.ssl.SNIHostName;

Expand Down Expand Up @@ -551,67 +550,6 @@ void startRouterAndAwait() throws InterruptedException {
assertThat(f.isDone()).isTrue();
}

@Test
void nonContentStatusCodes() {
disposableServer =
createServer()
.host("localhost")
.route(r -> r.get("/204-1", (req, res) -> res.status(HttpResponseStatus.NO_CONTENT)
.sendHeaders())
.get("/204-2", (req, res) -> res.status(HttpResponseStatus.NO_CONTENT))
.get("/205-1", (req, res) -> res.status(HttpResponseStatus.RESET_CONTENT)
.sendHeaders())
.get("/205-2", (req, res) -> res.status(HttpResponseStatus.RESET_CONTENT))
.get("/304-1", (req, res) -> res.status(HttpResponseStatus.NOT_MODIFIED)
.sendHeaders())
.get("/304-2", (req, res) -> res.status(HttpResponseStatus.NOT_MODIFIED))
.get("/304-3", (req, res) -> res.status(HttpResponseStatus.NOT_MODIFIED)
.send()))
.bindNow();

InetSocketAddress address = (InetSocketAddress) disposableServer.address();
checkResponse("/204-1", address);
checkResponse("/204-2", address);
checkResponse("/205-1", address);
checkResponse("/205-2", address);
checkResponse("/304-1", address);
checkResponse("/304-2", address);
checkResponse("/304-3", address);
}

private void checkResponse(String url, InetSocketAddress address) {
Mono<Tuple3<Integer, HttpHeaders, String>> response =
createClient(() -> address)
.get()
.uri(url)
.responseSingle((r, buf) ->
Mono.zip(Mono.just(r.status().code()),
Mono.just(r.responseHeaders()),
buf.asString().defaultIfEmpty("NO BODY"))
);

StepVerifier.create(response)
.expectNextMatches(t -> {
int code = t.getT1();
HttpHeaders h = t.getT2();
if (code == 204 || code == 304) {
return !h.contains("Transfer-Encoding") &&
!h.contains("Content-Length") &&
"NO BODY".equals(t.getT3());
}
else if (code == 205) {
return !h.contains("Transfer-Encoding") &&
h.getInt("Content-Length").equals(0) &&
"NO BODY".equals(t.getT3());
}
else {
return false;
}
})
.expectComplete()
.verify(Duration.ofSeconds(30));
}

@Test
void testContentLengthHeadRequest() {
AtomicReference<HttpHeaders> sentHeaders = new AtomicReference<>();
Expand Down

0 comments on commit fcda69a

Please sign in to comment.