Re: RFR[8234812]: 'Add micros for DatagramChannel send/receive'

[Alan Bateman]

On 13/02/2020 16:53, Chris Hegarty wrote:

:
Did you intentionally use heap byte buffers, or should the test use direct 
buffers?

That is a good discussion point as there is a matrix of heap vs. direct, 
blocking vs. non-blocking, scatter/gather, ... that could be added to 
create more comprehensive micros. I don't have a strong opinion on this 
but your suggestion for heap vs. direct would be useful to have as it 
would help track of the overhead of copying in or out of temporary 
direct buffers (this overhead has been reduced significantly in recent 
releases but should still be observable in micros that don't do anything 
but send/receive or read/write).


-Alan

RE: RFR 8238579: HttpsURLConnection drops the timeout and hangs forever in read(Internet mail)

[Vyom Tewari26]

Hi Felix,
 
Thanks for review, writing a test case is not straight forward. 
 
we have to manually inject the write failure, if you see the attached reproducer with the issue it  hacks the send native API and inject  on the client side which simulates a transient network failure.
 
Even i was not able to reproduce the issue with hooks provided by submitter with latest JDK code.  I was not able inject the failure so HttpURLConnect.writeRequests() did not follow the path where we retry and create new HttpClient, but with JDK8 i was able to reproduce the issue and after my fix issue got resolved.
 
 
Thanks,
Vyom
- Original message -From: "felixxfyang(杨晓峰)" Sent by: "core-libs-dev" To: Vyom Tiwari , net-dev , core-libs-dev Cc:Subject: [EXTERNAL] Re: RFR 8238579: HttpsURLConnection drops the timeout and hangs forever in read(Internet mail)Date: Fri, Feb 14, 2020 12:01 PM 
Hi Vyom,  The patch looks fine. Can you add a regression test for it?Thanks,Felix Yang在 2020/2/14 下午1:01，“core-libs-dev 代表 Vyom Tiwari” 写入:    Hi All,        Please find the below fix  which resolves the issue(    https://bugs.openjdk.java.net/browse/JDK-8238579 ).        "HttpURLConnection.writeRequests()" retry in case of any write failure,    during retry it creates new HttpsClient  without connectTimeout &    readTimeout. Below fix sets the connect & read timeout.        Thanks,    Vyom        diff -r 7e6165c9c606    src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java    ---    a/src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java    Thu Feb 13 17:14:45 2020 -0800    +++    b/src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java    Fri Feb 14 10:11:06 2020 +0530    @@ -87,10 +87,15 @@          */         public void setNewClient (URL url, boolean useCache)             throws IOException {    +        int readTimeout = getReadTimeout();             http = HttpsClient.New (getSSLSocketFactory(),                                     url,                                     getHostnameVerifier(),    -                                useCache, this);    + null,    + -1,    +                                useCache,    + getConnectTimeout(), this);    + http.setReadTimeout(readTimeout);             ((HttpsClient)http).afterConnect();         }        @@ -132,10 +137,14 @@                 boolean useCache) throws IOException {             if (connected)                 return;    -        http = HttpsClient.New (getSSLSocketFactory(),    -                                url,    -                                getHostnameVerifier(),    -                                proxyHost, proxyPort, useCache, this);    +        int readTimeout = getReadTimeout();    +        http = HttpsClient.New(getSSLSocketFactory(),    +                url,    +                getHostnameVerifier(),    +                proxyHost, proxyPort,    +                useCache,    +                getConnectTimeout(), this);    +        http.setReadTimeout(readTimeout);             connected = true;         }         
 

Re: RFR[15] JDK-8239025: ProblemList java/net/httpclient/HandshakeFailureTest.java due to JDK-8238990

[Daniel Fuchs]

+1

I thought I had a possible fix for this test, but unfortunately
I couldn't manage to push it yesterday. I still need a bit of time
to experiment with several options.

best regards,

-- daniel

On 13/02/2020 23:06, sha.ji...@oracle.com wrote:

Hi,
java/net/httpclient/HandshakeFailureTest.java should be in the problem 
list until JDK-8238990 is resolved.


diff -r 87651cb03ebc test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt    Thu Feb 13 13:43:09 2020 -0800
+++ b/test/jdk/ProblemList.txt    Fri Feb 14 07:05:39 2020 +0800
@@ -627,6 +627,8 @@

  java/net/ServerSocket/AcceptInheritHandle.java 8211854 aix-ppc64

+java/net/httpclient/HandshakeFailureTest.java 8238990 windows-all
+
  

  # jdk_nio

Best regards,
John Jiang

Re: RFR[8237480]: Add micros for DatagramSocket send/receive

[Patrick Concannon]

Hi,

With further discussion on this benchmark test with Claes off-list, I've 
updated the code to use more descriptive names. The updated webrev can 
be found below.


http://cr.openjdk.java.net/~pconcannon/8237480/webrevs/webrev.03/


Kind regards,

Patrick


On 13/02/2020 12:54, Patrick Concannon wrote:


Hi Claes,


Thanks for the feedback.

I've reduced the parameters to 5 i.e. "128", "512", "2048", "8192", 
"32768"


I've also removed the main method to conform with the layout of the other 
benchmark tests.

You can find the changes in the updated webrev below:
http://cr.openjdk.java.net/~pconcannon/8237480/webrevs/webrev.02/

Kind regards,

Patrick


On 12/02/2020 15:22, Claes Redestad wrote:

Hi,

looks reasonable to me.

To keep default execution times at a manageable level, I'd recommend
excluding some of the 9 values for the size parameter (3-5 values seem
reasonable).

/Claes

On 2020-02-12 16:04, Patrick Concannon wrote:

Hi,


Could someone please review my webrev for JDK-8237480 'Add micros 
for DatagramSocket send/receive' ?


This change adds some benchmarks for the DatagramSocket::send and 
DatagramSocket::receive methods.



bug: https://bugs.openjdk.java.net/browse/JDK-8237480

webrev: 
http://cr.openjdk.java.net/~pconcannon/8237480/webrevs/webrev.01/



Kind regards,

Patrick

Re: RFR[8237480]: Add micros for DatagramSocket send/receive

[Chris Hegarty]

> On 14 Feb 2020, at 11:57, Patrick Concannon  
> wrote:
> 
> Hi,
> 
> With further discussion on this benchmark test with Claes off-list, I've 
> updated the code to use more descriptive names. The updated webrev can be 
> found below.
> 
> http://cr.openjdk.java.net/~pconcannon/8237480/webrevs/webrev.03/ 
> 
LGTM.

-Chris.

Re: RFR[8237480]: Add micros for DatagramSocket send/receive

[Claes Redestad]

On 2020-02-14 13:01, Chris Hegarty wrote:

http://cr.openjdk.java.net/~pconcannon/8237480/webrevs/webrev.03/


LGTM.


+1

/Claes

Re: RFR[8237480]: Add micros for DatagramSocket send/receive

[Daniel Fuchs]

Looks good to me too.

best regards,

-- daniel

On 14/02/2020 11:57, Patrick Concannon wrote:

Hi,

With further discussion on this benchmark test with Claes off-list, I've 
updated the code to use more descriptive names. The updated webrev can 
be found below.


http://cr.openjdk.java.net/~pconcannon/8237480/webrevs/webrev.03/


Kind regards,

Patrick

Re: RFR[8234812]: 'Add micros for DatagramChannel send/receive'

[Patrick Concannon]

Hi,

Thanks for the feedback Chris and Alan.

I've added an boolean parameter to the benchmark to allow for the use of 
a direct ByteBuffer -- true for direct, false for heap. In interest of 
keeping the run time of the benchmark down, I've only included a 'true' 
value. However, 'false' can be added if a more comprehensive comparison 
is required.


http://cr.openjdk.java.net/~pconcannon/8234812/webrevs/webrev.02/

Kind regards,

Patrick


On 14/02/2020 09:41, Alan Bateman wrote:

On 13/02/2020 16:53, Chris Hegarty wrote:

:
Did you intentionally use heap byte buffers, or should the test use 
direct buffers?


That is a good discussion point as there is a matrix of heap vs. 
direct, blocking vs. non-blocking, scatter/gather, ... that could be 
added to create more comprehensive micros. I don't have a strong 
opinion on this but your suggestion for heap vs. direct would be 
useful to have as it would help track of the overhead of copying in or 
out of temporary direct buffers (this overhead has been reduced 
significantly in recent releases but should still be observable in 
micros that don't do anything but send/receive or read/write).


-Alan

Re: RFR 8238579: HttpsURLConnection drops the timeout and hangs forever in read

[Daniel Fuchs]

Hi Vyom,

On the surface, your patch seems reasonable.
I would be more confident if there was a test, but I understand
it might well be one of these noreg-hard issues.

I've put it on my TODO list to import your patch and
try to test it in our test system.

I'll get back to you when I have managed to find some
cycles to do so.

best regards,

-- daniel

On 14/02/2020 04:57, Vyom Tiwari wrote:

Hi All,

Please find the below fix  which resolves the issue(
https://bugs.openjdk.java.net/browse/JDK-8238579).

"HttpURLConnection.writeRequests()" retry in case of any write failure,
during retry it creates new HttpsClient  without connectTimeout &
readTimeout. Below fix sets the connect & read timeout.

Thanks,
Vyom

diff -r 7e6165c9c606
src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
---
a/src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
Thu Feb 13 17:14:45 2020 -0800
+++
b/src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
Fri Feb 14 10:11:06 2020 +0530
@@ -87,10 +87,15 @@
   */
  public void setNewClient (URL url, boolean useCache)
  throws IOException {
+int readTimeout = getReadTimeout();
  http = HttpsClient.New (getSSLSocketFactory(),
  url,
  getHostnameVerifier(),
-useCache, this);
+ null,
+ -1,
+useCache,
+ getConnectTimeout(), this);
+ http.setReadTimeout(readTimeout);
  ((HttpsClient)http).afterConnect();
  }

@@ -132,10 +137,14 @@
  boolean useCache) throws IOException {
  if (connected)
  return;
-http = HttpsClient.New (getSSLSocketFactory(),
-url,
-getHostnameVerifier(),
-proxyHost, proxyPort, useCache, this);
+int readTimeout = getReadTimeout();
+http = HttpsClient.New(getSSLSocketFactory(),
+url,
+getHostnameVerifier(),
+proxyHost, proxyPort,
+useCache,
+getConnectTimeout(), this);
+http.setReadTimeout(readTimeout);
  connected = true;
  }

RE: RFR 8238579: HttpsURLConnection drops the timeout and hangs forever in read

[Vyom Tewari26]

Sounds good
Thanks,
Vyom
 
- Original message -From: Daniel Fuchs Sent by: "net-dev" To: Vyom Tiwari , net-dev , core-libs-dev Cc:Subject: [EXTERNAL] Re: RFR 8238579: HttpsURLConnection drops the timeout and hangs forever in readDate: Sat, Feb 15, 2020 1:11 AM 
Hi Vyom,On the surface, your patch seems reasonable.I would be more confident if there was a test, but I understandit might well be one of these noreg-hard issues.I've put it on my TODO list to import your patch andtry to test it in our test system.I'll get back to you when I have managed to find somecycles to do so.best regards,-- danielOn 14/02/2020 04:57, Vyom Tiwari wrote:> Hi All,>> Please find the below fix  which resolves the issue(> https://bugs.openjdk.java.net/browse/JDK-8238579 ).>> "HttpURLConnection.writeRequests()" retry in case of any write failure,> during retry it creates new HttpsClient  without connectTimeout &> readTimeout. Below fix sets the connect & read timeout.>> Thanks,> Vyom>> diff -r 7e6165c9c606> src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java> ---> a/src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java> Thu Feb 13 17:14:45 2020 -0800> +++> b/src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java> Fri Feb 14 10:11:06 2020 +0530> @@ -87,10 +87,15 @@>        */>       public void setNewClient (URL url, boolean useCache)>           throws IOException {> +        int readTimeout = getReadTimeout();>           http = HttpsClient.New (getSSLSocketFactory(),>                                   url,>                                   getHostnameVerifier(),> -                                useCache, this);> + null,> + -1,> +                                useCache,> + getConnectTimeout(), this);> + http.setReadTimeout(readTimeout);>           ((HttpsClient)http).afterConnect();>       }>> @@ -132,10 +137,14 @@>               boolean useCache) throws IOException {>           if (connected)>               return;> -        http = HttpsClient.New (getSSLSocketFactory(),> -                                url,> -                                getHostnameVerifier(),> -                                proxyHost, proxyPort, useCache, this);> +        int readTimeout = getReadTimeout();> +        http = HttpsClient.New(getSSLSocketFactory(),> +                url,> +                getHostnameVerifier(),> +                proxyHost, proxyPort,> +                useCache,> +                getConnectTimeout(), this);> +        http.setReadTimeout(readTimeout);>           connected = true;>       }>