Skip to content

Commit

Permalink
AbstractNettyHttpServerTest payload validation bug (#1173)
Browse files Browse the repository at this point in the history 
Motivation:
AbstractNettyHttpServerTest attempts to validate that the payload is
read in the same boundaries that it was written. This is not guaranteed
and the payload maybe chunked differently than written.

Modifications:
- Verify the aggregated payload is received as expected so we are
agnostic to intermediate patching.

Result:
More reliable verification of payload contents.
  • Loading branch information
@Scottmitch
Scottmitch authored Oct 10, 2020
1 parent da277d1 commit c00c1b2
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 51 deletions.
8 changes: 8 additions & 0 deletions servicetalk-http-netty/gradle/spotbugs/test-exclusions.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,12 @@
</Or>
<Bug pattern="RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE"/>
</Match>
<!-- false positive in Java 11, see https://github.com/spotbugs/spotbugs/issues/756 -->
<Match>
<Class name="io.servicetalk.http.netty.StreamObserverTest"/>
<Or>
<Method name="maxActiveStreamsViolationError" />
</Or>
<Bug pattern="RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE"/>
</Match>
</FindBugsFilter>
21 changes: 6 additions & 15 deletions ...etalk-http-netty/src/test/java/io/servicetalk/http/netty/AbstractNettyHttpServerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@

import java.net.InetSocketAddress;
import java.net.StandardSocketOptions;
import java.util.List;
import java.util.concurrent.ExecutionException;
import java.util.function.Function;
import java.util.function.Supplier;
Expand All @@ -79,10 +78,8 @@
import static java.nio.charset.StandardCharsets.US_ASCII;
import static java.util.Objects.requireNonNull;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assume.assumeThat;

public abstract class AbstractNettyHttpServerTest {
Expand Down Expand Up @@ -258,16 +255,15 @@ void assertResponse(final StreamingHttpResponse response, final HttpProtocolVers
}

void assertResponse(final StreamingHttpResponse response, final HttpProtocolVersion version,
final HttpResponseStatus status, final List<String> expectedPayloadChunksAsStrings)
final HttpResponseStatus status, final String expectedPayload)
throws ExecutionException, InterruptedException {
assertEquals(status, response.status());
assertEquals(version, response.version());
final List<String> bodyAsListOfStrings = getBodyAsListOfStrings(response);
if (expectedPayloadChunksAsStrings.isEmpty()) {
assertTrue(bodyAsListOfStrings.isEmpty());
} else {
assertThat(bodyAsListOfStrings, contains(expectedPayloadChunksAsStrings.toArray()));
}
String actualPayload = response.payloadBody().collect(StringBuilder::new, (sb, chunk) -> {
sb.append(chunk.toString(US_ASCII));
return sb;
}).toFuture().get().toString();
assertThat(actualPayload, is(expectedPayload));
}

Publisher<Buffer> getChunkPublisherFromStrings(final String... texts) {
Expand All @@ -282,11 +278,6 @@ Buffer getChunkFromString(final String text) {
return DEFAULT_ALLOCATOR.fromAscii(text);
}

static List<String> getBodyAsListOfStrings(final StreamingHttpResponse response)
throws ExecutionException, InterruptedException {
return awaitIndefinitelyNonNull(response.payloadBody().map(c -> c.toString(US_ASCII)));
}

static <T> T awaitSingleIndefinitelyNonNull(Single<T> single) {
try {
return requireNonNull(single.toFuture().get());
Expand Down
3 changes: 1 addition & 2 deletions ...-netty/src/test/java/io/servicetalk/http/netty/NettyHttpServerConnectionAcceptorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
import static io.servicetalk.http.netty.AbstractNettyHttpServerTest.ExecutorSupplier.CACHED;
import static io.servicetalk.http.netty.AbstractNettyHttpServerTest.ExecutorSupplier.IMMEDIATE;
import static io.servicetalk.http.netty.TestServiceStreaming.SVC_ECHO;
import static java.util.Collections.singletonList;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.instanceOf;
Expand Down Expand Up @@ -140,7 +139,7 @@ public void testAcceptConnection() throws Exception {
getChunkPublisherFromStrings("hello"));
request.headers().set(CONTENT_LENGTH, "5");
final StreamingHttpResponse response = makeRequest(request);
assertResponse(response, HTTP_1_1, OK, singletonList("hello"));
assertResponse(response, HTTP_1_1, OK, "hello");
if (!filterMode.expectAccept) {
throw new AssertionError("Expected filter to reject connection");
}
Expand Down
66 changes: 32 additions & 34 deletions servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/NettyHttpServerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,6 @@
import static io.servicetalk.http.netty.TestServiceStreaming.SVC_THROW_ERROR;
import static java.nio.charset.StandardCharsets.US_ASCII;
import static java.util.Arrays.asList;
import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
import static org.hamcrest.Matchers.is;
Expand Down Expand Up @@ -125,14 +123,14 @@ public static Collection<ExecutorSupplier[]> clientExecutors() {
public void testGetNoRequestPayloadWithoutResponseLastChunk() throws Exception {
final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_COUNTER_NO_LAST_CHUNK);
final StreamingHttpResponse response = makeRequest(request);
assertResponse(response, HTTP_1_1, OK, singletonList("Testing1\n"));
assertResponse(response, HTTP_1_1, OK, "Testing1\n");
}

@Test
public void testGetNoRequestPayload() throws Exception {
final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_COUNTER);
final StreamingHttpResponse response = makeRequest(request);
assertResponse(response, HTTP_1_1, OK, singletonList("Testing1\n"));
assertResponse(response, HTTP_1_1, OK, "Testing1\n");
}

@Test
Expand All @@ -145,15 +143,15 @@ public void testGetEchoPayloadContentLength() throws Exception {
});
request.headers().set(CONTENT_LENGTH, "5");
final StreamingHttpResponse response = makeRequest(request);
assertResponse(response, HTTP_1_1, OK, singletonList("hello"));
assertResponse(response, HTTP_1_1, OK, "hello");
}

@Test
public void testGetEchoPayloadChunked() throws Exception {
final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_ECHO).payloadBody(
getChunkPublisherFromStrings("hello"));
final StreamingHttpResponse response = makeRequest(request);
assertResponse(response, HTTP_1_1, OK, singletonList("hello"));
assertResponse(response, HTTP_1_1, OK, "hello");
}

