RFR 8187026/10, Remove intermittent key from java/net/httpclient/http2/BasicTest.java
Hi there, please review a minor patch to remove key intermittent. The original bug has been resolved in JDK-8177935 and no intermittent failure observed since there. Bug: https://bugs.openjdk.java.net/browse/JDK-8187026 Patch: diff -r 777356696811 test/java/net/httpclient/http2/BasicTest.java --- a/test/java/net/httpclient/http2/BasicTest.java Fri Sep 08 18:24:17 2017 + +++ b/test/java/net/httpclient/http2/BasicTest.java Wed Sep 13 23:14:37 2017 -0700 @@ -24,7 +24,6 @@ /* * @test * @bug 8087112 - * @key intermittent * @library /lib/testlibrary server * @build jdk.testlibrary.SimpleSSLContext * @modules jdk.incubator.httpclient/jdk.incubator.http.internal.common Thanks, Felix
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
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 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
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
On 9/13/2017 8:52 AM, Rob McKenna wrote: W.r.t. a separate timeout, the underlying mechanics of a close already depend on the readTimeout in this situation. That's a concerns of mine. In order to work for your countermeasure, applications have to set receiving timeout, and take care of the closing timeout when evaluate what's a right timeout value. The mixing could be misleading and not easy to use. Xuelei
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
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? 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. 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? 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
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
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 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 >>>
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
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 > >
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
On 9/13/2017 5:52 AM, 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. typo and missed, "not granted to be closed 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
Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
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
Re: RFR[10]: JDK-8023649 - java.net.NetworkInterface.getInterfaceAddresses() call flow clean up
Hi Vyom, thanks for the feedback ... interesting question and at a more general level is it safe to release a local reference if there is a pending exception ...looking at the logic in getByName0 function, it would appear that it is possible to release the name_utf reference before the createNetworkInterface call and avoid any potential pending exception regards Mark On 13/09/2017 04:16, vyom tewari wrote: Hi Mark, Thanks for doing this, i see that "createNetworkInterface" method is getting called from multiple places in NetworkInterface.c there is pending JNI Exception at line 695 in NetworkInterface.c. I am not sure if it is safe to call "(*env)->ReleaseStringUTFChars" even if there is pending JNI Exception. Thanks, Vyom On Tuesday 12 September 2017 10:50 PM, Mark Sheppard wrote: Hi, please oblige and review the follows changes: http://cr.openjdk.java.net/~msheppar/8023649/webrev/ for the issue: https://bugs.openjdk.java.net/browse/JDK-8023649 This is performed under the auspices of reliability, robustness and stability. * As such, a number of error checks are amended to free malloc-ed memory, * to add consistency to the code, additional ExceptionCheck have been added as a post SetObjectArrayElement invocation check, * A long standing issue reporting that NetworkInterface::getInterfacesAddresses can throw an NPE has been addressed. The context for such a failure is that there is an IPv4 only configuration and that configuration is incorrect in its setting. This may lead to the bindings array being allocated, but expected InterfaceAddress objects not allocated and set in the array so the bindings array implicitly contains null references. In NetworkInterface.c the function createNetworkInterface performs a check on an address mask which may lead to skipping the code block handling the InterfaceAddress allocation. regards Mark
