8352502: Response message is null if expect 100 assertion fails with non 100 #25999
Conversation
|
👋 Welcome back dclarke! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@DarraghClarke The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
| } catch (SocketException e) { | ||
| // ignore | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
This exception will be swallowed, it will have no effect.
| } catch (Exception ex) { | ||
| // swallow the exception | ||
| } |
There was a problem hiding this comment.
@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.
| Runnable runnable = () -> { | ||
| while (!control.stop) { | ||
| try { | ||
| Socket socket = control.serverSocket.accept(); |
There was a problem hiding this comment.
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?
| public void test(int expectedCode, String statusLine, String expectedMessage) throws Exception { | ||
| String body = "Testing: " + expectedCode; | ||
| Control control = this.control; | ||
| control.statusLine = statusLine + "\r\n\r\n"; |
There was a problem hiding this comment.
Shouldn't you add Content-Length: 0 here?
| String.format("Expected %s response, instead received %s", expectedCode, responseCode)); | ||
| assertTrue(expectedMessage.equals(responseMessage), | ||
| String.format("Expected Response Message %s, instead received %s", | ||
| expectedMessage, responseMessage)); |
There was a problem hiding this comment.
maybe the connection should be explicitely closed at the end.
maybe the accepted socket should be added to Control and closed here too?
|
|
||
| //send a wrong response | ||
| outputStream.write(control.statusLine.getBytes()); | ||
| outputStream.flush(); |
There was a problem hiding this comment.
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.
Currently if a request has set Expect-Continue and receives a non 100 response the
responseMessagewouldn't be set.This PR sets
responseMessage, it also updatesgetResponseMessageto check if the message has already been set. This should match the way thatresponseCodeis currently handled.I also added a test to cover some possible responses.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25999/head:pull/25999$ git checkout pull/25999Update a local copy of the PR:
$ git checkout pull/25999$ git pull https://git.openjdk.org/jdk.git pull/25999/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25999View PR using the GUI difftool:
$ git pr show -t 25999Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25999.diff
Using Webrev
Link to Webrev Comment