From 04b0e785f6b9b4629b77bb19f2b072434be4951c Mon Sep 17 00:00:00 2001 From: Daniel Fuchs Date: Tue, 30 May 2023 16:32:11 +0000 Subject: [PATCH] 8307648: java/net/httpclient/ExpectContinueTest.java timed out Reviewed-by: djelinski --- .../classes/jdk/internal/net/http/Stream.java | 34 ++++++++++++--- .../net/httpclient/ExpectContinueTest.java | 41 ++++++++++++++++--- .../test/lib/http2/Http2TestExchangeImpl.java | 3 ++ 3 files changed, 68 insertions(+), 10 deletions(-) diff --git a/src/java.net.http/share/classes/jdk/internal/net/http/Stream.java b/src/java.net.http/share/classes/jdk/internal/net/http/Stream.java index 4d3421d4b6e..fa00ae1949c 100644 --- a/src/java.net.http/share/classes/jdk/internal/net/http/Stream.java +++ b/src/java.net.http/share/classes/jdk/internal/net/http/Stream.java @@ -475,7 +475,7 @@ class Stream extends ExchangeImpl { if ((frame instanceof HeaderFrame hf)) { if (hf.endHeaders()) { Log.logTrace("handling response (streamid={0})", streamid); - handleResponse(); + handleResponse(hf); } if (hf.getFlag(HeaderFrame.END_STREAM)) { if (debug.on()) debug.log("handling END_STREAM: %d", streamid); @@ -511,7 +511,7 @@ class Stream extends ExchangeImpl { return rspHeadersConsumer::onDecoded; } - protected void handleResponse() throws IOException { + protected void handleResponse(HeaderFrame hf) throws IOException { HttpHeaders responseHeaders = responseHeadersBuilder.build(); if (!finalResponseCodeReceived) { @@ -519,8 +519,20 @@ class Stream extends ExchangeImpl { .firstValueAsLong(":status") .orElseThrow(() -> new IOException("no statuscode in response")); // If informational code, response is partially complete - if (responseCode < 100 || responseCode > 199) + if (responseCode < 100 || responseCode > 199) { this.finalResponseCodeReceived = true; + } else if (hf.getFlag(HeaderFrame.END_STREAM)) { + // see RFC 9113 section 8.1: + // A HEADERS frame with the END_STREAM flag set that carries an + // informational status code is malformed + String msg = ("Stream %s PROTOCOL_ERROR: " + + "HEADERS frame with status %s has END_STREAM flag set") + .formatted(streamid, responseCode); + if (debug.on()) { + debug.log(msg); + } + cancelImpl(new IOException(msg), ResetFrame.PROTOCOL_ERROR); + } response = new Response( request, exchange, responseHeaders, connection(), @@ -567,12 +579,20 @@ class Stream extends ExchangeImpl { // response to be read before the Reset is handled in the case where the client's // input stream is partially consumed or not consumed at all by the server. if (frame.getErrorCode() != ResetFrame.NO_ERROR) { + if (debug.on()) { + debug.log("completing requestBodyCF exceptionally due to received" + + " RESET(%s) (stream=%s)", frame.getErrorCode(), streamid); + } requestBodyCF.completeExceptionally(new IOException("RST_STREAM received")); } else { + if (debug.on()) { + debug.log("completing requestBodyCF normally due to received" + + " RESET(NO_ERROR) (stream=%s)", streamid); + } requestBodyCF.complete(null); } } - if (response == null && subscriber == null) { + if ((response == null || !finalResponseCodeReceived) && subscriber == null) { // we haven't received the headers yet, and won't receive any! // handle reset now. handleReset(frame, null); @@ -950,6 +970,10 @@ class Stream extends ExchangeImpl { private void onNextImpl(ByteBuffer item) { // Got some more request body bytes to send. if (requestBodyCF.isDone()) { + if (debug.on()) { + debug.log("RequestSubscriber: requestBodyCf is done: " + + "cancelling subscription"); + } // stream already cancelled, probably in timeout sendScheduler.stop(); subscription.cancel(); @@ -1528,7 +1552,7 @@ class Stream extends ExchangeImpl { // create and return the PushResponseImpl @Override - protected void handleResponse() { + protected void handleResponse(HeaderFrame hf) { HttpHeaders responseHeaders = responseHeadersBuilder.build(); if (!finalPushResponseCodeReceived) { diff --git a/test/jdk/java/net/httpclient/ExpectContinueTest.java b/test/jdk/java/net/httpclient/ExpectContinueTest.java index ef0cb319593..838a5fbd046 100644 --- a/test/jdk/java/net/httpclient/ExpectContinueTest.java +++ b/test/jdk/java/net/httpclient/ExpectContinueTest.java @@ -28,7 +28,7 @@ * @summary Tests that when the httpclient sends a 100 Expect Continue header and receives * a response code of 417 Expectation Failed, that the client does not hang * indefinitely and closes the connection. - * @bug 8286171 + * @bug 8286171 8307648 * @library /test/lib /test/jdk/java/net/httpclient/lib * @build jdk.httpclient.test.lib.common.HttpServerAdapters * @run testng/othervm -Djdk.internal.httpclient.debug=err ExpectContinueTest @@ -56,6 +56,7 @@ import java.net.ServerSocket; import java.net.Socket; import java.net.URI; import java.net.http.HttpClient; +import java.net.http.HttpClient.Builder; import java.net.http.HttpRequest; import java.net.http.HttpResponse; import java.util.StringTokenizer; @@ -106,6 +107,10 @@ public class ExpectContinueTest implements HttpServerAdapters { h2postUri = URI.create("http://" + http2TestServer.serverAuthority() + "/http2/post"); h2hangUri = URI.create("http://" + http2TestServer.serverAuthority() + "/http2/hang"); + System.out.println("HTTP/1.1 server listening at: " + http1TestServer.serverAuthority()); + System.out.println("HTTP/1.1 hang server listening at: " + hangUri.getRawAuthority()); + System.out.println("HTTP/2 clear server listening at: " + http2TestServer.serverAuthority()); + http1TestServer.start(); http1HangServer.start(); http2TestServer.start(); @@ -124,8 +129,10 @@ public class ExpectContinueTest implements HttpServerAdapters { public void handle(HttpTestExchange exchange) throws IOException { try (InputStream is = exchange.getRequestBody(); OutputStream os = exchange.getResponseBody()) { + System.err.println("Server reading body"); is.readAllBytes(); byte[] bytes = "RESPONSE_BODY".getBytes(UTF_8); + System.err.println("Server sending 200 (length="+bytes.length+")"); exchange.sendResponseHeaders(200, bytes.length); os.write(bytes); } @@ -139,13 +146,16 @@ public class ExpectContinueTest implements HttpServerAdapters { // Http1 server has already sent 100 response at this point but not Http2 server if (exchange.getExchangeVersion().equals(HttpClient.Version.HTTP_2)) { // Send 100 Headers, tell client that we're ready for body + System.err.println("Server sending 100 (length = 0)"); exchange.sendResponseHeaders(100, 0); } // Read body from client and acknowledge with 200 try (InputStream is = exchange.getRequestBody(); OutputStream os = exchange.getResponseBody()) { + System.err.println("Server reading body"); is.readAllBytes(); + System.err.println("Server send 200 (length=0)"); exchange.sendResponseHeaders(200, 0); } } @@ -159,6 +169,7 @@ public class ExpectContinueTest implements HttpServerAdapters { try (InputStream is = exchange.getRequestBody(); OutputStream os = exchange.getResponseBody()) { byte[] bytes = EXPECTATION_FAILED_417.getBytes(); + System.err.println("Server send 417 (length="+bytes.length+")"); exchange.sendResponseHeaders(417, bytes.length); os.write(bytes); } @@ -184,11 +195,14 @@ public class ExpectContinueTest implements HttpServerAdapters { public void run() { byte[] bytes = EXPECTATION_FAILED_417.getBytes(); + boolean closed = this.closed; while (!closed) { try { // Not using try with resources here as we expect the client to close resources when // 417 is received - client = ss.accept(); + System.err.println("Http1HangServer accepting connections"); + var client = this.client = ss.accept(); + System.err.println("Http1HangServer accepted connection: " + client); InputStream is = client.getInputStream(); OutputStream os = client.getOutputStream(); @@ -213,7 +227,8 @@ public class ExpectContinueTest implements HttpServerAdapters { && version.equals("HTTP/1.1"); // If correct request, send 417 reply. Otherwise, wait for correct one if (validRequest) { - closed = true; + System.err.println("Http1HangServer sending 417"); + closed = this.closed = true; response.append("HTTP/1.1 417 Expectation Failed\r\n") .append("Content-Length: ") .append(0) @@ -224,17 +239,25 @@ public class ExpectContinueTest implements HttpServerAdapters { os.write(bytes); os.flush(); } else { + System.err.println("Http1HangServer received invalid request: closing"); client.close(); } } catch (IOException e) { - closed = true; + closed = this.closed = true; e.printStackTrace(); + } finally { + if (closed = this.closed) { + System.err.println("Http1HangServer: finished"); + } else { + System.err.println("Http1HangServer: looping for accepting next connection"); + } } } } @Override public void close() throws IOException { + var client = this.client; if (client != null) client.close(); if (ss != null) ss.close(); } @@ -244,13 +267,15 @@ public class ExpectContinueTest implements HttpServerAdapters { public Object[][] urisData() { return new Object[][]{ { getUri, postUri, hangUri, HTTP_1_1 }, - { h2getUri, h2postUri, h2hangUri, HttpClient.Version.HTTP_2 } + { h2getUri, h2postUri, h2hangUri, HTTP_2 } }; } @Test(dataProvider = "uris") public void test(URI getUri, URI postUri, URI hangUri, HttpClient.Version version) throws IOException, InterruptedException { + System.out.println("Testing with version: " + version); HttpClient client = HttpClient.newBuilder() + .proxy(Builder.NO_PROXY) .version(version) .build(); @@ -268,18 +293,24 @@ public class ExpectContinueTest implements HttpServerAdapters { .expectContinue(true) .build(); + System.out.printf("Sending request (%s): %s%n", version, getRequest); + System.err.println("Sending request: " + getRequest); CompletableFuture> cf = client.sendAsync(getRequest, HttpResponse.BodyHandlers.ofString()); HttpResponse resp = cf.join(); System.err.println("Response Headers: " + resp.headers()); System.err.println("Response Status Code: " + resp.statusCode()); assertEquals(resp.statusCode(), 200); + System.out.printf("Sending request (%s): %s%n", version, postRequest); + System.err.println("Sending request: " + postRequest); cf = client.sendAsync(postRequest, HttpResponse.BodyHandlers.ofString()); resp = cf.join(); System.err.println("Response Headers: " + resp.headers()); System.err.println("Response Status Code: " + resp.statusCode()); assertEquals(resp.statusCode(), 200); + System.out.printf("Sending request (%s): %s%n", version, hangRequest); + System.err.println("Sending request: " + hangRequest); cf = client.sendAsync(hangRequest, HttpResponse.BodyHandlers.ofString()); resp = cf.join(); System.err.println("Response Headers: " + resp.headers()); diff --git a/test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Http2TestExchangeImpl.java b/test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Http2TestExchangeImpl.java index be52e4856ed..28ea572a018 100644 --- a/test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Http2TestExchangeImpl.java +++ b/test/jdk/java/net/httpclient/lib/jdk/httpclient/test/lib/http2/Http2TestExchangeImpl.java @@ -133,6 +133,9 @@ public class Http2TestExchangeImpl implements Http2TestExchange { @Override public void sendResponseHeaders(int rCode, long responseLength) throws IOException { + // Do not set Content-Length for 100, and do not set END_STREAM + if (rCode == 100) responseLength = 0; + this.responseLength = responseLength; if (responseLength !=0 && rCode != 204 && !isHeadRequest()) { long clen = responseLength > 0 ? responseLength : 0;