8268965: TCP Connection Reset when connecting simple socket to SSL server #4520
Conversation
|
👋 Welcome back abakhtin! A progress list of the required criteria for merging this PR into |
|
@alexeybakhtin 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. |
Webrevs
|
|
I would like to have a look. I was wondering if there is a racing between close and read and if the closure could be hang. But I have no time to think more about them currently. Hopefully, I could have some cycles in the next couple of weeks. |
|
Could I have more details about the problem? For example, the thread stacks for the socket reset or the thread stacks of the call to close() method. The test case does not call close() method of the socket or the I/O stream. Will the reset happen if the socket and I/O call the close() methods properly? |
|
Hi Xuelei, In case of proposed patch applied, the client fails with SSL server closes the socket during the handshake, so no changes if we try to close the socket from the application |
|
Hi Alexey, Thank you for the details thread stacks. The basic idea of the fix looks good to me. But I did have a concern about the racing. If there is a read thread and a close/shutdown thread, there might be weird behaviors because each thread may only be able to read some of the information and make it impossible to decode the record. What do you think If placing the cleanup synchronized with readLock? For example, using the AppInputStream.deplete() method or a similar one that synch the reading. Best, |
|
Hi Xuelei, |
| // Try to clear the kernel buffer to avoid TCP connection resets. | ||
| if (conContext.inputRecord instanceof SSLSocketInputRecord && | ||
| isConnected) { | ||
| appInput.readLock.lock(); | ||
| try { | ||
| ((SSLSocketInputRecord) (conContext.inputRecord)).deplete(false); | ||
| } finally { | ||
| appInput.readLock.unlock(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The blocking on close() may be not good in practice. I would use tryLock() rather than lock() so as to avoid closure blocking. tryLock() is not perfect, but it may be better than blocking the close().
BTW, you could use the intanceof pattern matching so as to avoid the cast (See https://openjdk.java.net/jeps/394).
if (conContext.inputRecord instanceof
SSLSocketInputRecord inputRecord && isConnected) {
if (appInput.readLock.tryLock()) {
try {
inputRecord.deplete(false);
} finally {
appInput.readLock.unlock();
}
}
}
There was a problem hiding this comment.
Hi Xuelei,
Thank you a lot again. Replaced lock with tryLock as you suggested
|
@alexeybakhtin 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 298 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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@XueleiFan) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
/integrate |
|
@alexeybakhtin |
|
/sponsor |
|
Going to push as commit 6f171b9.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
|
@VladimirKempik @alexeybakhtin Pushed as commit 6f171b9. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review the fix for JDK-8268965.
The new jtreg test is added for the described issue.
sun/security/ssl and javax/net/ssl tests are passed
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4520/head:pull/4520$ git checkout pull/4520Update a local copy of the PR:
$ git checkout pull/4520$ git pull https://git.openjdk.java.net/jdk pull/4520/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4520View PR using the GUI difftool:
$ git pr show -t 4520Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4520.diff