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().
>
> >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)
-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
> >>>
