On Fri, 11 Feb 2022 09:27:11 GMT, Alexey Bakhtin <abakh...@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
