[RFR] 8184328: JDK8u131 socketRead0 hang at SSL read

[Rob McKenna]

On 15/09/17 07:07, Xuelei Fan wrote:
> On 9/15/2017 7:00 AM, Rob McKenna wrote:
> >When we call close() on the SSLSocket that calls close() on the
> >underlying java Socket which closes the native socket.
> >
> Sorry, I did not get the point.  Please see the close() implementation of
> SSLSocket (sun.security.ssl.SSLSocketImpl.close()) about the details.

Running my original test against an instrumented 8u-dev produces the
following:

java.lang.Exception: Stack trace
	at java.lang.Thread.dumpStack(Thread.java:1336)
	at java.net.Socket.close(Socket.java:1491)
	at sun.security.ssl.BaseSSLSocketImpl.close(BaseSSLSocketImpl.java:624)
	at sun.security.ssl.SSLSocketImpl.closeSocket(SSLSocketImpl.java:1579)
	at sun.security.ssl.SSLSocketImpl.fatal(SSLSocketImpl.java:1980)
	at sun.security.ssl.SSLSocketImpl.waitForClose(SSLSocketImpl.java:1793)
	at sun.security.ssl.SSLSocketImpl.closeSocket(SSLSocketImpl.java:1592)
	at sun.security.ssl.SSLSocketImpl.closeInternal(SSLSocketImpl.java:1726)
	at sun.security.ssl.SSLSocketImpl.close(SSLSocketImpl.java:1615)
	at ssl.SSLClient.close(SSLClient.java:143)
	at ssl.SocketTimeoutCloseHang.ReadHang.testSSLServer(ReadHang.java:77)

    -Rob

> 
> Xuelei
> 
> >     -Rob
> >
> >On 13/09/17 04:09, Xuelei Fan wrote:
> >>It's a little bit complicated for layered SSL connections.  Application can
> >>build a SSL connection on existing socket (we call it layered SSL
> >>connections).  The problem scenarios make look like:
> >>1. open a socket for applications.
> >>2. established a SSL connection on the existing socket.
> >>3. close the SSL connection, but leaving data in the socket.
> >>4. establish another SSL connection on the socket, as the existing data in
> >>the socket, the connection cannot be established.
> >>5. establish another app connection on the socket, as the existing data in
> >>the socket, the connection cannot be established.
> >>....
> >>
> >>Timeout happens even on very high speed network. If a timeout happens and
> >>the SSL connection is not closed gracefully, and then the following
> >>applications breaks.  IMHO, we need to take care of the case.
> >>
> >>Xuelei
> >>
> >>On 9/13/2017 1:06 PM, Chris Hegarty wrote:
> >>>Xuelei,
> >>>
> >>>Without diving deeper into this issue, Rob’s suggested approach seems reasonable to me, and better than existing out-of-the-box behaviour. I’m not sure what issues you are thinking of, with using the read timeout in combination with a retry mechanism, in this manner? If the network is so slow, surely there will be other issues with connecting and reading, why is closing any different.
> >>>
> >>>-Chris.
> >>>
> >>>>On 13 Sep 2017, at 16:52, Rob McKenna <rob.mckenna at oracle.com> wrote:
> >>>>
> >>>>Hi Xuelei,
> >>>>
> >>>>This behaviour is already exposed via the autoclose boolean in:
> >>>>
> >>>>https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLSocketFactory.html#createSocket-java.net.Socket-java.io.InputStream-boolean-
> >>>>
> >>>>My position would be that allowing 5 retries allows us to say with some
> >>>>confidence that we're not going to get a close_notify from the server.
> >>>>If this is the case I think its reasonable to close the connection.
> >>>>
> >>>>W.r.t. a separate timeout, the underlying mechanics of a close already
> >>>>depend on the readTimeout in this situation. (waiting on a close_notify
> >>>>requires performing a read so the read timeout makes sense in this
> >>>>context) I'm happy to alter that but I think that the combination of
> >>>>a timeout and a retry count is straightforward and lower impact.
> >>>>
> >>>>In my opinion the default behaviour of potentially hanging indefinitely
> >>>>is worse than the alternative here. (bearing in mind that we are closing
> >>>>the underlying socket)
> >>>>
> >>>>I'll file a CSR as soon as we settle on the direction this fix will
> >>>>take.
> >>>>
> >>>>    -Rob
> >>>>
> >>>>On 13/09/17 05:52, Xuelei Fan wrote:
> >>>>>In theory, there are intermittent compatibility problems as this update may
> >>>>>not close the SSL connection over the existing socket layer gracefully, even
> >>>>>for high speed networking environments, while the underlying socket is
> >>>>>alive.  The impact could be serious in some environment.
> >>>>>
> >>>>>For safe, I may suggest turn this countermeasure off by default.  And
> >>>>>providing options to turn on this countermeasure:
> >>>>>1. Close the SSL connection gracefully by default; or
> >>>>>2. Close the SSL connection after a timeout.
> >>>>>
> >>>>>It's hardly to say 5 times receiving timeout is better/safer than timeout
> >>>>>once in this context.  As you have already had a system property to control,
> >>>>>you may be able to use options other than the customized socket receiving
> >>>>>timeout, so that the closing timeout is not mixed/confused/dependent on/with
> >>>>>the receiving timeout.
> >>>>>
> >>>>>Put all together:
> >>>>>1. define a closing timeout, for example "jdk.tls.waitForClose".
> >>>>>2. the property default value is zero, no behavior changes.
> >>>>>3. applications can set positive milliseconds value for the property. The
> >>>>>SSL connection will be closed in the set milliseconds (or about the maximum
> >>>>>value between SO_TIMEOUT and closing timeout), the connection is not grant
> >>>>>to be gracefully.
> >>>>>
> >>>>>What do you think?
> >>>>>
> >>>>>BTW, please file a CSR as this update is introducing an external system
> >>>>>property.
> >>>>>
> >>>>>Thanks,
> >>>>>Xuelei
> >>>>>
> >>>>>On 9/11/2017 3:29 PM, Rob McKenna wrote:
> >>>>>>Hi folks,
> >>>>>>
> >>>>>>In high latency environments a client SSLSocket with autoClose set to false
> >>>>>>can hang indefinitely if it does not correctly recieve a close_notify
> >>>>>>from the server.
> >>>>>>
> >>>>>>In order to rectify this situation I would like to suggest that we
> >>>>>>implement an integer JDK property (jdk.tls.closeRetries) which instructs
> >>>>>>waitForClose to attempt the close no more times than the value of the
> >>>>>>property. I would also suggest that 5 is a reasonable default.
> >>>>>>
> >>>>>>Note: each attempt times out based on the value of
> >>>>>>Socket.setSoTimeout(int timeout).
> >>>>>>
> >>>>>>Also, the behaviour here is similar to that of waitForClose() when
> >>>>>>autoClose is set to true, less the retries.
> >>>>>>
> >>>>>>http://cr.openjdk.java.net/~robm/8184328/webrev.01/
> >>>>>>
> >>>>>>    -Rob
> >>>>>>
> >>>