On Thu, 26 Jun 2025 10:48:56 GMT, Darragh Clarke <dcla...@openjdk.org> wrote:
> Currently if a request has set Expect-Continue and receives a non 100 > response the `responseMessage` wouldn't be set. > > This PR sets `responseMessage`, it also updates `getResponseMessage` to check > if the message has already been set. This should match the way that > `responseCode` is currently handled. > > I also added a test to cover some possible responses. test/jdk/java/net/httpclient/ExpectContinueResponseMessageTest.java line 98: > 96: while (!control.stop) { > 97: try { > 98: Socket socket = control.serverSocket.accept(); So this socket will not be closed... I understand closing it here might cause a reset... Is that the reason for the catch clause on the client side? test/jdk/java/net/httpclient/ExpectContinueResponseMessageTest.java line 124: > 122: //send a wrong response > 123: outputStream.write(control.statusLine.getBytes()); > 124: outputStream.flush(); If you added Content-Length: 0 the the status line (which should be better called response BTW) then you should be able to at least call `socket.shutdownOutput()` here. test/jdk/java/net/httpclient/ExpectContinueResponseMessageTest.java line 149: > 147: String body = "Testing: " + expectedCode; > 148: Control control = this.control; > 149: control.statusLine = statusLine + "\r\n\r\n"; Shouldn't you add Content-Length: 0 here? test/jdk/java/net/httpclient/ExpectContinueResponseMessageTest.java line 159: > 157: } catch (Exception ex) { > 158: // swallow the exception > 159: } @DarraghClarke - I am curious, is this catch clause necessary? If it is maybe you could improve the comment here to explain why it is necessary. test/jdk/java/net/httpclient/ExpectContinueResponseMessageTest.java line 167: > 165: assertTrue(expectedMessage.equals(responseMessage), > 166: String.format("Expected Response Message %s, instead > received %s", > 167: expectedMessage, responseMessage)); maybe the connection should be explicitely closed at the end. maybe the accepted socket should be added to Control and closed here too? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25999#discussion_r2168831749 PR Review Comment: https://git.openjdk.org/jdk/pull/25999#discussion_r2168849333 PR Review Comment: https://git.openjdk.org/jdk/pull/25999#discussion_r2168833905 PR Review Comment: https://git.openjdk.org/jdk/pull/25999#discussion_r2168825892 PR Review Comment: https://git.openjdk.org/jdk/pull/25999#discussion_r2168844935