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

[Daniel Fuchs]

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