8379549: [lworld] sun/security/ssl/SSLSocketImpl/SSLSocketSSLEngineCloseInbound.java failed with SocketException: Connection reset by peer#30340
Conversation
…oseInbound.java failed with SocketException: Connection reset by peer
|
👋 Welcome back hchao! A progress list of the required criteria for merging this PR into |
|
@haimaychao This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 13 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@haimaychao 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. |
dcubed-ojdk
left a comment
dcubed-ojdk
left a comment
There was a problem hiding this comment.
I like the use of CountDownLatch instead of sleep!
Thumbs up.
| /* | ||
| * Enables logging of the SSL/TLS operations. | ||
| */ | ||
| private static final boolean logging = true; |
There was a problem hiding this comment.
Since this logging is always set to true I personally don't think this should be even included. What do you think about removing it and always logging?
There was a problem hiding this comment.
I have no objections to the additional configuration options. (i.e. leaving as is.)
| * including specific handshake messages, and might be best examined | ||
| * after gaining some familiarity with this application. | ||
| */ | ||
| private static final boolean debug = false; |
There was a problem hiding this comment.
How abut making the ssl debug option as a part of @run? It is always false as well which means that it does nothing atm.
If you believe it's needed, could you please change it so it takes a boolean system property test.debug? Like this:
Boolean.getBoolean("test.debug");There was a problem hiding this comment.
There is no need to output the ssl debugging information by default, it just adds lots of extra output that is usually never looked at. If there is a problem with this test, developers can quickly toggle the switch to get the needed debug info.
We just turned the default JSSE debug output off in many unneeded test cases. (JDK-8368493) No need to turn it back on.
During the review of JDK-8368493, there was a suggestion to update all of the JSSE test cases to use test.debug. We could consider that, but let's do that as a unified chunk of work, rather than 1-offs here.
Thanks.
| * read() ... ChangeCipherSpec | ||
| * read() ... Finished | ||
| */ | ||
| import javax.net.ssl.*; |
| } | ||
|
|
||
| // Server signals client it has finished writing | ||
| serverReadyLatch.countDown(); |
There was a problem hiding this comment.
minor: what if exceptions are thrown, would the test just time out? Wouldn't it make sense to release the latch on the exceptions too?
There was a problem hiding this comment.
Or in the finally block?
| public void run() { | ||
| // client-side socket | ||
| try (SSLSocket sslSocket = (SSLSocket)sslc.getSocketFactory(). | ||
| createSocket("localhost", port)) { |
There was a problem hiding this comment.
Could you please change this to loopback address since you are touching the file?
|
|
||
| // server-side socket that will read | ||
| try (Socket socket = serverSocket.accept()) { | ||
| socket.setSoTimeout(500); |
There was a problem hiding this comment.
| socket.setSoTimeout((int)Utils.adjustTimeout(500)); |
bradfordwetmore
left a comment
bradfordwetmore
left a comment
There was a problem hiding this comment.
A few comments to previous comments, but otherwise looks ok to me.
| } | ||
|
|
||
| // Server signals client it has finished writing | ||
| serverReadyLatch.countDown(); |
There was a problem hiding this comment.
Or in the finally block?
| /* | ||
| * Enables logging of the SSL/TLS operations. | ||
| */ | ||
| private static final boolean logging = true; |
There was a problem hiding this comment.
I have no objections to the additional configuration options. (i.e. leaving as is.)
| * including specific handshake messages, and might be best examined | ||
| * after gaining some familiarity with this application. | ||
| */ | ||
| private static final boolean debug = false; |
There was a problem hiding this comment.
There is no need to output the ssl debugging information by default, it just adds lots of extra output that is usually never looked at. If there is a problem with this test, developers can quickly toggle the switch to get the needed debug info.
We just turned the default JSSE debug output off in many unneeded test cases. (JDK-8368493) No need to turn it back on.
During the review of JDK-8368493, there was a suggestion to update all of the JSSE test cases to use test.debug. We could consider that, but let's do that as a unified chunk of work, rather than 1-offs here.
Thanks.
4 participants
The SSLSocketSSLEngineCloseInbound.java test runs with different protocols, namely TLSv1.3, TLSv1.2, TLSv1.1, TLSv1, and TLS. The test log file shows that it completed successfully for the first four protocols, but it failed when running with protocol=TLS. Protocol TLS is resolved to TLSv1.3. The failure in the test is a timing-related issue, that server continues with write() after the client has already closed its side of the connection. This change removes the race condition by synchronizing the client and server with CountDownLatch.
Have validated the change to ensure the test to pass (with JTREG=REPEAT_COUNT=50).
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30340/head:pull/30340$ git checkout pull/30340Update a local copy of the PR:
$ git checkout pull/30340$ git pull https://git.openjdk.org/jdk.git pull/30340/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30340View PR using the GUI difftool:
$ git pr show -t 30340Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30340.diff
Using Webrev
Link to Webrev Comment