RFR: 8274524: SSLSocket.close() hangs if it is called during the ssl handshake

[Xue-Lei Andrew Fan]

On Fri, 11 Feb 2022 09:27:11 GMT, Alexey Bakhtin <abakhtin at openjdk.org> wrote:

>> src/java.base/share/classes/sun/security/ssl/SSLSocketImpl.java line 1792:
>> 
>>> 1790:                     try {
>>> 1791:                         if (soTimeout == 0)
>>> 1792:                             setSoTimeout(DEFAULT_SKIP_TIMEOUT);
>> 
>> This set will impact the socket overall behavior unexpectedly, not just the close() method.
>> 
>> Maybe, an input stream level synchronization is missed in the SSLSocketInputRecord method, so that logic of deplete could be interrupted.
>
> Hi @XueleiFan,
> Not quite sure What could be wrong with setting SO_TIMEOUT during the socketClose() operation. We still close the socket and SO_TIMEOUT will be restored just after skip() operation is completed. These changes could affect the parallel read of the handshake records (we hold appInput.readLock during the skip) but we still close the socket and interrupt connection in this case.
> If you still think these changes are incorrect, What do you think about the most first version of the patch: https://github.com/openjdk/jdk/pull/7432/commits/d1c2a4fe23916ff0f1c38150bc0ea1c9c38fe39b
> This version adds a new lock in the SSLSocketInputRecord and protects the parallel execution of the read and skip operations.

If I understand the issue correctly, the problem is about that the skip method cannot get the right length if the input stream could be accessed in another thread.

1. get the available input stream bytes length;
2. a third thread read the input stream, and the input stream get changed;
3. skip up to the bytes length in step 1.
4. the skip() method hangs on waiting for more bytes.

I think we could focus on on address the synchronization problem, because the problem could result in other weird behaviors we don't know yet.

For the SO_TIMEOUT, I think it is a good workaround.  But I'm not sure if it will impact other behaviors or not.  Besides, I know you are trying to use a small timeout, but it is still blocked and it not easy to find a number fit all situation that does not impact the performance.

As I commented in the 1st version, I'm not sure of the locks logic in the patch, which changes the behavior of SSLSocket.

I may suggest to an input stream level synchronization.  I know the update could take a while, and may not be able to  integrate in time.  If you want to fix the issue as soon as possible, I'm OK to move ahead with your current direction, but please set the SO_TIMEOUT to as minimal as possible, for example 1 ms; and have a comment that this is a temporary/workaround solution.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7432