[RFR] 8184328: JDK8u131 socketRead0 hang at SSL read

[Rob McKenna]

On 15/09/17 07:32, Xuelei Fan wrote:
> On 9/15/2017 7:16 AM, Rob McKenna wrote:
> >On 13/09/17 03:52, Xuelei Fan wrote:
> >>
> >>
> >>On 9/13/2017 8:52 AM, Rob McKenna 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-
> >>>
> >>I did not get the point.  What do you mean by this behavior is already
> >>exposed?
> >
> >In SSLSocketImpl.closeSocket() waitForClose is only called if autoclose
> >is true. If not the SSLSocket simply calls super.close().
> >
> Did you get something different?  I think waitForClose is only called if
> autoclose is false.
> 
> No matter the autoclose is true or false, I'm not sure what do you mean by
> this behavior is already exposed.  Can you describe more about the point.
> 

Ack, yes, sorry, I got that backwards. If you set autoclose to true
SSLSocket.close() will skip waitForClose() and simply call
super.close(). When I say this behaviour is already exposed I am
referring to the call to super.close(). (which is effectively what this
fix does after the specified number of attempts via the call to fatal)

    -Rob

> >>
> >>>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.
> >>You have more chance to get the close_notify, but it does not mean you can
> >>always get the close_notify in 5 retries.  When you cannot get it, something
> >>bad happens.
> >
> >No, the property would need to be tuned to suit the networking
> >environment in which the application is deployed. Much the same as a
> >timeout would be.
> >
> >>
> >>>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 did not get the point, are we really closing the underlying socket (or the
> >>layered ssl connection?) for the context of you update?
> >
> >We're calling fatal which calls closeSocket which in turn calls
> >super.close(). (this calls Socket.close() via BaseSSLSocketImpl /
> >SSLSocket) As noted in an earlier reply, this will close the
> >underlying native socket. (I'll perform more testing to verify this)
> >
> When the fatal get called?  I may miss something.  Could you describe the
> scenarios in more details?
> 
> Xuelei
> 
> >     -Rob
> >
> >>
> >>Xuelei
> >>
> >>>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
> >>>>>