@Test
Expand All @@ -162,44 +160,44 @@ public void testGetRot13Payload() throws Exception {
getChunkPublisherFromStrings("hello"));
request.headers().set(CONTENT_LENGTH, "5");
final StreamingHttpResponse response = makeRequest(request);
assertResponse(response, HTTP_1_1, OK, singletonList("uryyb"));
assertResponse(response, HTTP_1_1, OK, "uryyb");
}

@Test
public void testGetIgnoreRequestPayload() throws Exception {
final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_COUNTER).payloadBody(
getChunkPublisherFromStrings("hello"));
final StreamingHttpResponse response = makeRequest(request);
assertResponse(response, HTTP_1_1, OK, singletonList("Testing1\n"));
assertResponse(response, HTTP_1_1, OK, "Testing1\n");
}

@Test
public void testGetNoRequestPayloadNoResponsePayload() throws Exception {
final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_NO_CONTENT);
final StreamingHttpResponse response = makeRequest(request);
assertResponse(response, HTTP_1_1, NO_CONTENT, emptyList());
assertResponse(response, HTTP_1_1, NO_CONTENT, "");
}

@Test
public void testMultipleGetsNoRequestPayloadWithoutResponseLastChunk() throws Exception {
final StreamingHttpRequest request1 = reqRespFactory.newRequest(GET, SVC_COUNTER_NO_LAST_CHUNK);
final StreamingHttpResponse response1 = makeRequest(request1);
assertResponse(response1, HTTP_1_1, OK, singletonList("Testing1\n"));
assertResponse(response1, HTTP_1_1, OK, "Testing1\n");

final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_COUNTER_NO_LAST_CHUNK);
final StreamingHttpResponse response2 = makeRequest(request2);
assertResponse(response2, HTTP_1_1, OK, singletonList("Testing2\n"));
assertResponse(response2, HTTP_1_1, OK, "Testing2\n");
}

@Test
public void testMultipleGetsNoRequestPayload() throws Exception {
final StreamingHttpRequest request1 = reqRespFactory.newRequest(GET, SVC_COUNTER);
final StreamingHttpResponse response1 = makeRequest(request1);
assertResponse(response1, HTTP_1_1, OK, singletonList("Testing1\n"));
assertResponse(response1, HTTP_1_1, OK, "Testing1\n");

final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_COUNTER);
final StreamingHttpResponse response2 = makeRequest(request2);
assertResponse(response2, HTTP_1_1, OK, singletonList("Testing2\n"));
assertResponse(response2, HTTP_1_1, OK, "Testing2\n");
}

