RFR [13] JDK-8224829 : AsyncSSLSocketClose.java has timing issue

[Xuelei Fan]

Hi Daniel,

All good points.  Here is the update webrev:
   http://cr.openjdk.java.net/~xuelei/8224829/webrev.01/

Thanks,
Xuelei

On 6/12/2019 8:19 AM, Daniel Fuchs wrote:
> Hi Xuelei,
> 
> I am not an expert of the field so please get another
> reviewer for this change:
> 
>   656                     if (!super.isOutputShutdown() &&
>                           ...
>   661                     } else if (!super.isOutputShutdown()) {
> 
> I think it might be clearer with a nested if:
> 
>     if (!super.isOutputShutdown()) {
>         if (isLayered() && !autoClose) {
>             throw exception ...
>         } else {
>             super.shutdownOutput();
>             log ...
>         }
>     }
> 
> Could lines 630 .. 647 be moved to a private method?
> Something like:
> 
>       private void handleClosedNotifyAlert() {
>   630     try {
>               // send a user_canceled alert if needed.
>               if (useUserCanceled) {
>                   conContext.warning(Alert.USER_CANCELED);
>               }
> 
>               // send a close_notify alert
>               conContext.warning(Alert.CLOSE_NOTIFY);
>           } finally {
>               if (!conContext.isOutboundClosed()) {
>                   conContext.outputRecord.close();
>               }
> 
>               if (!super.isOutputShutdown() &&
>                       (autoClose || !isLayered())) {
>                   super.shutdownOutput();
>   647         }
>            }
>        }
> 
> because if I'm not mistaken this exact same piece of code
> appears at lines 700 - 717.
> 
> I believe this would simplify this long method a little bit,
> and possibly makes what is happening here clearer:
> 
>   623     if (linger >= 0) {
>              // don't wait more than SO_LINGER for obtaining the
>              // the lock...
>               ...
>   626         try {
>   627             if (conContext.outputRecord.recordLock.tryLock() ||
>   628                     conContext.outputRecord.recordLock.tryLock(
>   629                             linger, TimeUnit.SECONDS)) {
>                       try {
>                           handleClosedNotifyAlert();
>                       } finally {
>                           conContext.outputRecord.recordLock.unlock();
>                       }
>   652             } else {
>   653                 // For layered, non-autoclose sockets, we are not
>   654                 // able to bring them into a usable state, so we
>   655                 // treat it as fatal error.
>                       ...
>   688             }
>   689         } catch (InterruptedException ex) {
>                 ...
>   692         }
>   693
>   694         // restore the interrupted status
>               ...
>   698     } else {
>   699         conContext.outputRecord.recordLock.lock();
>   700         try {
>                   handleClosedNotifyAlert();
>               } finally {
>                   conContext.outputRecord.recordLock.unlock();
>               }
>            }
>            ...
> 
> 
> BlockedAsyncClose.java:
> 
>     Can you bind the server to the loopback instead of the
>     wildcard? My experience is that it make tests more reliable.
> 
>               InetAddress loopback = InetAddress.getLoopbackAddress();
>    74         ss = (SSLServerSocket) sslssf.createServerSocket();
>               ss.bind(new InetSosketAddress(loopback, 0));
>    75
>    76         SSLSocketFactory sslsf =
>    77             (SSLSocketFactory)SSLSocketFactory.getDefault();
>               socket = (SSLSocket)sslsf.createSocket(loopback, 
> ss.getLocalPort());
> 
> 
> best regards,
> 
> -- daniel
> 
> On 12/06/2019 14:51, Xuelei Fan wrote:
>> ping ...
>>
>> On 6/4/2019 11:10 AM, Xuelei Fan wrote:
>>> Hi,
>>>
>>> Could I get the following update reviewed?
>>>     http://cr.openjdk.java.net/~xuelei/8224829/webrev.00/
>>>
>>> If using one thread for closing, one thread for writing.  Closing and 
>>> writing threads are synchronized in order to delivery close_notify 
>>> TLS record.  There are could be race between the two threads.  If the 
>>> output stream buffer reach the limit, the write will be blocked, and 
>>> then the close thread may be pending.
>>>
>>> The SO_LINGER is used to resolve the issue for general socket.  With 
>>> the above update, the SSLSocket implementation will check the 
>>> SO_LINGER as well.  If SO_LINGER is on, the SSLSocket close will try 
>>> to get the synchronization lock in the time of SO_LINGER value.  If 
>>> timeout, the close process will move on, and close the underlying 
>>> socket by force.
>>>
>>> Thanks,
>>> Xuelei
>