@Test
Expand All @@ -208,26 +206,26 @@ public void testMultipleGetsEchoPayloadContentLength() throws Exception {
getChunkPublisherFromStrings("hello"));
request1.headers().set(CONTENT_LENGTH, "5");
final StreamingHttpResponse response1 = makeRequest(request1);
assertResponse(response1, HTTP_1_1, OK, singletonList("hello"));
assertResponse(response1, HTTP_1_1, OK, "hello");

final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_ECHO).payloadBody(
getChunkPublisherFromStrings("hello"));
request2.headers().set(CONTENT_LENGTH, "5");
final StreamingHttpResponse response2 = makeRequest(request2);
assertResponse(response2, HTTP_1_1, OK, singletonList("hello"));
assertResponse(response2, HTTP_1_1, OK, "hello");
}

@Test
public void testMultipleGetsEchoPayloadChunked() throws Exception {
final StreamingHttpRequest request1 = reqRespFactory.newRequest(GET, SVC_ECHO).payloadBody(
getChunkPublisherFromStrings("hello"));
final StreamingHttpResponse response1 = makeRequest(request1);
assertResponse(response1, HTTP_1_1, OK, singletonList("hello"));
assertResponse(response1, HTTP_1_1, OK, "hello");

final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_ECHO).payloadBody(
getChunkPublisherFromStrings("hello"));
final StreamingHttpResponse response2 = makeRequest(request2);
assertResponse(response2, HTTP_1_1, OK, singletonList("hello"));
assertResponse(response2, HTTP_1_1, OK, "hello");
}

@Test
Expand All @@ -236,20 +234,20 @@ public void testMultipleGetsIgnoreRequestPayload() throws Exception {
getChunkPublisherFromStrings("hello"));
request1.headers().set(CONTENT_LENGTH, "5");
final StreamingHttpResponse response1 = makeRequest(request1);
assertResponse(response1, HTTP_1_1, OK, singletonList("Testing1\n"));
assertResponse(response1, HTTP_1_1, OK, "Testing1\n");

final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_COUNTER).payloadBody(
getChunkPublisherFromStrings("hello"));
request2.headers().set(CONTENT_LENGTH, "5");
final StreamingHttpResponse response2 = makeRequest(request2);
assertResponse(response2, HTTP_1_1, OK, singletonList("Testing2\n"));
assertResponse(response2, HTTP_1_1, OK, "Testing2\n");
}

@Test
public void testHttp10CloseConnection() throws Exception {
final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_COUNTER).version(HTTP_1_0);
final StreamingHttpResponse response = makeRequest(request);
assertResponse(response, HTTP_1_0, OK, singletonList("Testing1\n"));
assertResponse(response, HTTP_1_0, OK, "Testing1\n");
assertFalse(response.headers().contains(CONNECTION));

assertConnectionClosed();
Expand All @@ -260,13 +258,13 @@ public void testHttp10KeepAliveConnection() throws Exception {
final StreamingHttpRequest request1 = reqRespFactory.newRequest(GET, SVC_COUNTER).version(HTTP_1_0);
request1.headers().set("connection", "keep-alive");
final StreamingHttpResponse response1 = makeRequest(request1);
assertResponse(response1, HTTP_1_0, OK, singletonList("Testing1\n"));
assertResponse(response1, HTTP_1_0, OK, "Testing1\n");
assertTrue(response1.headers().contains(CONNECTION, KEEP_ALIVE));

final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_COUNTER).version(HTTP_1_0);
request2.headers().set("connection", "keep-alive");
final StreamingHttpResponse response2 = makeRequest(request2);
assertResponse(response2, HTTP_1_0, OK, singletonList("Testing2\n"));
assertResponse(response2, HTTP_1_0, OK, "Testing2\n");
assertTrue(response1.headers().contains(CONNECTION, KEEP_ALIVE));
}

Expand All @@ -275,7 +273,7 @@ public void testHttp11CloseConnection() throws Exception {
final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_COUNTER);
request.headers().set("connection", "close");
final StreamingHttpResponse response = makeRequest(request);
assertResponse(response, HTTP_1_1, OK, singletonList("Testing1\n"));
assertResponse(response, HTTP_1_1, OK, "Testing1\n");
assertTrue(response.headers().contains(CONNECTION, CLOSE));

assertConnectionClosed();
Expand All @@ -285,12 +283,12 @@ public void testHttp11CloseConnection() throws Exception {
public void testHttp11KeepAliveConnection() throws Exception {
final StreamingHttpRequest request1 = reqRespFactory.newRequest(GET, SVC_COUNTER);
final StreamingHttpResponse response1 = makeRequest(request1);
assertResponse(response1, HTTP_1_1, OK, singletonList("Testing1\n"));
assertResponse(response1, HTTP_1_1, OK, "Testing1\n");
assertFalse(response1.headers().contains(CONNECTION));

final StreamingHttpRequest request2 = reqRespFactory.newRequest(GET, SVC_COUNTER);
final StreamingHttpResponse response2 = makeRequest(request2);
assertResponse(response2, HTTP_1_1, OK, singletonList("Testing2\n"));
assertResponse(response2, HTTP_1_1, OK, "Testing2\n");
assertFalse(response2.headers().contains(CONNECTION));
}

Expand All @@ -299,7 +297,7 @@ public void testHttp11KeepAliveConnection() throws Exception {
public void testGracefulShutdownWhileIdle() throws Exception {
final StreamingHttpRequest request1 = reqRespFactory.newRequest(GET, SVC_COUNTER);
final StreamingHttpResponse response1 = makeRequest(request1);
assertResponse(response1, HTTP_1_1, OK, singletonList("Testing1\n"));
assertResponse(response1, HTTP_1_1, OK, "Testing1\n");
assertFalse(response1.headers().contains(CONNECTION));

// Use a very high timeout for the graceful close. It should happen quite quickly because there are no
Expand All @@ -323,7 +321,7 @@ public void testGracefulShutdownWhileReadingPayload() throws Exception {
publisher.onNext(getChunkFromString("Hello"));
publisher.onComplete();

assertResponse(response1, HTTP_1_1, OK, singletonList("Hello"));
assertResponse(response1, HTTP_1_1, OK, "Hello");
assertFalse(response1.headers().contains(CONNECTION)); // Eventually this should be assertTrue

assertConnectionClosed();
Expand Down Expand Up @@ -434,15 +432,15 @@ public void testDeferCloseConnection() throws Exception {
public void testSynchronousError() throws Exception {
final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_THROW_ERROR);
final StreamingHttpResponse response = makeRequest(request);
assertResponse(response, HTTP_1_1, INTERNAL_SERVER_ERROR, emptyList());
assertResponse(response, HTTP_1_1, INTERNAL_SERVER_ERROR, "");
assertTrue(response.headers().contains(CONTENT_LENGTH, ZERO));
}

@Test
public void testSingleError() throws Exception {
final StreamingHttpRequest request = reqRespFactory.newRequest(GET, SVC_SINGLE_ERROR);
final StreamingHttpResponse response = makeRequest(request);
assertResponse(response, HTTP_1_1, INTERNAL_SERVER_ERROR, emptyList());
assertResponse(response, HTTP_1_1, INTERNAL_SERVER_ERROR, "");
assertTrue(response.headers().contains(CONTENT_LENGTH, ZERO));
}

Expand Down Expand Up @@ -489,20 +487,20 @@ public void testErrorDuringRead() throws Exception {
assertEquals(HTTP_1_1, response.version());

final BlockingIterator<Buffer> httpPayloadChunks = response.payloadBody().toIterable().iterator();
assertEquals("Goodbye", httpPayloadChunks.next().toString(US_ASCII));
assertEquals("cruel", httpPayloadChunks.next().toString(US_ASCII));
assertEquals("world!", httpPayloadChunks.next().toString(US_ASCII));

StringBuilder sb = new StringBuilder();
// Due to a race condition, the exception cause here can vary.
// If the socket closure is delayed slightly (for example, by delaying the Publisher.error(...) on the server)
// then the client throws ClosedChannelException. However if the socket closure happens quickly enough,
// the client throws NativeIoException (KQueue) or IOException (NIO).
try {
httpPayloadChunks.next();
while (httpPayloadChunks.hasNext()) {
sb.append(httpPayloadChunks.next().toString(US_ASCII));
}
fail("Server should close upon receiving the request");
} catch (RuntimeException wrapped) { // BlockingIterator wraps
assertClientTransportInboundClosed(wrapped.getCause());
}
assertEquals("Goodbyecruelworld!", sb.toString());
assertConnectionClosed();
// Client inbound channel closed - should be same exception as above
Throwable clientThrowable = ((NettyConnectionContext) streamingHttpConnection().connectionContext())
Expand Down

0 comments on commit c00c1b2

Please sign in to comment.