RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java

[Vyom Tewari]

Hi everyone,
Can you please review my changes for below bug.

Bug:
 JDK-8080402 : File Leak in 
jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java

Webrev:
http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/

This change ensure that if close() fails we throw correct io exception 
and there is no file leak.


Thanks,
Vyom

Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java

[Vyom Tewari]

Hi All,

Please find the latest diff, which contains the  latest fix.
http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/

Thanks,
Vyom


On 9/7/2015 3:48 PM, Alan Bateman wrote:

On 07/09/2015 10:26, Vyom Tewari wrote:

Hi everyone,
Can you please review my changes for below bug.

Bug:
 JDK-8080402 : File Leak in 
jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java

Webrev:
http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/

This change ensure that if close() fails we throw correct io 
exception and there is no file leak.
close isn't restartable so I think we need to look at that while we 
are here.


-Alan.

Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java

[Vyom Tewari]

Hi Mark,

Is it OK to go ahead with the patch as it is, or you are expecting any 
additional modification is required ?.


Thanks,
Vyom


On 9/8/2015 6:35 PM, Mark Sheppard wrote:

that's true, the documentation is a bit nebulous on this issue.
but the inference is that the file descriptors are indeterminate state.
some older man pages allude to this

as per solaris man pages, close will
" If close() is interrupted by a signal that is to be caught,
it  will return  -1  with errno set to EINTR and the state of fildes 
is unspecified"


if dup2(s, fd) is viewed as a combination of close(fd) and fcntl (s, 
F_DUP2FD, fd), and close is not restartable

then similar semantics could be inferred for dup2 in a EINTR scenario?
presume that subsequent call in the RESTARTABLE loop  will return 
another error.





On 08/09/2015 09:28, Chris Hegarty wrote:

On 7 Sep 2015, at 17:41, Mark Sheppard <mark.shepp...@oracle.com> wrote:


a couple of other considerations in the context of this issue perhaps?

in this s is being duped onto fd, and part of the dup2 operation is 
the closing of fd, but
what's is the expected state of file descriptor fd in the event of a 
dup2 failure?
I’m not sure that the documentation for dup2 gives us enough detail 
here. The only situation I can see where fd would not be closed is 
when EBADF is returned. Should not be an issue here.


s is closed in any case, but what about fd, should it be attended to 
if dup2 < 0

and closed ? is it still considered a valid fd?

what can be said about the state of the encapsulating FileDescriptor 
associated with fd?

at this stage can it still be considered valid?
should valid() return false?
Probably, but may not be worth bothering with unless there are 
operations that can access it after this method throws.


-Chris.


regards
Mark

On 07/09/2015 14:29, Ivan Gerasimov wrote:

Thanks!

It looks good to me now.

Sincerely yours,
Ivan

On 07.09.2015 16:08, Vyom Tewari wrote:

Hi All,

Please find the latest diff, which contains the  latest fix.
http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/

Thanks,
Vyom


On 9/7/2015 3:48 PM, Alan Bateman wrote:

On 07/09/2015 10:26, Vyom Tewari wrote:

Hi everyone,
Can you please review my changes for below bug.

Bug:
 JDK-8080402 : File Leak in 
jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java

Webrev:
http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/

This change ensure that if close() fails we throw correct io 
exception and there is no file leak.
close isn't restartable so I think we need to look at that while 
we are here.


-Alan.

RFR 8073542 : File Leak in jdk/src/java/base/unix/native/libnet/PlainDatagramSocketImpl.c

[Vyom Tewari]

Hi All,

Please review my changes for below bug.

Bug:
  JDK-8073542 : File Leak in 
jdk/src/java/base/unix/native/libnet/PlainDatagramSocketImpl.c

Webrev:
http://cr.openjdk.java.net/~dfuchs/vyom/8073542/webrev.00

This change ensure that if "setsocketopt" fails we close the 
corresponding fd and throw correct  exception.


Thanks,
Vyom

RFR 8144008: Setting NO_PROXY on an URLConnection is not complied with

[Vyom Tewari]

Hi All,

Please review the below fix.
Bug   : JDK-8144008 Setting NO_PROXY on an URLConnection is not 
complied with
Webrev : 
http://cr.openjdk.java.net/~vtewari/8144008/webrev0.0/index.html 



Thanks,
Vyom

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Vyom Tewari]

Hi Pavel,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.4/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.4/index.html>). I 
had incorporated the review comments.


Thanks,
Vyom


On Monday 20 June 2016 07:27 PM, Pavel Rappo wrote:

Vyom, please consider the following changes:

1. Append 8071660 to the lists of tests here:

* @bug 8010464 8027570 8027687 8029354 8071660
^
2. Reformat the code the way it's in the rest of the file:

from:

  266 URLPermission that = (URLPermission)p;
  267
  268 if(this.methods.isEmpty() && !that.methods.isEmpty())
  269 return false;
  270
  271 if (!this.methods.isEmpty() && !this.methods.get(0).equals("*") &&
  272 Collections.indexOfSubList(this.methods, that.methods) == 
-1) {
  273 return false;
  274 }

to:

  266 URLPermission that = (URLPermission)p;
  267
  268 if (this.methods.isEmpty() && !that.methods.isEmpty()) {
  269 return false;
  270 }
  271
  272 if (!this.methods.isEmpty() &&
  273 !this.methods.get(0).equals("*") &&
  274 Collections.indexOfSubList(this.methods,
  275that.methods) == -1) {
  276 return false;
  277 }

Other than these minor things, looks good.

Thanks,
-Pavel


On 20 Jun 2016, at 12:36, Daniel Fuchs <daniel.fu...@oracle.com> wrote:

On 17/06/16 15:09, Vyom Tewari wrote:

Hi,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.3/index.html
<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.3/index.html>). I
fixed the typo along with spaces issue.

Looks good!

-- daniel


Thanks,
Vyom


On Friday 17 June 2016 06:34 PM, Roger Riggs wrote:

Hi Vyom,

URLPermissionTest.java:

line 125:  it looks odd to assign the same variable twice with the
same value.
   Perhaps it should have been assigning this.url2 = url2.

line 330-332: please add a space after "," in argument lists.

Roger




On 6/17/2016 8:42 AM, Daniel Fuchs wrote:

On 17/06/16 13:25, Vyom Tewari wrote:

Hi Daniel,

thanks for review please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.2/index.html
<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.2/index.html>) .
I added some more tests where base url is different.

Thanks Vyom,
Looks good to me.

See if you can get someone more experienced in the area
to give a thumb up too :-)

best regards,

-- daniel



Thanks,
Vyom

On Friday 17 June 2016 04:57 PM, Daniel Fuchs wrote:

On 17/06/16 09:21, Vyom Tewari wrote:

Hi All,

Please find the new
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.1/index.html

<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.1/index.html>).

I  addressed the review comments given by Daniel.

Hi Vyom,

Looks good to me - but I'm a bit concerned that the previous
mistake was not caught that by any test.
Could you add a test that fails with you previous fix
but passes with the new one?

best regards,

-- daniel


Thanks,
Vyom


On Sunday 12 June 2016 02:04 PM, Daniel Fuchs wrote:

Hi Vyom,

This looks strange to me:

268 if(!this.methods.isEmpty() && that.methods.isEmpty())
269 return true;
270 if(this.methods.isEmpty() && !that.methods.isEmpty())
271 return false;
272 if(this.methods.isEmpty() && that.methods.isEmpty())
273 return true;

Namely, lines 269 & 273 will return true before the URL part
of the permission has been checked.
Is that really the expected behavior?

best regards,

-- daniel

On 11/06/16 05:50, vyom wrote:

Hi All,

Please review the below fix.

Bug   : JDK-8071660 URLPermission not handling empty
method
lists correctly
Webrev :
http://cr.openjdk.java.net/~vtewari/8071660/webrev0.0/index.html
<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.0/index.html>


Thanks,
Vyom

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Vyom Tewari]

Hi Daniel,

thanks for review please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.2/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.2/index.html>) . 
I added some more tests where base url is different.


Thanks,
Vyom

On Friday 17 June 2016 04:57 PM, Daniel Fuchs wrote:

On 17/06/16 09:21, Vyom Tewari wrote:

Hi All,

Please find the new
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.1/index.html
<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.1/index.html>).
I  addressed the review comments given by Daniel.


Hi Vyom,

Looks good to me - but I'm a bit concerned that the previous
mistake was not caught that by any test.
Could you add a test that fails with you previous fix
but passes with the new one?

best regards,

-- daniel



Thanks,
Vyom


On Sunday 12 June 2016 02:04 PM, Daniel Fuchs wrote:

Hi Vyom,

This looks strange to me:

 268 if(!this.methods.isEmpty() && that.methods.isEmpty())
 269 return true;
 270 if(this.methods.isEmpty() && !that.methods.isEmpty())
 271 return false;
 272 if(this.methods.isEmpty() && that.methods.isEmpty())
 273 return true;

Namely, lines 269 & 273 will return true before the URL part
of the permission has been checked.
Is that really the expected behavior?

best regards,

-- daniel

On 11/06/16 05:50, vyom wrote:

Hi All,

Please review the below fix.

Bug   : JDK-8071660 URLPermission not handling empty 
method

lists correctly
Webrev :
http://cr.openjdk.java.net/~vtewari/8071660/webrev0.0/index.html
<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.0/index.html>

Thanks,
Vyom

Re: RFR 8114860: Behavior of java.net.URLPermission.getActions() contradicts spec

[Vyom Tewari]

Hi,

Please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8114860/webrev0.1/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8114860/webrev0.1/index.html>). I 
incorporated the review comments and remove the "HashCodeTest" where it 
compares the hashcode with hard coded hashcode.


Thanks,
Vyom


On Monday 20 June 2016 03:57 PM, Pavel Rappo wrote:

Hi Vyom, thanks for looking into this!

0.  * Copyright (c) 2013,2016 Oracle and/or its affiliates. All rights reserved.

If I understand correctly, we're not required to update years in the header
manually. But if you decided to do so, please follow the established pattern (in
both files you've changed):

 * Copyright (c) 2013, 2016, Oracle and/or its affiliates. All rights 
reserved.
  ^^

1. I wonder if we could use String convenience methods instead of bringing
dependency on java.util.stream.Collector. Could something like this be a bit
better both in terms of dependencies and readability?

 private String actions() {
 String b = String.join(",", methods);
 if (!requestHeaders.isEmpty()) {
 b = ":" + String.join(",", requestHeaders);
 }
 return b;
 }

2. Is there a particular reason for these parentheses?

 150 return (expectedActions.equals(urlp.getActions()));
^ ^

3. Why are these lines commented out?

 259 static Test[] hashTests = {
 260 //hashtest("http://www.foo.com/path;, "GET:X-Foo", 388644203),
 261 //hashtest("http:*", "*:*", 3255810)
 262 };
 
IMO they either have to be updates or removed altogether.


Thanks,
-Pavel


On 20 Jun 2016, at 08:44, Vyom Tewari <vyom.tew...@oracle.com> wrote:

ping!

On Saturday 11 June 2016 10:34 AM, vyom wrote:

Hi All,

Please review the below fix.

Bug   : JDK-8114860 Behavior of java.net.URLPermission.getActions() 
contradicts spec
Webrev : http://cr.openjdk.java.net/~vtewari/8114860/webrev0.0/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8114860/webrev0.0/index.html>

Thanks,
Vyom

Re: RFR 8114860: Behavior of java.net.URLPermission.getActions() contradicts spec

[Vyom Tewari]

Hi Pavel,

Please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8114860/webrev0.2/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8114860/webrev0.2/index.html>). I 
reverted the commented "hashtest" test.


Thanks,
Vyom


On Monday 20 June 2016 10:53 PM, Pavel Rappo wrote:

Same here. I got this only after I had a look at the history of changes.


On 20 Jun 2016, at 17:53, Vyom Tewari <vyom.tew...@oracle.com> wrote:

I was not expecting that hashcodetest is testing NPE by comparing the 
"hardcoded" hash values.

Re: RFR 8144008: Setting NO_PROXY on an URLConnection is not complied with

[Vyom Tewari]

Hi All,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8144008/webrev0.1/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8144008/webrev0.1/index.html>), i 
got some off line comments from Chris.


Thanks,
Vyom

On Tuesday 14 June 2016 12:11 PM, Vyom Tewari wrote:

Hi All,

Please review the below fix.
Bug   : JDK-8144008 Setting NO_PROXY on an URLConnection is 
not complied with
Webrev : 
http://cr.openjdk.java.net/~vtewari/8144008/webrev0.0/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8144008/webrev0.0/index.html>


Thanks,
Vyom

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Vyom Tewari]

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.3/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.3/index.html>). I 
fixed the typo along with spaces issue.


Thanks,
Vyom


On Friday 17 June 2016 06:34 PM, Roger Riggs wrote:

Hi Vyom,

URLPermissionTest.java:

line 125:  it looks odd to assign the same variable twice with the 
same value.

   Perhaps it should have been assigning this.url2 = url2.

line 330-332: please add a space after "," in argument lists.

Roger




On 6/17/2016 8:42 AM, Daniel Fuchs wrote:

On 17/06/16 13:25, Vyom Tewari wrote:

Hi Daniel,

thanks for review please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.2/index.html
<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.2/index.html>) .
I added some more tests where base url is different.


Thanks Vyom,
Looks good to me.

See if you can get someone more experienced in the area
to give a thumb up too :-)

best regards,

-- daniel




Thanks,
Vyom

On Friday 17 June 2016 04:57 PM, Daniel Fuchs wrote:

On 17/06/16 09:21, Vyom Tewari wrote:

Hi All,

Please find the new
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.1/index.html 

<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.1/index.html>). 


I  addressed the review comments given by Daniel.


Hi Vyom,

Looks good to me - but I'm a bit concerned that the previous
mistake was not caught that by any test.
Could you add a test that fails with you previous fix
but passes with the new one?

best regards,

-- daniel



Thanks,
Vyom


On Sunday 12 June 2016 02:04 PM, Daniel Fuchs wrote:

Hi Vyom,

This looks strange to me:

 268 if(!this.methods.isEmpty() && that.methods.isEmpty())
 269 return true;
 270 if(this.methods.isEmpty() && !that.methods.isEmpty())
 271 return false;
 272 if(this.methods.isEmpty() && that.methods.isEmpty())
 273 return true;

Namely, lines 269 & 273 will return true before the URL part
of the permission has been checked.
Is that really the expected behavior?

best regards,

-- daniel

On 11/06/16 05:50, vyom wrote:

Hi All,

Please review the below fix.

Bug   : JDK-8071660 URLPermission not handling empty
method
lists correctly
Webrev :
http://cr.openjdk.java.net/~vtewari/8071660/webrev0.0/index.html
<http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.0/index.html> 



Thanks,
Vyom

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Vyom Tewari]

Hi All,

Please find the new 
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.1/index.html 
). 
I  addressed the review comments given by Daniel.


Thanks,
Vyom


On Sunday 12 June 2016 02:04 PM, Daniel Fuchs wrote:

Hi Vyom,

This looks strange to me:

 268 if(!this.methods.isEmpty() && that.methods.isEmpty())
 269 return true;
 270 if(this.methods.isEmpty() && !that.methods.isEmpty())
 271 return false;
 272 if(this.methods.isEmpty() && that.methods.isEmpty())
 273 return true;

Namely, lines 269 & 273 will return true before the URL part
of the permission has been checked.
Is that really the expected behavior?

best regards,

-- daniel

On 11/06/16 05:50, vyom wrote:

Hi All,

Please review the below fix.

Bug   : JDK-8071660 URLPermission not handling empty method
lists correctly
Webrev :
http://cr.openjdk.java.net/~vtewari/8071660/webrev0.0/index.html


Thanks,
Vyom

Re: RFR 8150521: Test test/java/net/InetAddress/getOriginalHostName.java fails to load JavaNetInetAddressAccess from SharedSecrets

[Vyom Tewari]

incorporated the review comment, updated webrev in place.

http://cr.openjdk.java.net/~nkumar/vyom/8150521/webrev0.1/index.html 



Vyom

On 3/2/2016 1:45 PM, Alan Bateman wrote:

On 02/03/2016 07:44, vyom wrote:

Hi Chris,

Thanks for review please find the updated 
webrev(http://cr.openjdk.java.net/~nkumar/vyom/8150521/webrev0.1/index.html 
). 

This looks okay to me. Style wide, can you put a space in "if(" to 
keep it consistent.


-Alan.

Re: RFR 8148609: supportedOptions() methods return a mutable set

[Vyom Tewari]

please find the updated webrev

http://cr.openjdk.java.net/~nkumar/vyom/8148609/webrev0.2/ 



Thanks,
Vyom


On 3/2/2016 2:36 PM, Chris Hegarty wrote:

On 2 Mar 2016, at 08:19, Alan Bateman  wrote:



On 02/03/2016 06:47, vyom wrote:

Hi Chris/Alan,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~nkumar/vyom/8148609/webrev0.1/index.html 
).

This looks much better.

+1


I see the test is named SupportedOptionTest and so hints that it tests more 
than it does. I wonder if we could find a better name for it, maybe 
ImmutableOptions or something along those lines.

ImmutableOptions is better.

-Chris.

Re: RFR 8148609: supportedOptions() methods return a mutable set

[Vyom Tewari]

i need sponsor for this fix.
Vyom

On 3/3/2016 2:37 PM, Alan Bateman wrote:



On 03/03/2016 09:03, Chris Hegarty wrote:

On 3 Mar 2016, at 05:36, Vyom Tewari <vyom.tew...@oracle.com> wrote:


please find the updated webrev

http://cr.openjdk.java.net/~nkumar/vyom/8148609/webrev0.2/ 
<http://cr.openjdk.java.net/%7Enkumar/vyom/8148609/webrev0.2/>

Looks fine.


Looks okay to me too.

-Alan

Re: RFR 8150521: Test test/java/net/InetAddress/getOriginalHostName.java fails to load JavaNetInetAddressAccess from SharedSecrets

[Vyom Tewari]

i need sponsor for this fix as well.
Vyom

On 3/3/2016 2:34 PM, Chris Hegarty wrote:

On 3 Mar 2016, at 05:33, Vyom Tewari <vyom.tew...@oracle.com> wrote:


incorporated the review comment, updated webrev in place.

http://cr.openjdk.java.net/~nkumar/vyom/8150521/webrev0.1/index.html 
<http://cr.openjdk.java.net/%7Enkumar/vyom/8150521/webrev0.1/index.html>

Looks fine.

-Chris.


Vyom

On 3/2/2016 1:45 PM, Alan Bateman wrote:

On 02/03/2016 07:44, vyom wrote:

Hi Chris,

Thanks for review please find the updated 
webrev(http://cr.openjdk.java.net/~nkumar/vyom/8150521/webrev0.1/index.html 
<http://cr.openjdk.java.net/%7Enkumar/vyom/8150521/webrev0.1/index.html>).

This looks okay to me. Style wide, can you put a space in "if(" to keep it 
consistent.

-Alan.

RFR 8161291: Serialization Tests for URLPermission is failing

[Vyom Tewari]

Hi,

Please review the below code change.

Bug: JDK-8161291  Serialization Tests for URLPermission is failing

Webrev  : 
http://cr.openjdk.java.net/~vtewari/8161291/webrev0.0/index.html 



This change make sure that colon separator need to be present even if 
the request headers list is empty.


Thanks,

Vyom

RFR 8144692: HttpServer API: use of non-existant method in example in package Javadoc

[Vyom Tewari]

Hi All,

Please review below small doc fix.

Bug: JDK-8144692 HttpServer API: use of non-existant method 
in example in package Javadoc
Webrev  : 
http://cr.openjdk.java.net/~vtewari/8144692/webrev0.0/index.html 



Thanks,
Vyom

Re: RFR 8151788: NullPointerException from ntlm.Client.type3

[Vyom Tewari]

gentile reminder.
Vyom

On Thursday 30 June 2016 02:16 PM, Vyom Tewari wrote:

Hi All,

Please review the below simple fix.

Bug: JDK-8151788  NullPointerException from ntlm.Client.type3
Webrev  : 
http://cr.openjdk.java.net/~vtewari/8151788/webrev0.0/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8151788/webrev0.0/index.html>


Thanks,
Vyom

Re: RFR 8151788: NullPointerException from ntlm.Client.type3

[Vyom Tewari]

Hi All,

Please find the updated 
webrev(http://cr.openjdk.java.net/%7Evtewari/8151788/webrev0.1/index.html). 
I addressed the review comments.


Thanks,
Vyom


On Tuesday 12 July 2016 08:25 PM, Weijun Wang wrote:



On 7/12/2016 22:34, Pavel Rappo wrote:
What's the difference between no security buffer and an empty one 
(from the

com.sun.security.ntlm.Client#type3's perspective)?


I quickly browse through the NTLM protocol and yes they look like the 
same in each case. (Except for one which I am not sure, is there any 
difference between no domain and empty domain?) In all cases where a 
security buffer is optional, there is a flag we can rely on, and no 
need to look at whether the offset of the security buffer is zero.


So it does look safer to return a new byte[0] right inside 
readSecurityBuffer(int offset) when the offset is zero.


Thanks
Max




On 12 Jul 2016, at 15:25, Wang Weijun  wrote:

When there is no offset, there is no security buffer at all. When 
the length is zero, the security buffer is an empty byte array.

Re: RFR 8144692: HttpServer API: use of non-existant method in example in package Javadoc

[Vyom Tewari]

Hi Pavel,

Thanks for  review, i updated the 
webrev(http://cr.openjdk.java.net/~vtewari/8144692/webrev0.0/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8144692/webrev0.0/index.html>) in 
place.


Thanks,
Vyom


On Tuesday 12 July 2016 06:10 PM, Pavel Rappo wrote:

Hi Vyom,

A minor comment. Do you think it would make sense to use space after

 new InetSocketAddress(8000),
 
as in every other line in this javadoc where multiple arguments passed, the

space is used. Just to be consistent in formatting.

Thanks.


On 12 Jul 2016, at 12:36, Vyom Tewari <vyom.tew...@oracle.com> wrote:

Hi All,

Please review below small doc fix.

Bug: JDK-8144692 HttpServer API: use of non-existant method in 
example in package Javadoc
Webrev  : http://cr.openjdk.java.net/~vtewari/8144692/webrev0.0/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8144692/webrev0.0/index.html>

Thanks,
Vyom

RFR 8151788: NullPointerException from ntlm.Client.type3

[Vyom Tewari]

Hi All,

Please review the below simple fix.

Bug: JDK-8151788  NullPointerException from ntlm.Client.type3
Webrev  : 
http://cr.openjdk.java.net/~vtewari/8151788/webrev0.0/index.html 



Thanks,
Vyom

Re: RFR: 8167178 Exported elements referring to inaccessible types in java.naming

[Vyom Tewari]

Hi Roger,

Thanks for the review i remove the comment saying it should be treated 
as read only. Please find below the latest diff.


Thanks,

Vyom

--- a/src/java.naming/share/classes/javax/naming/CompoundName.java Fri 
Dec 23 09:31:24 2016 +0530
+++ b/src/java.naming/share/classes/javax/naming/CompoundName.java Wed 
Jan 18 12:14:25 2017 +0530

@@ -149,11 +149,10 @@
 public class CompoundName implements Name {

 /**
-  * Implementation of this compound name.
-  * This field is initialized by the constructors and cannot be null.
-  * It should be treated as a read-only variable by subclasses.
-  */
-protected transient NameImpl impl;
+ * Implementation of this compound name. This field is initialized 
by the

+ * constructors and cannot be null.
+ */
+private transient NameImpl impl;
 /**
   * Syntax properties for this compound name.
   * This field is initialized by the constructors and cannot be null.


On Tuesday 17 January 2017 09:20 PM, Roger Riggs wrote:

Hi Vyom,

Please also correct or remove the comment saying it should be treated 
as read-only by subclasses.


Roger


On 1/16/2017 4:56 AM, Chris Hegarty wrote:

Looks good. Thanks Vyom.

-Chris.


On 16 Jan 2017, at 09:10, Vyom Tewari <vyom.tew...@oracle.com> wrote:

Hi All,

Please review  below the small fix for the issue.

BugId : https://bugs.openjdk.java.net/browse/JDK-8167178

The compatibility impact is minimum as no code in JDK is currently 
depend on it.


I will file a CCC today.

Thanks,

Vyom


--- a/src/java.naming/share/classes/javax/naming/CompoundName.java 
Fri Dec 23 09:31:24 2016 +0530
+++ b/src/java.naming/share/classes/javax/naming/CompoundName.java 
Mon Jan 16 13:36:48 2017 +0530

@@ -153,7 +153,7 @@
   * This field is initialized by the constructors and cannot be 
null.

   * It should be treated as a read-only variable by subclasses.
   */
-protected transient NameImpl impl;
+private transient NameImpl impl;
 /**
   * Syntax properties for this compound name.
   * This field is initialized by the constructors and cannot be 
null.

RFR: 8167178 Exported elements referring to inaccessible types in java.naming

[Vyom Tewari]

Hi All,

Please review  below the small fix for the issue.

BugId : https://bugs.openjdk.java.net/browse/JDK-8167178

The compatibility impact is minimum as no code in JDK is currently 
depend on it.


I will file a CCC today.

Thanks,

Vyom


--- a/src/java.naming/share/classes/javax/naming/CompoundName.java Fri 
Dec 23 09:31:24 2016 +0530
+++ b/src/java.naming/share/classes/javax/naming/CompoundName.java Mon 
Jan 16 13:36:48 2017 +0530

@@ -153,7 +153,7 @@
   * This field is initialized by the constructors and cannot be null.
   * It should be treated as a read-only variable by subclasses.
   */
-protected transient NameImpl impl;
+private transient NameImpl impl;
 /**
   * Syntax properties for this compound name.
   * This field is initialized by the constructors and cannot be null.

RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

Hi All,

Please review the code changes for below issue.

Bug : https://bugs.openjdk.java.net/browse/JDK-8075484

webrev: 
http://cr.openjdk.java.net/~vtewari/8075484/webrev0.0/index.html 



This issue is SocketInputStream.socketread0() hangs even with 
"soTimeout" set.the implementation of 
Java_java_net_SocketInputStream_socketRead0 assumes that read() won't 
block after poll() reports that a read is possible.


This assumption does not hold, as noted on the man page for select 
(referenced by the man page for poll): Under Linux, select() may report 
a socket file descriptor as "ready for reading", while nevertheless a 
subsequent read blocks. This could for example happen when data has 
arrived but upon examination has wrong checksum and is discarded.


Thanks,

Vyom

Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.2/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.2/index.html>). I 
have incorporated the review comments.


Thanks,

Vyom


On Monday 05 September 2016 08:37 PM, Chris Hegarty wrote:

On 05/09/16 15:37, Mark Sheppard wrote:


if the desire is to avoid making double calls on gettimeofday in the
NET_ReadWithTimeout's while loop for its main call flow,


Yes, the desire is to make no more calls to gettimeofday, than are
already done.


then consider a modification to NET_Timeout and create a version
int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time)

int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time) {
int result;
struct timeval t;
long prevtime, newtime;
struct pollfd pfd;
pfd.fd = s;
pfd.events = POLLIN;

if (timeout > 0) {
prevtime = (current_time->tv_sec * 1000)  +
current_time->tv_usec / 1000;
}

for(;;) {
result = poll(, 1, timeout);
if (result < 0 && errno == EINTR) {
if (timeout > 0) {
gettimeofday(, NULL);
newtime = (t.tv_sec * 1000)  +  t.tv_usec /1000;
timeout -= newtime - prevtime;
if (timeout <= 0)
return 0;
prevtime = newtime;
}
} else {
return result;
}
}
}

the NET_ReadWithTimeout is modified with.

   while (timeout > 0) {
result = NET_TimeoutWithCurrentTime(fd, timeout, );

in any case there is the potential for multiple invocation of
gettimeofday due to a need to make
poll restartable,


Yes, and that is fine. Just no more than is already there.

 and adjust the originally specified timeout

accordingly, and for the edge case
after the non blocking read.


After an initial skim, your suggestion seems reasonable.

-Chris.


regards
Mark



On 05/09/2016 12:02, Chris Hegarty wrote:

Vyom,

>>>
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html

There is some concern about the potential performance effect, etc,
of gettimeofday, maybe there is a way out of this. The reuse of
NET_Timeout is good, but it also calls gettimeofday. It seems that
a specific NET_ReadWithTimeout could be written to NOT reuse
NET_Timeout, but effectively inline its interruptible operation.
Or write a variant of NET_Timeout that takes a function to
execute. Rather than effectively two loops conditioned on the
timeout.  Disclaimer: I have not actually tried to do this, but
it seems worth considering / evaluating.

-Chris.

On 02/09/16 04:39, Vyom Tewari wrote:

hi Dimitry,

thanks for review, I did consider to use  a monotonically increasing
clock like "clock_gettime" but  existing nearby code("NET_Timeout") 
uses

"gettimeofday"  so i choose to be consistent with the existing code.

Thanks,

Vyom


On Friday 02 September 2016 01:38 AM, Dmitry Samersoff wrote:

Vyom,

Did you consider to use select() to calculate timeout instead of
gettimeofday ?

gettimeofday is affected by system time changes, so running ntpd can
cause unpredictable behavior of this code. Also it's rather expensive
syscall.

-Dmitry

On 2016-09-01 19:03, Vyom Tewari wrote:

please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html 



<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.1/index.html>). 


I
incorporated the review comments.

Thanks,

Vyom


On Tuesday 30 August 2016 04:11 PM, Mark Sheppard wrote:

Hi
   perhaps there is an opportunity to do  some refactoring here 
(...

for me a "goto " carries a code smell! )

along the lines

if (timeout) {
 nread =  NET_ReadWithTimeout(...);
} else {
  nread = NET_Read(...);
}


the NET_ReadWithTimeout (...) function will contain a
restructuring of
your goto loop
while (_timeout > 0) { nread = NET_Timeout(fd, _timeout);
   if (nread <= 0) {
   if (nread == 0) {
   JNU_ThrowByName(env, JNU_JAVANETPKG
"SocketTimeoutException",
   "Read timed out");
   } else if (nread == -1) {
   if (errno == EBADF) {
JNU_ThrowByName(env, JNU_JAVANETPKG
"SocketException", "Socket closed");
   } else if (errno == ENOMEM) {
   JNU_ThrowOutOfMemoryError(env, "NET_Timeout
native heap allocation failed");
   } else {
JNU_ThrowByNameWithMessageAndLastError
   (env, JNU_JAVANETPKG "SocketException",
"select/poll failed");
   }
   }
 // release buffer in main call flow
//  if (bufP != BUF) {
//  free(bufP);
// }
   

Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

On Tuesday 06 September 2016 06:25 PM, Chris Hegarty wrote:

Vyom,


On 6 Sep 2016, at 10:20, Vyom Tewari <vyom.tew...@oracle.com> wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.2/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.2/index.html>). I have 
incorporated the review comments.

Your changes completely avoid NET_Timeout, which is quite complex on
Linux, Mac, and AIX, for various reasons. ( NET_Timeout will use the
async close mechanism to signal/interrupt a thread blocked in a poll /
select ). Also, select is used on Mac, which will use poll after your
changes ( I do remember that there was a bug in poll on Mac around
the time of the original Mac port ).

that is why , in webrev0.1 i was re using the NET_Timeout.


Would is be better, rather than replicating the logic in  NET_Timeout,
to have a version that supports passing a function pointer, to run if
the poll/select returns before the timeout? Just an idea.

let me try out if it works.

Thanks,
Vyom

-Chris.


Thanks,

Vyom


On Monday 05 September 2016 08:37 PM, Chris Hegarty wrote:

On 05/09/16 15:37, Mark Sheppard wrote:

if the desire is to avoid making double calls on gettimeofday in the
NET_ReadWithTimeout's while loop for its main call flow,

Yes, the desire is to make no more calls to gettimeofday, than are
already done.


then consider a modification to NET_Timeout and create a version
int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time)

int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time) {
int result;
struct timeval t;
long prevtime, newtime;
struct pollfd pfd;
pfd.fd = s;
pfd.events = POLLIN;

if (timeout > 0) {
prevtime = (current_time->tv_sec * 1000)  +
current_time->tv_usec / 1000;
}

for(;;) {
result = poll(, 1, timeout);
if (result < 0 && errno == EINTR) {
if (timeout > 0) {
gettimeofday(, NULL);
newtime = (t.tv_sec * 1000)  +  t.tv_usec /1000;
timeout -= newtime - prevtime;
if (timeout <= 0)
return 0;
prevtime = newtime;
}
} else {
return result;
}
}
}

the NET_ReadWithTimeout is modified with.

   while (timeout > 0) {
result = NET_TimeoutWithCurrentTime(fd, timeout, );

in any case there is the potential for multiple invocation of
gettimeofday due to a need to make
poll restartable,

Yes, and that is fine. Just no more than is already there.

and adjust the originally specified timeout

accordingly, and for the edge case
after the non blocking read.

After an initial skim, your suggestion seems reasonable.

-Chris.


regards
Mark



On 05/09/2016 12:02, Chris Hegarty wrote:

Vyom,

webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html

There is some concern about the potential performance effect, etc,
of gettimeofday, maybe there is a way out of this. The reuse of
NET_Timeout is good, but it also calls gettimeofday. It seems that
a specific NET_ReadWithTimeout could be written to NOT reuse
NET_Timeout, but effectively inline its interruptible operation.
Or write a variant of NET_Timeout that takes a function to
execute. Rather than effectively two loops conditioned on the
timeout.  Disclaimer: I have not actually tried to do this, but
it seems worth considering / evaluating.

-Chris.

On 02/09/16 04:39, Vyom Tewari wrote:

hi Dimitry,

thanks for review, I did consider to use  a monotonically increasing
clock like "clock_gettime" but  existing nearby code("NET_Timeout") uses
"gettimeofday"  so i choose to be consistent with the existing code.

Thanks,

Vyom


On Friday 02 September 2016 01:38 AM, Dmitry Samersoff wrote:

Vyom,

Did you consider to use select() to calculate timeout instead of
gettimeofday ?

gettimeofday is affected by system time changes, so running ntpd can
cause unpredictable behavior of this code. Also it's rather expensive
syscall.

-Dmitry

On 2016-09-01 19:03, Vyom Tewari wrote:

please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html

<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.1/index.html>).
I
incorporated the review comments.

Thanks,

Vyom


On Tuesday 30 August 2016 04:11 PM, Mark Sheppard wrote:

Hi
   perhaps there is an opportunity to do  some refactoring here (...
for me a "goto " carries a code smell! )

along the lines

if (timeout) {
 nread =  NET_ReadWithTimeout(...);
} else {
  nread = NET_Read(...);
}


the NET_ReadWithTimeout (...) function will contain a
restructuring of
your goto loop
while (_timeout > 0) { nread = NET_Timeout(fd, _timeout);
   if (nread <= 0) {
   if (nread == 0) {
   JNU_ThrowByName(env, JNU_JAVANETPKG
"SocketTi

Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.1/index.html>). I 
incorporated the review comments.


Thanks,

Vyom


On Tuesday 30 August 2016 04:11 PM, Mark Sheppard wrote:


Hi
  perhaps there is an opportunity to do  some refactoring here (...  
for me a "goto " carries a code smell! )


along the lines

if (timeout) {
nread =  NET_ReadWithTimeout(...);
} else {
 nread = NET_Read(...);
}


the NET_ReadWithTimeout (...) function will contain a restructuring of 
your goto loop

while (_timeout > 0) { nread = NET_Timeout(fd, _timeout);
   if (nread <= 0) {
   if (nread == 0) {
   JNU_ThrowByName(env, JNU_JAVANETPKG "SocketTimeoutException",
   "Read timed out");
   } else if (nread == -1) {
   if (errno == EBADF) {
JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", 
"Socket closed");
   } else if (errno == ENOMEM) {
   JNU_ThrowOutOfMemoryError(env, "NET_Timeout native heap 
allocation failed");
   } else {
   JNU_ThrowByNameWithMessageAndLastError
   (env, JNU_JAVANETPKG "SocketException", "select/poll 
failed");
   }
   }
 // release buffer in main call flow
//  if (bufP != BUF) {
//  free(bufP);
// }
  nread = -1;
  break;
   } else {
nread = NET_NonBlockingRead(fd, bufP, len);
if (nread == -1 && ((errno == EAGAIN) || (errno == EWOULDBLOCK))) {
   gettimeofday(, NULL);
newtime = t.tv_sec * 1000 + t.tv_usec / 1000;
_timeout -= newtime - prevtime;
if(_timeout > 0){
prevtime = newtime;
   }
} else { break; } } } return nread;

e

regards
Mark


On 29/08/2016 10:58, Vyom Tewari wrote:

gentle reminder, please review the below code change.

Vyom


On Monday 22 August 2016 05:12 PM, Vyom Tewari wrote:

Hi All,

Please review the code changes for below issue.

Bug : https://bugs.openjdk.java.net/browse/JDK-8075484

webrev: 
http://cr.openjdk.java.net/~vtewari/8075484/webrev0.0/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.0/index.html>


This issue is SocketInputStream.socketread0() hangs even with 
"soTimeout" set.the implementation of 
Java_java_net_SocketInputStream_socketRead0 assumes that read() 
won't block after poll() reports that a read is possible.


This assumption does not hold, as noted on the man page for select 
(referenced by the man page for poll): Under Linux, select() may 
report a socket file descriptor as "ready for reading", while 
nevertheless a subsequent read blocks. This could for example happen 
when data has arrived but upon examination has wrong checksum and is 
discarded.


Thanks,

Vyom

Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

hi Dimitry,

thanks for review, I did consider to use  a monotonically increasing 
clock like "clock_gettime" but  existing nearby code("NET_Timeout") uses 
"gettimeofday"  so i choose to be consistent with the existing code.


Thanks,

Vyom


On Friday 02 September 2016 01:38 AM, Dmitry Samersoff wrote:

Vyom,

Did you consider to use select() to calculate timeout instead of
gettimeofday ?

gettimeofday is affected by system time changes, so running ntpd can
cause unpredictable behavior of this code. Also it's rather expensive
syscall.

-Dmitry

On 2016-09-01 19:03, Vyom Tewari wrote:

please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.1/index.html>). I
incorporated the review comments.

Thanks,

Vyom


On Tuesday 30 August 2016 04:11 PM, Mark Sheppard wrote:

Hi
   perhaps there is an opportunity to do  some refactoring here (...
for me a "goto " carries a code smell! )

along the lines

if (timeout) {
 nread =  NET_ReadWithTimeout(...);
} else {
  nread = NET_Read(...);
}


the NET_ReadWithTimeout (...) function will contain a restructuring of
your goto loop
while (_timeout > 0) { nread = NET_Timeout(fd, _timeout);
   if (nread <= 0) {
   if (nread == 0) {
   JNU_ThrowByName(env, JNU_JAVANETPKG "SocketTimeoutException",
   "Read timed out");
   } else if (nread == -1) {
   if (errno == EBADF) {
JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", 
"Socket closed");
   } else if (errno == ENOMEM) {
   JNU_ThrowOutOfMemoryError(env, "NET_Timeout native heap 
allocation failed");
   } else {
   JNU_ThrowByNameWithMessageAndLastError
   (env, JNU_JAVANETPKG "SocketException", "select/poll 
failed");
   }
   }
 // release buffer in main call flow
//  if (bufP != BUF) {
//  free(bufP);
// }
  nread = -1;
  break;
   } else {
nread = NET_NonBlockingRead(fd, bufP, len);
if (nread == -1 && ((errno == EAGAIN) || (errno == EWOULDBLOCK))) {
   gettimeofday(, NULL);
newtime = t.tv_sec * 1000 + t.tv_usec / 1000;
_timeout -= newtime - prevtime;
if(_timeout > 0){
prevtime = newtime;
   }
} else { break; } } } return nread;

e

regards
Mark


On 29/08/2016 10:58, Vyom Tewari wrote:

gentle reminder, please review the below code change.

Vyom


On Monday 22 August 2016 05:12 PM, Vyom Tewari wrote:

Hi All,

Please review the code changes for below issue.

Bug : https://bugs.openjdk.java.net/browse/JDK-8075484

webrev:
http://cr.openjdk.java.net/~vtewari/8075484/webrev0.0/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.0/index.html>

This issue is SocketInputStream.socketread0() hangs even with
"soTimeout" set.the implementation of
Java_java_net_SocketInputStream_socketRead0 assumes that read()
won't block after poll() reports that a read is possible.

This assumption does not hold, as noted on the man page for select
(referenced by the man page for poll): Under Linux, select() may
report a socket file descriptor as "ready for reading", while
nevertheless a subsequent read blocks. This could for example happen
when data has arrived but upon examination has wrong checksum and is
discarded.

Thanks,

Vyom

Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

sending this mail again as my laptop clock got screwed.

Vyom


On Thursday 08 September 2016 08:49 AM, Vyom Tewari wrote:

Hi All,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.5/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.5/index.html>).


I fixed the AIX flag issue and move some come common code to 
"net_util_md.c" file.


Thanks,
Vyom

On 9/8/2016 12:32 PM, Langer, Christoph wrote:

Hi Vyom,

ok, thanks. I have one addition to my proposal: I don't think there's 
a need for a global NET_GetCurrentTime function. This one should 
probably be a static function inside net_util_md.c (static long 
getCurrentTime()).


Best
Christoph


-Original Message-
From: Vyom Tewari [mailto:vyom.tew...@oracle.com]
Sent: Mittwoch, 7. September 2016 18:31
To: Langer, Christoph <christoph.lan...@sap.com>
Cc: net-dev@openjdk.java.net; Chris Hegarty <chris.hega...@oracle.com>
Subject: Re: RFR 8075484:SocketInputStream.socketRead0 can hang even 
with

soTimeout set

Hi Chris,

Sorry I was mostly focusing on our test  failure first, i will check
your suggestions.

Thanks,

Vyom


On Wednesday 07 September 2016 08:44 PM, Langer, Christoph wrote:

Hi Vyom,

did you have a look at my suggestions regarding AIX and 
refactoring? I can't

find none of it in the new webrev nor did you comment on it.

Best regards
Christoph


-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of

Vyom

Tewari
Sent: Mittwoch, 7. September 2016 17:10
To: Chris Hegarty <chris.hega...@oracle.com>
Cc: net-dev@openjdk.java.net
Subject: Re: RFR 8075484:SocketInputStream.socketRead0 can hang even

with

soTimeout set

Hi All,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.4/index.html 

<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.4/index.html>). 


there were some test failures in JPRT and Chris also pointed the same
issue in modified code. Now with latest code JPRT is clean.

Thanks,

Vyom


On Wednesday 07 September 2016 03:18 PM, Chris Hegarty wrote:

On 07/09/16 07:45, Vyom Tewari wrote:

Hi Chris,

Please find the latest


webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.3/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.3/index.html>). 
I

did some code refactoring, JPRT is in progress.

In terms of NET_Timeout**, I'm happy with this version, but I
have an additional comment.

If NET_ReadWithTimeout returns -1 because

NET_TimeoutWithCurrentTime

returns <= 0, then a JNI pending exception will be set. So the code
calling NET_ReadWithTimeout should, a) free bufP, and b) return -1,
immediately rather than continuing and possibly trying to set 
another

JNI pending exception.

One option is to check the return value from NET_ReadWithTimeout and
also (*env)->ExceptionCheck(env). Another is to possibly consolidate
the setting of JNI pending exceptions in one place, towards the end
of Java_java_net_SocketInputStream_socketRead0, for example EBADF is
handled in two places.

-Chris.

I need help from some one to build and test latest changes on 
AIX, may

be Christoph can help.

Thanks,

Vyom


On Tuesday 06 September 2016 06:25 PM, Chris Hegarty wrote:

Vyom,


On 6 Sep 2016, at 10:20, Vyom Tewari <vyom.tew...@oracle.com>

wrote:

Hi,

Please find the latest

webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.2/index.html 

<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.2/index.html>). 


I
have incorporated the review comments.
Your changes completely avoid NET_Timeout, which is quite 
complex on
Linux, Mac, and AIX, for various reasons. ( NET_Timeout will 
use the
async close mechanism to signal/interrupt a thread blocked in a 
poll /
select ). Also, select is used on Mac, which will use poll 
after your

changes ( I do remember that there was a bug in poll on Mac around
the time of the original Mac port ).

Would is be better, rather than replicating the logic in 
NET_Timeout,
to have a version that supports passing a function pointer, to 
run if

the poll/select returns before the timeout? Just an idea.

-Chris.


Thanks,

Vyom


On Monday 05 September 2016 08:37 PM, Chris Hegarty wrote:

On 05/09/16 15:37, Mark Sheppard wrote:
if the desire is to avoid making double calls on 
gettimeofday in the

NET_ReadWithTimeout's while loop for its main call flow,
Yes, the desire is to make no more calls to gettimeofday, 
than are

already done.

then consider a modification to NET_Timeout and create a 
version
int NET_TimeoutWithCurrentTime(int s, long timeout, stuct 
timeval *

current_time)

int NET_TimeoutWithCurrentTime(int s, long timeout, stuct 
timeval *

current_time) {
  int result;
  struct timeval t;
  long prevtime, newtime;
  struct pollfd pfd;
  pfd.fd = s;
  pfd.events = POLLIN;

  if (timeout > 0) {
  prevtime = (current_time->tv_sec * 1000)  +
c

Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

Hi All,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.5/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.5/index.html>).


I fixed the AIX flag issue and move some come common code to 
"net_util_md.c" file.


Thanks,
Vyom

On 9/8/2016 12:32 PM, Langer, Christoph wrote:

Hi Vyom,

ok, thanks. I have one addition to my proposal: I don't think there's a need 
for a global NET_GetCurrentTime function. This one should probably be a static 
function inside net_util_md.c (static long getCurrentTime()).

Best
Christoph


-Original Message-
From: Vyom Tewari [mailto:vyom.tew...@oracle.com]
Sent: Mittwoch, 7. September 2016 18:31
To: Langer, Christoph <christoph.lan...@sap.com>
Cc: net-dev@openjdk.java.net; Chris Hegarty <chris.hega...@oracle.com>
Subject: Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with
soTimeout set

Hi Chris,

Sorry I was mostly focusing on our test  failure first, i will check
your suggestions.

Thanks,

Vyom


On Wednesday 07 September 2016 08:44 PM, Langer, Christoph wrote:

Hi Vyom,

did you have a look at my suggestions regarding AIX and refactoring? I can't

find none of it in the new webrev nor did you comment on it.

Best regards
Christoph


-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of

Vyom

Tewari
Sent: Mittwoch, 7. September 2016 17:10
To: Chris Hegarty <chris.hega...@oracle.com>
Cc: net-dev@openjdk.java.net
Subject: Re: RFR 8075484:SocketInputStream.socketRead0 can hang even

with

soTimeout set

Hi All,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.4/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.4/index.html>).
there were some test failures in JPRT and Chris also pointed the same
issue in modified code. Now with latest code JPRT is clean.

Thanks,

Vyom


On Wednesday 07 September 2016 03:18 PM, Chris Hegarty wrote:

On 07/09/16 07:45, Vyom Tewari wrote:

Hi Chris,

Please find the latest


webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.3/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.3/index.html>). I

did some code refactoring, JPRT is in progress.

In terms of NET_Timeout**, I'm happy with this version, but I
have an additional comment.

If NET_ReadWithTimeout returns -1 because

NET_TimeoutWithCurrentTime

returns <= 0, then a JNI pending exception will be set. So the code
calling NET_ReadWithTimeout should, a) free bufP, and b) return -1,
immediately rather than continuing and possibly trying to set another
JNI pending exception.

One option is to check the return value from NET_ReadWithTimeout and
also (*env)->ExceptionCheck(env). Another is to possibly consolidate
the setting of JNI pending exceptions in one place, towards the end
of Java_java_net_SocketInputStream_socketRead0, for example EBADF is
handled in two places.

-Chris.


I need help from some one to build and test latest changes on AIX, may
be Christoph can help.

Thanks,

Vyom


On Tuesday 06 September 2016 06:25 PM, Chris Hegarty wrote:

Vyom,


On 6 Sep 2016, at 10:20, Vyom Tewari <vyom.tew...@oracle.com>

wrote:

Hi,

Please find the latest


webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.2/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.2/index.html>).

I
have incorporated the review comments.

Your changes completely avoid NET_Timeout, which is quite complex on
Linux, Mac, and AIX, for various reasons. ( NET_Timeout will use the
async close mechanism to signal/interrupt a thread blocked in a poll /
select ). Also, select is used on Mac, which will use poll after your
changes ( I do remember that there was a bug in poll on Mac around
the time of the original Mac port ).

Would is be better, rather than replicating the logic in NET_Timeout,
to have a version that supports passing a function pointer, to run if
the poll/select returns before the timeout? Just an idea.

-Chris.


Thanks,

Vyom


On Monday 05 September 2016 08:37 PM, Chris Hegarty wrote:

On 05/09/16 15:37, Mark Sheppard wrote:

if the desire is to avoid making double calls on gettimeofday in the
NET_ReadWithTimeout's while loop for its main call flow,

Yes, the desire is to make no more calls to gettimeofday, than are
already done.


then consider a modification to NET_Timeout and create a version
int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time)

int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time) {
  int result;
  struct timeval t;
  long prevtime, newtime;
  struct pollfd pfd;
  pfd.fd = s;
  pfd.events = POLLIN;

  if (timeout > 0) {
  prevtime = (current_time->tv_sec * 1000)  +
current_time->tv_usec / 1000;
  }

  for(;;) {
  result = poll(, 1, timeout);
  if (result < 0 && errn

Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

On Thursday 08 September 2016 02:50 PM, Langer, Christoph wrote:

Hi Vyom,

this looks good.

Is there any particular reason why NET_ReadWithTimeout should remain in 
SocketInputStream.c and not also move to net_util_md.c? That way you could also have a 
"static long getCurrentTime()" inside net_util_md.c, instead of an exported 
NET_GetCurrentTime().
I thought this "NET_ReadWithTimeou" is specific to SocketInputStream so 
i will be good if we put this method to socketinputstream.c.


In future we will in using the monotonic increasing clock so 
"NET_GetCurrentTime()" will help , Just change the implementation you 
are done.


Vyom

And no "#include " would be needed in SocketInputStream.c - maybe 
not even now as it is.

Best regards
Christoph


-Original Message-
From: Vyom Tewari [mailto:vyom.tew...@oracle.com]
Sent: Donnerstag, 8. September 2016 05:20
To: Langer, Christoph <christoph.lan...@sap.com>
Cc: net-dev@openjdk.java.net
Subject: Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with
soTimeout set

Hi All,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.5/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.5/index.html>).

I fixed the AIX flag issue and move some come common code to
"net_util_md.c" file.

Thanks,
Vyom

On 9/8/2016 12:32 PM, Langer, Christoph wrote:

Hi Vyom,

ok, thanks. I have one addition to my proposal: I don't think there's a need for

a global NET_GetCurrentTime function. This one should probably be a static
function inside net_util_md.c (static long getCurrentTime()).

Best
Christoph


-Original Message-
From: Vyom Tewari [mailto:vyom.tew...@oracle.com]
Sent: Mittwoch, 7. September 2016 18:31
To: Langer, Christoph <christoph.lan...@sap.com>
Cc: net-dev@openjdk.java.net; Chris Hegarty <chris.hega...@oracle.com>
Subject: Re: RFR 8075484:SocketInputStream.socketRead0 can hang even

with

soTimeout set

Hi Chris,

Sorry I was mostly focusing on our test  failure first, i will check
your suggestions.

Thanks,

Vyom


On Wednesday 07 September 2016 08:44 PM, Langer, Christoph wrote:

Hi Vyom,

did you have a look at my suggestions regarding AIX and refactoring? I

can't

find none of it in the new webrev nor did you comment on it.

Best regards
Christoph


-Original Message-----
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of

Vyom

Tewari
Sent: Mittwoch, 7. September 2016 17:10
To: Chris Hegarty <chris.hega...@oracle.com>
Cc: net-dev@openjdk.java.net
Subject: Re: RFR 8075484:SocketInputStream.socketRead0 can hang even

with

soTimeout set

Hi All,

Please find the latest


webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.4/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.4/index.html>).

there were some test failures in JPRT and Chris also pointed the same
issue in modified code. Now with latest code JPRT is clean.

Thanks,

Vyom


On Wednesday 07 September 2016 03:18 PM, Chris Hegarty wrote:

On 07/09/16 07:45, Vyom Tewari wrote:

Hi Chris,

Please find the latest


webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.3/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.3/index.html>). I

did some code refactoring, JPRT is in progress.

In terms of NET_Timeout**, I'm happy with this version, but I
have an additional comment.

If NET_ReadWithTimeout returns -1 because

NET_TimeoutWithCurrentTime

returns <= 0, then a JNI pending exception will be set. So the code
calling NET_ReadWithTimeout should, a) free bufP, and b) return -1,
immediately rather than continuing and possibly trying to set another
JNI pending exception.

One option is to check the return value from NET_ReadWithTimeout and
also (*env)->ExceptionCheck(env). Another is to possibly consolidate
the setting of JNI pending exceptions in one place, towards the end
of Java_java_net_SocketInputStream_socketRead0, for example EBADF

is

handled in two places.

-Chris.


I need help from some one to build and test latest changes on AIX, may
be Christoph can help.

Thanks,

Vyom


On Tuesday 06 September 2016 06:25 PM, Chris Hegarty wrote:

Vyom,


On 6 Sep 2016, at 10:20, Vyom Tewari <vyom.tew...@oracle.com>

wrote:

Hi,

Please find the latest


webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.2/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.2/index.html>).

I
have incorporated the review comments.

Your changes completely avoid NET_Timeout, which is quite complex

on

Linux, Mac, and AIX, for various reasons. ( NET_Timeout will use the
async close mechanism to signal/interrupt a thread blocked in a poll /
select ). Also, select is used on Mac, which will use poll after your
changes ( I do remember that there was a bug in poll on Mac around
the time of the original Mac port ).

Would is be better, rather than replicating the logic

Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

Hi Mark,

Thanks for the review, even i don't like "goto" , i wanted to do code 
changes as less as possible(not even refactoring) and "goto" helped me 
in this.


I will incorporate the review comment and  send the modified webrev.

Thanks,
Vyom

On 8/30/2016 4:11 PM, Mark Sheppard wrote:


Hi
  perhaps there is an opportunity to do  some refactoring here (...  
for me a "goto " carries a code smell! )


along the lines

if (timeout) {
nread =  NET_ReadWithTimeout(...);
} else {
 nread = NET_Read(...);
}


the NET_ReadWithTimeout (...) function will contain a restructuring of 
your goto loop

while (_timeout > 0) { nread = NET_Timeout(fd, _timeout);
   if (nread <= 0) {
   if (nread == 0) {
   JNU_ThrowByName(env, JNU_JAVANETPKG "SocketTimeoutException",
   "Read timed out");
   } else if (nread == -1) {
   if (errno == EBADF) {
JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException", 
"Socket closed");
   } else if (errno == ENOMEM) {
   JNU_ThrowOutOfMemoryError(env, "NET_Timeout native heap 
allocation failed");
   } else {
   JNU_ThrowByNameWithMessageAndLastError
   (env, JNU_JAVANETPKG "SocketException", "select/poll 
failed");
   }
   }
 // release buffer in main call flow
//  if (bufP != BUF) {
//  free(bufP);
// }
  nread = -1;
  break;
   } else {
nread = NET_NonBlockingRead(fd, bufP, len);
if (nread == -1 && ((errno == EAGAIN) || (errno == EWOULDBLOCK))) {
   gettimeofday(, NULL);
newtime = t.tv_sec * 1000 + t.tv_usec / 1000;
_timeout -= newtime - prevtime;
if(_timeout > 0){
prevtime = newtime;
   }
} else { break; } } } return nread;

e

regards
Mark


On 29/08/2016 10:58, Vyom Tewari wrote:

gentle reminder, please review the below code change.

Vyom


On Monday 22 August 2016 05:12 PM, Vyom Tewari wrote:

Hi All,

Please review the code changes for below issue.

Bug : https://bugs.openjdk.java.net/browse/JDK-8075484

webrev: 
http://cr.openjdk.java.net/~vtewari/8075484/webrev0.0/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.0/index.html>


This issue is SocketInputStream.socketread0() hangs even with 
"soTimeout" set.the implementation of 
Java_java_net_SocketInputStream_socketRead0 assumes that read() 
won't block after poll() reports that a read is possible.


This assumption does not hold, as noted on the man page for select 
(referenced by the man page for poll): Under Linux, select() may 
report a socket file descriptor as "ready for reading", while 
nevertheless a subsequent read blocks. This could for example happen 
when data has arrived but upon examination has wrong checksum and is 
discarded.


Thanks,

Vyom

Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

Hi All,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.4/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.4/index.html>). 
there were some test failures in JPRT and Chris also pointed the same 
issue in modified code. Now with latest code JPRT is clean.


Thanks,

Vyom


On Wednesday 07 September 2016 03:18 PM, Chris Hegarty wrote:



On 07/09/16 07:45, Vyom Tewari wrote:

Hi Chris,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.3/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.3/index.html>). I
did some code refactoring, JPRT is in progress.


In terms of NET_Timeout**, I'm happy with this version, but I
have an additional comment.

If NET_ReadWithTimeout returns -1 because NET_TimeoutWithCurrentTime
returns <= 0, then a JNI pending exception will be set. So the code
calling NET_ReadWithTimeout should, a) free bufP, and b) return -1,
immediately rather than continuing and possibly trying to set another
JNI pending exception.

One option is to check the return value from NET_ReadWithTimeout and
also (*env)->ExceptionCheck(env). Another is to possibly consolidate 
the setting of JNI pending exceptions in one place, towards the end

of Java_java_net_SocketInputStream_socketRead0, for example EBADF is
handled in two places.

-Chris.


I need help from some one to build and test latest changes on AIX, may
be Christoph can help.

Thanks,

Vyom


On Tuesday 06 September 2016 06:25 PM, Chris Hegarty wrote:

Vyom,


On 6 Sep 2016, at 10:20, Vyom Tewari <vyom.tew...@oracle.com> wrote:

Hi,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.2/index.html 

<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.2/index.html>). 
I

have incorporated the review comments.

Your changes completely avoid NET_Timeout, which is quite complex on
Linux, Mac, and AIX, for various reasons. ( NET_Timeout will use the
async close mechanism to signal/interrupt a thread blocked in a poll /
select ). Also, select is used on Mac, which will use poll after your
changes ( I do remember that there was a bug in poll on Mac around
the time of the original Mac port ).

Would is be better, rather than replicating the logic in NET_Timeout,
to have a version that supports passing a function pointer, to run if
the poll/select returns before the timeout? Just an idea.

-Chris.


Thanks,

Vyom


On Monday 05 September 2016 08:37 PM, Chris Hegarty wrote:

On 05/09/16 15:37, Mark Sheppard wrote:

if the desire is to avoid making double calls on gettimeofday in the
NET_ReadWithTimeout's while loop for its main call flow,

Yes, the desire is to make no more calls to gettimeofday, than are
already done.


then consider a modification to NET_Timeout and create a version
int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time)

int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time) {
int result;
struct timeval t;
long prevtime, newtime;
struct pollfd pfd;
pfd.fd = s;
pfd.events = POLLIN;

if (timeout > 0) {
prevtime = (current_time->tv_sec * 1000)  +
current_time->tv_usec / 1000;
}

for(;;) {
result = poll(, 1, timeout);
if (result < 0 && errno == EINTR) {
if (timeout > 0) {
gettimeofday(, NULL);
newtime = (t.tv_sec * 1000)  + t.tv_usec /1000;
timeout -= newtime - prevtime;
if (timeout <= 0)
return 0;
prevtime = newtime;
}
} else {
return result;
}
}
}

the NET_ReadWithTimeout is modified with.

   while (timeout > 0) {
result = NET_TimeoutWithCurrentTime(fd, timeout, );

in any case there is the potential for multiple invocation of
gettimeofday due to a need to make
poll restartable,

Yes, and that is fine. Just no more than is already there.

and adjust the originally specified timeout

accordingly, and for the edge case
after the non blocking read.

After an initial skim, your suggestion seems reasonable.

-Chris.


regards
Mark



On 05/09/2016 12:02, Chris Hegarty wrote:

Vyom,

webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html 




There is some concern about the potential performance effect, etc,
of gettimeofday, maybe there is a way out of this. The reuse of
NET_Timeout is good, but it also calls gettimeofday. It seems that
a specific NET_ReadWithTimeout could be written to NOT reuse
NET_Timeout, but effectively inline its interruptible operation.
Or write a variant of NET_Timeout that takes a function to
execute. Rather than effectively two loops conditioned on the
timeout.  Disclaimer: I have not actually tried to do this, but
it seems worth considering / evaluating.

-Chris.

On 02/09/16 04:39, Vyom Tewari wrote:

hi Dimitry,

thanks for review, I d

RFR 8153674: Expected SocketException not thrown when calling bind() with setReuseAddress(false)

[Vyom Tewari]

Hi All,

Please review the below code change.

Bug: https://bugs.openjdk.java.net/browse/JDK-8153674

Webrev  : 
http://cr.openjdk.java.net/~vtewari/8153674/webrev0.0/index.html 



This change override the "get/setReuseAddress"  for MulticaseSocket and 
will abstract with both reuse attributes(SO_REUSEADDR & SO_REUSEPORT).


Thanks,

Vyom

RFR 8163482: java.net.URLPermission.getActions() adds a trailing colon when header-names is empty

[Vyom Tewari]

Hi All,

Please find the spec change for below issue.

BugId  : https://bugs.openjdk.java.net/browse/JDK-8163482

Webrev   : 
http://cr.openjdk.java.net/~vtewari/8163482/webrev0.0/index.html 



The reason of this change  is a side effect of a recent past fix [1] 
changed the behavior of URLPermission to omit the colon separator when 
the request headers list is empty (the spec allows this). However some 
previously existing tests started failing [2] because of that.


As the spec allows the colon separator to be always  present, whether 
the request list is empty or not. For the sake of backward compatibility 
with existing code that might expect getActions() to always return a 
string containing a colon we chooses the conservative approach and 
continue to add the colon separator in all cases.


 [1] https://bugs.openjdk.java.net/browse/JDK-8114860
 [2] https://bugs.openjdk.java.net/browse/JDK-8161291

Note: i will file a CCC for this spec change.

Thanks,

Vyom

Re: RFR 8163482: java.net.URLPermission.getActions() adds a trailing colon when header-names is empty

[Vyom Tewari]

On Thursday 06 October 2016 03:04 PM, Daniel Fuchs wrote:

Hi Vyom,

So if I understand well, the class level API documentation allowed
for the colon to be either omitted or present:

 121  * 
 122  * The colon separator need not be present if the request headers 
list is empty.


but the getActions() javadoc indicated that the colon would be
omitted if there were no headers:

 219  * There is no white space in the returned String. If 
header-names is empty

 220  * then the colon separator will not be present.

However, the JDK implementation used to always include the
colon, in contradiction to lines #219-220.
We tried to bring the implementation in conformance to line
#219-220, but this caused failures in serialization tests
(see JDK-8161291 [2]) - so we reverted to always include
the colon for the sake of interoperability and serial-compatibility
with previous releases.

Now this fix (8163482) is just to alter the spec of getActions(),
lines #219-220 to match the spec at line #122 and acknowledge
the existing behavior.

Did I get it right?


yes , you are right.
Vyom


best regards,

-- daniel


On 06/10/16 08:31, Vyom Tewari wrote:

Hi All,

Please find the spec change for below issue.

BugId  : https://bugs.openjdk.java.net/browse/JDK-8163482

Webrev   :
http://cr.openjdk.java.net/~vtewari/8163482/webrev0.0/index.html
<http://cr.openjdk.java.net/%7Evtewari/8163482/webrev0.0/index.html>

The reason of this change  is a side effect of a recent past fix [1]
changed the behavior of URLPermission to omit the colon separator when
the request headers list is empty (the spec allows this). However some
previously existing tests started failing [2] because of that.

As the spec allows the colon separator to be always  present, whether
the request list is empty or not. For the sake of backward compatibility
with existing code that might expect getActions() to always return a
string containing a colon we chooses the conservative approach and
continue to add the colon separator in all cases.

 [1] https://bugs.openjdk.java.net/browse/JDK-8114860
 [2] https://bugs.openjdk.java.net/browse/JDK-8161291

Note: i will file a CCC for this spec change.

Thanks,

Vyom

Re: RFR - 8159410: InetAddress.isReachable returns true for non existing IP addresses

[Vyom Tewari]

Hi Rob,

Do you really think this extra check is required ?

if (pEchoReply->Status == IP_SUCCESS
+ && (int)pEchoReply->RoundTripTime <= timeout) I did not found any 
doc(MSDN) which explains this. I think in case of IP_SUCCESS 
"RoundTripTime" is always less than timeout. I think similar changes is 
required in Inet6Address.c as well ? Thanks, Vyom



On Wednesday 21 September 2016 08:46 PM, Rob McKenna wrote:

Hi folks,

I'd like to fix a bug caused by an incorrect assumption. The IcmpSendEcho* 
calls can actually return a similar set of errors regardless of whether the 
call itself failed or succeeded. This change checks that both the call and the 
ping were successful. In addition to that it takes a number of common failure 
causes into account before deciding to throw an exception.

http://cr.openjdk.java.net/~robm/8159410/webrev.01/

-Rob

Re: RFR - 8159410: InetAddress.isReachable returns true for non existing IP addresses

[Vyom Tewari]

So InetAddress.isReachable() will return false if the underline API 
IcmpSendEcho return with Status== IP_SUCESS and RoundTripTime > timeout.


Vyom


On Wednesday 21 September 2016 10:39 PM, Rob McKenna wrote:

Unfortunately the behaviour described is undocumented and was found the hard 
way. This part of the code is a necessity though.

-Rob

On 21/09/16 09:48, Vyom Tewari wrote:

Hi Rob,

Do you really think this extra check is required ?

if (pEchoReply->Status == IP_SUCCESS
+ && (int)pEchoReply->RoundTripTime <= timeout) I did not found any
doc(MSDN) which explains this. I think in case of IP_SUCCESS "RoundTripTime"
is always less than timeout. I think similar changes is required in
Inet6Address.c as well ? Thanks, Vyom


On Wednesday 21 September 2016 08:46 PM, Rob McKenna wrote:

Hi folks,

I'd like to fix a bug caused by an incorrect assumption. The IcmpSendEcho* 
calls can actually return a similar set of errors regardless of whether the 
call itself failed or succeeded. This change checks that both the call and the 
ping were successful. In addition to that it takes a number of common failure 
causes into account before deciding to throw an exception.

http://cr.openjdk.java.net/~robm/8159410/webrev.01/

-Rob

Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

Hi Chris,

Sorry I was mostly focusing on our test  failure first, i will check 
your suggestions.


Thanks,

Vyom


On Wednesday 07 September 2016 08:44 PM, Langer, Christoph wrote:

Hi Vyom,

did you have a look at my suggestions regarding AIX and refactoring? I can't 
find none of it in the new webrev nor did you comment on it.

Best regards
Christoph


-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Vyom
Tewari
Sent: Mittwoch, 7. September 2016 17:10
To: Chris Hegarty <chris.hega...@oracle.com>
Cc: net-dev@openjdk.java.net
Subject: Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with
soTimeout set

Hi All,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.4/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.4/index.html>).
there were some test failures in JPRT and Chris also pointed the same
issue in modified code. Now with latest code JPRT is clean.

Thanks,

Vyom


On Wednesday 07 September 2016 03:18 PM, Chris Hegarty wrote:


On 07/09/16 07:45, Vyom Tewari wrote:

Hi Chris,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.3/index.html
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.3/index.html>). I
did some code refactoring, JPRT is in progress.

In terms of NET_Timeout**, I'm happy with this version, but I
have an additional comment.

If NET_ReadWithTimeout returns -1 because NET_TimeoutWithCurrentTime
returns <= 0, then a JNI pending exception will be set. So the code
calling NET_ReadWithTimeout should, a) free bufP, and b) return -1,
immediately rather than continuing and possibly trying to set another
JNI pending exception.

One option is to check the return value from NET_ReadWithTimeout and
also (*env)->ExceptionCheck(env). Another is to possibly consolidate
the setting of JNI pending exceptions in one place, towards the end
of Java_java_net_SocketInputStream_socketRead0, for example EBADF is
handled in two places.

-Chris.


I need help from some one to build and test latest changes on AIX, may
be Christoph can help.

Thanks,

Vyom


On Tuesday 06 September 2016 06:25 PM, Chris Hegarty wrote:

Vyom,


On 6 Sep 2016, at 10:20, Vyom Tewari <vyom.tew...@oracle.com> wrote:

Hi,

Please find the latest


webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.2/index.html



<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.2/index.html>).

I
have incorporated the review comments.

Your changes completely avoid NET_Timeout, which is quite complex on
Linux, Mac, and AIX, for various reasons. ( NET_Timeout will use the
async close mechanism to signal/interrupt a thread blocked in a poll /
select ). Also, select is used on Mac, which will use poll after your
changes ( I do remember that there was a bug in poll on Mac around
the time of the original Mac port ).

Would is be better, rather than replicating the logic in NET_Timeout,
to have a version that supports passing a function pointer, to run if
the poll/select returns before the timeout? Just an idea.

-Chris.


Thanks,

Vyom


On Monday 05 September 2016 08:37 PM, Chris Hegarty wrote:

On 05/09/16 15:37, Mark Sheppard wrote:

if the desire is to avoid making double calls on gettimeofday in the
NET_ReadWithTimeout's while loop for its main call flow,

Yes, the desire is to make no more calls to gettimeofday, than are
already done.


then consider a modification to NET_Timeout and create a version
int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time)

int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time) {
 int result;
 struct timeval t;
 long prevtime, newtime;
 struct pollfd pfd;
 pfd.fd = s;
 pfd.events = POLLIN;

 if (timeout > 0) {
 prevtime = (current_time->tv_sec * 1000)  +
current_time->tv_usec / 1000;
 }

 for(;;) {
 result = poll(, 1, timeout);
 if (result < 0 && errno == EINTR) {
 if (timeout > 0) {
 gettimeofday(, NULL);
 newtime = (t.tv_sec * 1000)  + t.tv_usec /1000;
 timeout -= newtime - prevtime;
 if (timeout <= 0)
 return 0;
 prevtime = newtime;
 }
 } else {
 return result;
 }
 }
}

the NET_ReadWithTimeout is modified with.

while (timeout > 0) {
 result = NET_TimeoutWithCurrentTime(fd, timeout, );

in any case there is the potential for multiple invocation of
gettimeofday due to a need to make
poll restartable,

Yes, and that is fine. Just no more than is already there.

and adjust the originally specified timeout

accordingly, and for the edge case
after the non blocking read.

After an initial skim, your suggestion seems reasonable.

-Chris.


regards
Mark



On 05/09/2016 12:02, Chris Hegarty wrote:

Vyom,



webrev

Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

Hi Chris,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.3/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.3/index.html>). I 
did some code refactoring, JPRT is in progress.


I need help from some one to build and test latest changes on AIX, may 
be Christoph can help.


Thanks,

Vyom


On Tuesday 06 September 2016 06:25 PM, Chris Hegarty wrote:

Vyom,


On 6 Sep 2016, at 10:20, Vyom Tewari <vyom.tew...@oracle.com> wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.2/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.2/index.html>). I have 
incorporated the review comments.

Your changes completely avoid NET_Timeout, which is quite complex on
Linux, Mac, and AIX, for various reasons. ( NET_Timeout will use the
async close mechanism to signal/interrupt a thread blocked in a poll /
select ). Also, select is used on Mac, which will use poll after your
changes ( I do remember that there was a bug in poll on Mac around
the time of the original Mac port ).

Would is be better, rather than replicating the logic in  NET_Timeout,
to have a version that supports passing a function pointer, to run if
the poll/select returns before the timeout? Just an idea.

-Chris.


Thanks,

Vyom


On Monday 05 September 2016 08:37 PM, Chris Hegarty wrote:

On 05/09/16 15:37, Mark Sheppard wrote:

if the desire is to avoid making double calls on gettimeofday in the
NET_ReadWithTimeout's while loop for its main call flow,

Yes, the desire is to make no more calls to gettimeofday, than are
already done.


then consider a modification to NET_Timeout and create a version
int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time)

int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval *
current_time) {
int result;
struct timeval t;
long prevtime, newtime;
struct pollfd pfd;
pfd.fd = s;
pfd.events = POLLIN;

if (timeout > 0) {
prevtime = (current_time->tv_sec * 1000)  +
current_time->tv_usec / 1000;
}

for(;;) {
result = poll(, 1, timeout);
if (result < 0 && errno == EINTR) {
if (timeout > 0) {
gettimeofday(, NULL);
newtime = (t.tv_sec * 1000)  +  t.tv_usec /1000;
timeout -= newtime - prevtime;
if (timeout <= 0)
return 0;
prevtime = newtime;
}
} else {
return result;
}
}
}

the NET_ReadWithTimeout is modified with.

   while (timeout > 0) {
result = NET_TimeoutWithCurrentTime(fd, timeout, );

in any case there is the potential for multiple invocation of
gettimeofday due to a need to make
poll restartable,

Yes, and that is fine. Just no more than is already there.

and adjust the originally specified timeout

accordingly, and for the edge case
after the non blocking read.

After an initial skim, your suggestion seems reasonable.

-Chris.


regards
Mark



On 05/09/2016 12:02, Chris Hegarty wrote:

Vyom,

webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html

There is some concern about the potential performance effect, etc,
of gettimeofday, maybe there is a way out of this. The reuse of
NET_Timeout is good, but it also calls gettimeofday. It seems that
a specific NET_ReadWithTimeout could be written to NOT reuse
NET_Timeout, but effectively inline its interruptible operation.
Or write a variant of NET_Timeout that takes a function to
execute. Rather than effectively two loops conditioned on the
timeout.  Disclaimer: I have not actually tried to do this, but
it seems worth considering / evaluating.

-Chris.

On 02/09/16 04:39, Vyom Tewari wrote:

hi Dimitry,

thanks for review, I did consider to use  a monotonically increasing
clock like "clock_gettime" but  existing nearby code("NET_Timeout") uses
"gettimeofday"  so i choose to be consistent with the existing code.

Thanks,

Vyom


On Friday 02 September 2016 01:38 AM, Dmitry Samersoff wrote:

Vyom,

Did you consider to use select() to calculate timeout instead of
gettimeofday ?

gettimeofday is affected by system time changes, so running ntpd can
cause unpredictable behavior of this code. Also it's rather expensive
syscall.

-Dmitry

On 2016-09-01 19:03, Vyom Tewari wrote:

please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8075484/webrev0.1/index.html

<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.1/index.html>).
I
incorporated the review comments.

Thanks,

Vyom


On Tuesday 30 August 2016 04:11 PM, Mark Sheppard wrote:

Hi
   perhaps there is an opportunity to do  some refactoring here (...
for me a "goto " carries a code smell! )

along the lines

if (timeout) {
 nread =  NET_ReadWithTimeout(...);
} else {
  nread = NET_Read(...);
}


the NET_ReadWithTimeout (...) funct

Re: RFR 8075484:SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

gentle reminder, please review the below code change.

Vyom


On Monday 22 August 2016 05:12 PM, Vyom Tewari wrote:

Hi All,

Please review the code changes for below issue.

Bug : https://bugs.openjdk.java.net/browse/JDK-8075484

webrev: 
http://cr.openjdk.java.net/~vtewari/8075484/webrev0.0/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8075484/webrev0.0/index.html>


This issue is SocketInputStream.socketread0() hangs even with 
"soTimeout" set.the implementation of 
Java_java_net_SocketInputStream_socketRead0 assumes that read() won't 
block after poll() reports that a read is possible.


This assumption does not hold, as noted on the man page for select 
(referenced by the man page for poll): Under Linux, select() may 
report a socket file descriptor as "ready for reading", while 
nevertheless a subsequent read blocks. This could for example happen 
when data has arrived but upon examination has wrong checksum and is 
discarded.


Thanks,

Vyom

Re: RFR 8153674: Expected SocketException not thrown when calling bind() with setReuseAddress(false)

[Vyom Tewari]

Hi All,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8153674/webrev0.1/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8153674/webrev0.1/index.html>). I 
had removed the SO_REUSEPORT & spec from constructor.


Thanks,

Vyom


On Thursday 29 September 2016 01:38 AM, Chris Hegarty wrote:

Thank you Lucy,

We’ll proceed with removing the setting of SO_REUSEPORT from the
MulticastSocket constructor and spec.

-Chris.

On 28 Sep 2016, at 20:02, Lu, Yingqi <yingqi...@intel.com 
<mailto:yingqi...@intel.com>> wrote:


Hi Vyom,
Thank you very much checking with us.
We agree that SO_RESUEADDR and SO_REUSEPORT behave the same way for 
MulticastSocket. There is no need to check and enable SO_REUSEPORT 
for this particular type of socket. SO_REUSEADDR is sufficient.

Thanks,
Lucy
*From:*Vyom Tewari [mailto:vyom.tew...@oracle.com]
*Sent:*Wednesday, September 28, 2016 1:26 AM
*To:*Chris Hegarty <chris.hega...@oracle.com 
<mailto:chris.hega...@oracle.com>>; Mark Sheppard 
<mark.shepp...@oracle.com <mailto:mark.shepp...@oracle.com>>; net-dev 
<net-dev@openjdk.java.net <mailto:net-dev@openjdk.java.net>>; 
Kaczmarek, Eric <eric.kaczma...@intel.com 
<mailto:eric.kaczma...@intel.com>>; Viswanathan, Sandhya 
<sandhya.viswanat...@intel.com 
<mailto:sandhya.viswanat...@intel.com>>; Kharbas, Kishor 
<kishor.khar...@intel.com <mailto:kishor.khar...@intel.com>>; Aundhe, 
Shirish <shirish.aun...@intel.com <mailto:shirish.aun...@intel.com>>; 
Lu, Yingqi <yingqi...@intel.com <mailto:yingqi...@intel.com>>
*Subject:*Re: RFR 8153674: Expected SocketException not thrown when 
calling bind() with setReuseAddress(false)


Hi All,

I had off line discussion here at Oracle and we decided  not to 
override getReuseAddr/setReuseAddr for MulticastSocket. If user 
wants, he can set the SO_REUSEPORT with "setOption"before bind.


For MulticastSocketSO_REUSEADDR_REUSEPORT are semantically 
equivalent which means either option will be sufficient to indicate 
that the address is reusable. So setting SO_REUSEPORT in 
constructor is really necessary/required ?


I am looking some comments on this from Intel people(they are in mail 
chain) who did this original change, before we decide how we wants to 
proceed on this issue.


Thanks,

Vyom

On Wednesday 14 September 2016 08:47 PM, Chris Hegarty wrote:

On 14/09/16 15:53, Mark Sheppard wrote:


that's true wrt SO_REUSEPORT in MulticastSocket constructor.
But the
same could have been argued for the original
invocation of setReuseAddress, by default , in the
constructors, which
is encapsulating, what pereceived as, common or defacto
practice wrt applying SO_REUSEADDR on mcast sockets at the
system level.
As I understand it, it is generally perceived that
SO_REUSEADDR and
SO_REUSEPORT are semantically equivalent for multicast sockets.
As such, I think in the case of MulticastSocket, the fact
that the
setRuseAddress() is called in the constructor, it is appropriate
to set SO_REUSEPORT also when it exists in the OS.

I take your point on the semantics of setReuseAddress in
DatagramSocket
as per its spec. The spec does allude to MulticastSocket.
As such, the current proposal's changes just lack the appropriate
javadoc to describe its behavior, and its additional
functionality of
setting SO_REUSEPORT.
It is not necessary that overridden method should mirror the
semantics
of the base class method.
If it is accepted that it is generally perceived that
SO_REUSEADDR and
SO_REUSEPORT are semantically equivalent for multicast sockets,
then it seems appropriate that an overriding
setReuseAddress(..) method
in MulticastSocket can reflect this.


That sounds reasonable.

-Chris.


regards
Mark



On 14/09/2016 14:58, Chris Hegarty wrote:

One additional remark.

Was it appropriate to update the legacy MC constructors
to set the new JDK 9 SO_REUSEPORT in the first place?
This can be achievable, opt-in from new code, by creating
an unbound MS, setting the option, then binding.

-Chris.

On 14/09/16 14:47, Chris Hegarty wrote:

Mark,

On 14/09/16 14:22, Mark Sheppard wrote:


Hi Chris,
 I don't fully understand your objections to
the approach taken.
Is there a compatibility issue with the addition
of the additional
methods to MulticastSocket?


The concern is with setReuseAddress performing an
operation that
  

Re: RFR 8153674: Expected SocketException not thrown when calling bind() with setReuseAddress(false)

[Vyom Tewari]

s.

-Chris.



regards
Mark

On 14/09/2016 13:34, Chris Hegarty wrote:

Mark,

On 14/09/16 13:23, Mark Sheppard wrote:


Hi Chris,
so are you accepting that it is correct to add the overridden
methods in MulticastSocket and that these need
appropriate javadoc ?


I think we need these, but they should just call their super
equivalents, i.e. no implicit setting of SO_REUSEPORT. They would
exist solely as a place to locate guidance, or a note, that one
will likely want to set SO_REUSEPORT too.


or are you advocating pushing the handing of the SO_REUSEPORT into
the
base DatagramSocket class ?


It is already there. I am not proposing to change this.


It is not clear how your code changes fit in the proposed fix i.e.
the
explicit setting of the option to false?


My proposal is an alternative. It is not related to the current
webrev.


With the current proposed changes then I think it would be
sufficient to
invoke setReuseAddress(true) in MulticastSocket constructors
rather than

// Enable SO_REUSEADDR before binding
setReuseAddress
<https://java.se.oracle.com/source/s?defs=setReuseAddress=jdk9-dev>(*true*); 






// Enable SO_REUSEPORT if supported before binding
*if* (supportedOptions
<https://java.se.oracle.com/source/xref/jdk9-dev/jdk/src/java.base/share/classes/java/net/MulticastSocket.java#supportedOptions>().contains 





<https://java.se.oracle.com/source/s?defs=contains=jdk9-dev>(StandardSocketOptions 





<https://java.se.oracle.com/source/s?defs=StandardSocketOptions=jdk9-dev>.SO_REUSEPORT 





<https://java.se.oracle.com/source/s?defs=SO_REUSEPORT=jdk9-dev>)) 




{
*this*.setOption
<https://java.se.oracle.com/source/s?defs=setOption=jdk9-dev>(StandardSocketOptions 





<https://java.se.oracle.com/source/s?defs=StandardSocketOptions=jdk9-dev>.SO_REUSEPORT 





<https://java.se.oracle.com/source/s?defs=SO_REUSEPORT=jdk9-dev>, 




*true*);
}


as the overridden setReuseAddress takes care of SO_REUSEPORT


Yes, this is what Vyom has proposed, in the webrev.

I would like to explore an alternative, so see what it would look
like.

-Chris.



regards
Mark

On 14/09/2016 11:43, Chris Hegarty wrote:

Vyom,

On 11/09/16 08:01, Vyom Tewari wrote:

Hi All,

Please review the below code change.

Bug: https://bugs.openjdk.java.net/browse/JDK-8153674

Webrev  :
http://cr.openjdk.java.net/~vtewari/8153674/webrev0.0/index.html
<http://cr.openjdk.java.net/%7Evtewari/8153674/webrev0.0/index.html> 




This change override the "get/setReuseAddress" for 
MulticaseSocket

and
will abstract with both reuse attributes(SO_REUSEADDR &
SO_REUSEPORT).


This issue arises since the changes for 6432031 "Add support for
SO_REUSEPORT" [1], which sets SO_REUSEPORT on MulticastSocket, if
the available. So setting setReuseAddress(false) alone is no 
longer

sufficient to disable address/port reuse, one must also set
SO_REUSEPORT to false.

I would be really nervous about changing set/getReuseAddress,
without
at least updating the javadoc to indicate that it is now, for MS,
operating on two socket options.  Although, I do have sympathy
here since SO_REUSEPORT and SO_REUSEADDR are almost identical when
dealing with multicasting.

An alternative, maybe; Since the MS constructors document that
SO_REUSEPORT will be set, where available, maybe a javadoc note
on the set/getReuseAddress methods would be sufficient, that
indicates that StandardSocketOptions#SO_REUSEPORT may also need
to be set to have the desired effect?

If so, then code would have to:

setReuseAddress(false);

if
(supportedOptions().contains(StandardSocketOptions.SO_REUSEPORT))
this.setOption(StandardSocketOptions.SO_REUSEPORT, false);

  , but at least it is explicit.

Q: not all MS constructors document that SO_REUSEPORT is set, but
they should, right? This seems to have slipped past during 6432031
[1].

-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-6432031

RFR 8168840: InetAddress.getByName() throws java.net.UnknownHostException no such interface when used with virtual interfaces on Solaris

[Vyom Tewari]

Hi,

Please review the code changes for below issue.

BugId: https://bugs.openjdk.java.net/browse/JDK-8168840

webrev : 
http://cr.openjdk.java.net/~vtewari/8168840/webrev0.0/index.html 



Thanks,

Vyom

Re: [8u-dev]: Request for Review and Approval: 8075484: SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

Hi Christoph,

As this is not direct backport what about using the existing function 
"JVM_CurrentTimeMillis(env, 0);" instead of "NET_GetCurrentTime() ". 
When i did this code change i did not know that we already have this.If 
you use the existing one then you can simply remove the 
"NET_GetCurrentTime() "  from your code change.


Thanks,

Vyom


On Thursday 29 December 2016 03:07 PM, Langer, Christoph wrote:


Hi,

please review (and eventually approve) the change for downporting 8075484.

Webrev for 8u-dev: 
http://cr.openjdk.java.net/~clanger/webrevs/8075484.8udev/ 



Bug: https://bugs.openjdk.java.net/browse/JDK-8075484 



JDK9 Change: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/af17b6bc08dd

JDK9 Review Thread(s):

http://mail.openjdk.java.net/pipermail/net-dev/2016-August/010171.html

http://mail.openjdk.java.net/pipermail/net-dev/2016-September/010201.html

We had customer reports who ran into that issue with Java 8. So this 
should be downported.


The problem is, that the fix does not apply to Solaris as Solaris 
needs some calls into hotspot. This is because in JDK 8 the flag for 
interruptible IO is still supported (though deprecated). But I think 
it is still worthwile to bring this down for the other platforms which 
I’m proposing with my changeset. So I extracted the new code manually 
from the JDK9 changeset and made it fit into JDK8 coding.


Thanks & Best regards

Christoph

Re: [8u-dev]: Request for Review and Approval: 8075484: SocketInputStream.socketRead0 can hang even with soTimeout set

[Vyom Tewari]

Hi Christoph,

Code change looks good to me, but i am not an official reviewer.

Thanks,

Vyom


On Monday 09 January 2017 11:26 AM, Langer, Christoph wrote:


Ping: Please review this backport to JDK8.

*From:*Langer, Christoph
*Sent:* Donnerstag, 29. Dezember 2016 10:37
*To:* net-dev@openjdk.java.net; jdk8u-...@openjdk.java.net
*Subject:* [8u-dev]: Request for Review and Approval: 8075484: 
SocketInputStream.socketRead0 can hang even with soTimeout set


Hi,

please review (and eventually approve) the change for downporting 8075484.

Webrev for 8u-dev: 
http://cr.openjdk.java.net/~clanger/webrevs/8075484.8udev/ 



Bug: https://bugs.openjdk.java.net/browse/JDK-8075484 



JDK9 Change: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/af17b6bc08dd

JDK9 Review Thread(s):

http://mail.openjdk.java.net/pipermail/net-dev/2016-August/010171.html

http://mail.openjdk.java.net/pipermail/net-dev/2016-September/010201.html

We had customer reports who ran into that issue with Java 8. So this 
should be downported.


The problem is, that the fix does not apply to Solaris as Solaris 
needs some calls into hotspot. This is because in JDK 8 the flag for 
interruptible IO is still supported (though deprecated). But I think 
it is still worthwile to bring this down for the other platforms which 
I’m proposing with my changeset. So I extracted the new code manually 
from the JDK9 changeset and made it fit into JDK8 coding.


Thanks & Best regards

Christoph

Re: RFR 8168840: InetAddress.getByName() throws java.net.UnknownHostException no such interface when used with virtual interfaces on Solaris

[Vyom Tewari]

Hi All,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8168840/webrev0.1/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8168840/webrev0.1/index.html>).


Thanks,

Vyom


On Tuesday 13 December 2016 01:38 PM, Langer, Christoph wrote:

Hi Vyom,

thanks for looking at this. Overall your fix and test look good to me.

The coding to do the parent search and if required a child search in 
Java_java_net_NetworkInterface_getByName0 could be done a bit more 
straightforward, e.g. like this:

 // search the list of interfaces by name
 // for virtual interfaces we need to find the parent first
 colonp = strchr(name_utf, ':');
 if (colonp == NULL) {
 searchName = name_utf;
 } else {
 jio_snprintf(pname, IFNAMESIZE, "%.*s", colonp - name_utf);
 searchName = pname;
 }
 curr = ifs;
 while (curr != NULL) {
 if (strcmp(searchName, curr->name) == 0) {
 break;
 }
 curr = curr->next;
 }

 // search the child list
 if (curr != NULL && colonp != NULL) {
 curr = curr->childs;
 while (curr != NULL) {
 if (strcmp(name_utf, curr->name) == 0) {
 break;
 }
 curr = curr->next;
 }
 }

Best regards
Christoph


-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Vyom
Tewari
Sent: Dienstag, 13. Dezember 2016 04:47
To: net-dev <net-dev@openjdk.java.net>
Subject: RFR 8168840: InetAddress.getByName() throws
java.net.UnknownHostException no such interface when used with virtual
interfaces on Solaris

Hi,

Please review the code changes for below issue.

BugId: https://bugs.openjdk.java.net/browse/JDK-8168840

webrev :
http://cr.openjdk.java.net/~vtewari/8168840/webrev0.0/index.html
<http://cr.openjdk.java.net/%7Evtewari/8168840/webrev0.0/index.html>

Thanks,

Vyom

Re: RFR 8168840: InetAddress.getByName() throws java.net.UnknownHostException no such interface when used with virtual interfaces on Solaris

[Vyom Tewari]

Hi Chris,

If you create the Inet6Address  using the following constructor 
"Inet6Address (String , byte[], String )" then it will call the 
following private method " initstr (hostName, addr, ifname);" and it you 
see the implementation of this method then you will see the below code.


###

try {
NetworkInterface nif = NetworkInterface.getByName (ifname);
if (nif == null) {
throw new UnknownHostException ("no such interface " + 
ifname);

}
initif (hostName, addr, nif);




So this is the connection between Inet6Address and 
NetworkInterface.getByName but same is not the case  for ipv4 address. 
As we added the scope name recently(not very recent but after 
jdk1.7.0_79) so if you pass the virtual sub interface interface 
like(2001:7a8:b0cd:1:0:0:0:17%net0:1) then it will fail to get the sub 
interface and UHE will be thrown.


to handle this i did the code change.


Thanks,
Vyom

On 12/21/2016 5:04 PM, Chris Hegarty wrote:

Hi Vyom,

Sorry, I'm missing the connection between Inet6Address and
NetworkInterface.getByName, how do these interact?

-Chris.

On 21/12/16 10:20, Vyom Tewari wrote:

incorporated the
comments(http://cr.openjdk.java.net/~vtewari/8168840/webrev0.2/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8168840/webrev0.2/index.html>).



Thanks,

Vyom


On Wednesday 21 December 2016 02:58 PM, Langer, Christoph wrote:

Hi Vyom,

looks good, thanks for the update.

Minor formatting:
- Add a blank line between line 258/259 and 268/269 in the new file
version.
- line 259 //search the child list - add a space between "//" and
"search..."

Disclaimer: I'm not an official reviewer.

Best regards
Christoph


-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
Vyom
Tewari
Sent: Mittwoch, 21. Dezember 2016 09:51
Cc: net-dev <net-dev@openjdk.java.net>
Subject: Re: RFR 8168840: InetAddress.getByName() throws
java.net.UnknownHostException no such interface when used with virtual
interfaces on Solaris

Hi All,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8168840/webrev0.1/index.html 


<http://cr.openjdk.java.net/%7Evtewari/8168840/webrev0.1/index.html>).

Thanks,

Vyom


On Tuesday 13 December 2016 01:38 PM, Langer, Christoph wrote:

Hi Vyom,

thanks for looking at this. Overall your fix and test look good to 
me.


The coding to do the parent search and if required a child search in

Java_java_net_NetworkInterface_getByName0 could be done a bit more
straightforward, e.g. like this:

  // search the list of interfaces by name
  // for virtual interfaces we need to find the parent first
  colonp = strchr(name_utf, ':');
  if (colonp == NULL) {
  searchName = name_utf;
  } else {
  jio_snprintf(pname, IFNAMESIZE, "%.*s", colonp - name_utf);
  searchName = pname;
  }
  curr = ifs;
  while (curr != NULL) {
  if (strcmp(searchName, curr->name) == 0) {
  break;
  }
  curr = curr->next;
  }

  // search the child list
  if (curr != NULL && colonp != NULL) {
  curr = curr->childs;
  while (curr != NULL) {
  if (strcmp(name_utf, curr->name) == 0) {
  break;
  }
  curr = curr->next;
  }
  }

Best regards
Christoph


-----Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of

Vyom

Tewari
Sent: Dienstag, 13. Dezember 2016 04:47
To: net-dev <net-dev@openjdk.java.net>
Subject: RFR 8168840: InetAddress.getByName() throws
java.net.UnknownHostException no such interface when used with 
virtual

interfaces on Solaris

Hi,

Please review the code changes for below issue.

BugId: https://bugs.openjdk.java.net/browse/JDK-8168840

webrev :
http://cr.openjdk.java.net/~vtewari/8168840/webrev0.0/index.html
<http://cr.openjdk.java.net/%7Evtewari/8168840/webrev0.0/index.html>

Thanks,

Vyom

Re: RFR 8168840: InetAddress.getByName() throws java.net.UnknownHostException no such interface when used with virtual interfaces on Solaris

[Vyom Tewari]

Hi Chris,

i will do the change( *colonP = '\0';') before pushing still i have to 
run the jprt. Once it is done and clean i will do the suggested change 
and push the code.


thanks,

Vyom


On 12/21/2016 7:23 PM, Chris Hegarty wrote:

Vyom,

Thank you for the explanation. Makes sense.

Your changes look fine as are, but maybe '250 *colonP = '\0';'
would be clearer ( though I do note that we do *name_colonP = 0;
elsewhere in this file ).

-Chris.

On 21/12/16 13:41, Vyom Tewari wrote:

Hi Chris,

If you create the Inet6Address  using the following constructor
"Inet6Address (String , byte[], String )" then it will call the
following private method " initstr (hostName, addr, ifname);" and it you
see the implementation of this method then you will see the below code.

###

try {
NetworkInterface nif = NetworkInterface.getByName (ifname);
if (nif == null) {
throw new UnknownHostException ("no such interface " +
ifname);
}
initif (hostName, addr, nif);




So this is the connection between Inet6Address and
NetworkInterface.getByName but same is not the case  for ipv4 address.
As we added the scope name recently(not very recent but after
jdk1.7.0_79) so if you pass the virtual sub interface interface
like(2001:7a8:b0cd:1:0:0:0:17%net0:1) then it will fail to get the sub
interface and UHE will be thrown.

to handle this i did the code change.


Thanks,
Vyom

On 12/21/2016 5:04 PM, Chris Hegarty wrote:

Hi Vyom,

Sorry, I'm missing the connection between Inet6Address and
NetworkInterface.getByName, how do these interact?

-Chris.

On 21/12/16 10:20, Vyom Tewari wrote:

incorporated the
comments(http://cr.openjdk.java.net/~vtewari/8168840/webrev0.2/index.html 


<http://cr.openjdk.java.net/%7Evtewari/8168840/webrev0.2/index.html>).


Thanks,

Vyom


On Wednesday 21 December 2016 02:58 PM, Langer, Christoph wrote:

Hi Vyom,

looks good, thanks for the update.

Minor formatting:
- Add a blank line between line 258/259 and 268/269 in the new file
version.
- line 259 //search the child list - add a space between "//" and
"search..."

Disclaimer: I'm not an official reviewer.

Best regards
Christoph


-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
Vyom
Tewari
Sent: Mittwoch, 21. Dezember 2016 09:51
Cc: net-dev <net-dev@openjdk.java.net>
Subject: Re: RFR 8168840: InetAddress.getByName() throws
java.net.UnknownHostException no such interface when used with 
virtual

interfaces on Solaris

Hi All,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8168840/webrev0.1/index.html 



<http://cr.openjdk.java.net/%7Evtewari/8168840/webrev0.1/index.html>). 



Thanks,

Vyom


On Tuesday 13 December 2016 01:38 PM, Langer, Christoph wrote:

Hi Vyom,

thanks for looking at this. Overall your fix and test look good to
me.

The coding to do the parent search and if required a child 
search in

Java_java_net_NetworkInterface_getByName0 could be done a bit more
straightforward, e.g. like this:

  // search the list of interfaces by name
  // for virtual interfaces we need to find the parent first
  colonp = strchr(name_utf, ':');
  if (colonp == NULL) {
  searchName = name_utf;
  } else {
  jio_snprintf(pname, IFNAMESIZE, "%.*s", colonp - 
name_utf);

  searchName = pname;
  }
  curr = ifs;
  while (curr != NULL) {
  if (strcmp(searchName, curr->name) == 0) {
  break;
  }
  curr = curr->next;
  }

  // search the child list
  if (curr != NULL && colonp != NULL) {
  curr = curr->childs;
  while (curr != NULL) {
  if (strcmp(name_utf, curr->name) == 0) {
  break;
  }
  curr = curr->next;
  }
  }

Best regards
Christoph


-----Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On 
Behalf Of

Vyom

Tewari
Sent: Dienstag, 13. Dezember 2016 04:47
To: net-dev <net-dev@openjdk.java.net>
Subject: RFR 8168840: InetAddress.getByName() throws
java.net.UnknownHostException no such interface when used with
virtual
interfaces on Solaris

Hi,

Please review the code changes for below issue.

BugId: https://bugs.openjdk.java.net/browse/JDK-8168840

webrev :
http://cr.openjdk.java.net/~vtewari/8168840/webrev0.0/index.html
<http://cr.openjdk.java.net/%7Evtewari/8168840/webrev0.0/index.html> 



Thanks,

Vyom

Re: RFR:8170868: DefaultProxySelector should use system defaults on Windows, MacOS and Gnome

[Vyom Tewari]

On 12/22/2016 4:08 PM, Zeller, Arno wrote:


Hi Vyom,

thanks you for having a look at my patch!

Regarding your suggestion to call CHECK_NULL_RETURN in 
DefaultProxySelector.c:


I guess you mean the Unix variant of DefaultProxySelector.c 
(src/java.base/unix/native/libnet/DefaultProxySelector.c).


419 if (proxies) {

420 if (!error) {

421 int i;

422 // create an empty LinkedList to add the Proxy objects.

423 proxyList = (*env)->NewObject(env, list_class, list_ctrID);

424 if (proxyList != NULL) {

425 for(i = 0; proxies[i]; i++) {

...

454 }

455 }

456 }

457 (*g_strfreev)(proxies);

458 }

There I check in the next line if proxyList is NULL and skip the rest 
in this case, but I cannot return without freeing the memory I got 
from the gnome function call by calling (g_strfreev) later - otherwise 
there would be a memory leak.


yes that true, but if NewObject failed at line 423 then there will be 
pending JNI exception in "getSystemProxy" method which calls 
"getProxyByGProxyResolver" method. I will suggest you to check for any 
JNI exception after calling the "getProxyByGProxyResolver".


And in the Windows case it is the same pattern - at the point where I 
removed the CHECK_NULL_RETURN macros I hold Windows system memory (in 
proxy_info and ie_proxy_config) that I have to free with 
GlobalFree(...) and I also have to release the JNI memory I hold with 
ReleaseStringChars(...).


I hope this explains the motivation of my change at this point.

it make sense but I am not the JNI expert may be some one else can 
comments if it is safe to call the functions like "ReleaseStringChars" 
etc even if there is pending JNI exception before(if NewString fails at 
266) function call.


Thanks,
Vyom


Best regards,

Arno

*From:*net-dev [mailto:net-dev-boun...@openjdk.java.net] *On Behalf Of 
*Vyom Tewari

*Sent:* Mittwoch, 21. Dezember 2016 17:08
*To:* net-dev@openjdk.java.net
*Subject:* Re: RFR:8170868: DefaultProxySelector should use system 
defaults on Windows, MacOS and Gnome


Hi Arno,
I suggest you to call "CHECK_NULL_RETURN" after below line in 
DefaultProxySelector.c

// create an empty LinkedList to add the Proxy objects.
+proxyList = (*env)->NewObject(env, list_class, list_ctrID);
in windows/native/libnet/DefaultProxySelector.c file you remove the 
couple of "CHECK_NULL_RETURN"

 jstring jhost;
-  if (pport == 0)
-pport = defport;
-  jhost = (*env)->NewStringUTF(env, phost);
-  CHECK_NULL_RETURN(jhost, NULL);
-  isa = (*env)->CallStaticObjectMethod(env, isaddr_class, 
isaddr_createUnresolvedID, jhost, pport);

-  CHECK_NULL_RETURN(isa, NULL);
-  proxy = (*env)->NewObject(env, proxy_class, proxy_ctrID, 
type_proxy, isa);

-  return proxy;
+  if (portVal == 0)
+portVal = defport;
+  jhost = (*env)->NewString(env, phost, (jsize)wcslen(phost));
+  if (jhost != NULL) {
+isa = (*env)->CallStaticObjectMethod(env, isaddr_class, 
isaddr_createUnresolvedID, jhost, portVal);

+if (isa != NULL) {
+  jobject proxy = (*env)->NewObject(env, proxy_class, 
proxy_ctrID, type_proxy, isa);

+  if (proxy != NULL) {
+(*env)->CallBooleanMethod(env, proxy_list, 
list_addID, proxy);

+  }
+}
+  }
 }
Is there is specific reason behind removing these checks ?
Thanks,
Vyom

On 12/21/2016 6:23 PM, Zeller, Arno wrote:

Hi Volker,

thanks for your valuable comments! I have a new patch ready that
should address your issues and contains also a forgotten change to
the map file...

New webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170868.1/
<http://cr.openjdk.java.net/%7Eclanger/webrevs/8170868.1/>

- make/lib/NetworkingLibraries.gmk

...

Have you tried to use

LIBNET_EXCLUDE_FILES :=

$(JDK_TOPDIR)/src/java.base/unix/native/libnet/DefaultProxySelector.c

I think this should work and it would mke it possible to rename:

src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c

to:

src/java.base/macosx/native/libnet/DefaultProxySelector.c

which is much nicer IMHO :)

Great idea - it works and is of course the much nicer solution!

- DefaultProxySelector.java

322 return proxyList == null ?
Arrays.asList(Proxy.NO_PROXY) :

proxyList;

Not sure if it would make sense to preallocate a static List
with a single

Proxy.NO_PROXY element and always return that if proxyList
equals null?

I return a new List object each time, because the select(URI uri)
method does not state anything about

not modifying the returned list. In case I return a st

Re: RFR:8170868: DefaultProxySelector should use system defaults on Windows, MacOS and Gnome

[Vyom Tewari]

Hi Arno,

I suggest you to call "CHECK_NULL_RETURN" after below line in 
DefaultProxySelector.c
// create an empty LinkedList to add the Proxy objects.
+ proxyList = (*env)->NewObject(env, list_class, list_ctrID); in 
windows/native/libnet/DefaultProxySelector.c file you remove the couple of 
"CHECK_NULL_RETURN"
 
 jstring jhost;

- if (pport == 0)
- pport = defport;
- jhost = (*env)->NewStringUTF(env, phost);
- CHECK_NULL_RETURN(jhost, NULL);
- isa = (*env)->CallStaticObjectMethod(env, isaddr_class, 
isaddr_createUnresolvedID, jhost, pport);

- CHECK_NULL_RETURN(isa, NULL);
- proxy = (*env)->NewObject(env, proxy_class, proxy_ctrID, type_proxy, isa);
- return proxy;
+ if (portVal == 0)
+ portVal = defport;
+ jhost = (*env)->NewString(env, phost, (jsize)wcslen(phost));
+ if (jhost != NULL) {
+ isa = (*env)->CallStaticObjectMethod(env, isaddr_class, 
isaddr_createUnresolvedID, jhost, portVal);

+ if (isa != NULL) {
+ jobject proxy = (*env)->NewObject(env, proxy_class, proxy_ctrID, 
type_proxy, isa);

+ if (proxy != NULL) {
+ (*env)->CallBooleanMethod(env, proxy_list, list_addID, proxy);
+ }
+ }
+ }
 }


Is there is specific reason behind removing these checks ?

Thanks,
Vyom


On 12/21/2016 6:23 PM, Zeller, Arno wrote:

Hi Volker,

thanks for your valuable comments! I have a new patch ready that should address 
your issues and contains also a forgotten change to the map file...

New webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170868.1/



- make/lib/NetworkingLibraries.gmk

...

Have you tried to use
LIBNET_EXCLUDE_FILES :=
$(JDK_TOPDIR)/src/java.base/unix/native/libnet/DefaultProxySelector.c

I think this should work and it would mke it possible to rename:
src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c
to:
src/java.base/macosx/native/libnet/DefaultProxySelector.c
which is much nicer IMHO :)

Great idea - it works and is of course the much nicer solution!


- DefaultProxySelector.java

322 return proxyList == null ? Arrays.asList(Proxy.NO_PROXY) :
proxyList;

Not sure if it would make sense to preallocate a static List with a single
Proxy.NO_PROXY element and always return that if proxyList equals null?

I return a new List object each time, because the select(URI uri) method does 
not state anything about
not modifying the returned list. In case I return a static list containing only 
the NO_PROXY element
a caller could remove the object from the list and other caller would use the 
same modified list.
To avoid this I always create a new List object.


- java.base/unix/native/libnet/DefaultProxySelector.c

You've removed "#include ". Have you built on all Unix platforms
(AIX, Solaris) to make sure you don't break anything?

It compiled on Linux, AIX, Solaris and Mac without problems for me.


- java.base/windows/native/libnet/DefaultProxySelector.c

Not sure if I understand this right, but in the gconf case above you insert all
proxies returned by "g_proxy_resolver_lookup" into the prox-list returned by
DefaultProxySelector_getSystemProxies. In the Windows case you write:

247* From MSDN: The proxy server list contains one or more of
the following strings separated by semicolons or whitespace.
248* ([=]["://"][":"])
249* We will only take the first entry here because the
java.net.Proxy class has only one entry.

Why can't you build up a proxy list here in the same way you do it for the
gconf case on Unix?

Sorry - I just forgot to implement it. Good that you found it. The new webrev 
contains the missing functionality.


- src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c

  76 #define kResolveProxyRunLoopMode
CFSTR("com.sap.jvm.DefaultProxySelector")


I'm not familiar with the Mac programming model, but I don't think
"com.sap.jvm.DefaultProxySelector" is a good name for
kResolveProxyRunLoopMode. It should be something like
"java.net.DefaultProxySelector" but I'm open for better proposals :)

You are right - I changed it to "sun.net.spi.DefaultProxySelector".


PS: for your next RFR, can you please also add the estimated sisze and the bug
id to the subject line (e.g. "RFR(M):
8170868:DefaultProxySelector should..."). This makes it much easier to find a
review thread :)

I'll do my best next time...

Best regards,
Arno


On Wed, Dec 14, 2016 at 5:06 PM, Zeller, Arno  wrote:

Hi,

can you please review my proposal for bug 8170868 - DefaultProxySelector

should use system defaults on Windows, MacOS and Gnome.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8170868

Webrev:
http://cr.openjdk.java.net/~clanger/webrevs/8170868.0/

Thanks a lot,
Arno Zeller

Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Vyom Tewari]

Hi Roger,

thanks for review, please see my  comment inline.

Vyom


On Wednesday 12 April 2017 08:17 PM, Roger Riggs wrote:

Hi Vyom,

Thanks for taking this on.   I have a few comments and questions.

In aix_close.c line 547 The code for if (nanoTimeout >= NSEC_PER_MSEC) 
seems ineffective.


agreed, but this check was previously there(timeout > 0) and this check 
will prevent the additional call to "JVM_NanoTime/gettimeofday()" in 
modified code. I think  we leave this check as it is and put the 
corresponding else block. Please do let me know your thought on this.
The update of nanoTime at 549-550 ensures the timeout is > 
NSEC_PER_MSEC if it loops.
On the first pass through the code if nanoTimeout was < NSEC_PER_MSEC 
it would
poll with a timeout of zero which returns immediately, EINTR is not 
possible.


The comment at 546 would be inaccurate if nanoTimeout is too small, 
because it will loop again.


right, this is on wrong place it has to be in inner if block. I put this 
comment to avoid the confusion why "nanoTimeout < NET_NSEC_PER_MSEC".
I think the behavior would be the same if the extra condition at 547 
is removed.


Most of the other xxx_close.c classes have the same structure.

PlainSocketImpl.c has the same structure


The bsd_close.c had the same kind of before and after checking but did 
previously
and conservatively I'd keep that structure though I think it was 
ineffective in that case too.


Thanks, Roger




On 4/11/2017 9:04 AM, Vyom Tewari wrote:

Hi,

Please review the code change for below issue.

Bug:   https://bugs.openjdk.java.net/browse/JDK-8165437

Webrev: http://cr.openjdk.java.net/~vtewari/8165437/webrev0.5/index.html

This change will replace the "gettimeofday" to use the monotonic 
increasing clock(i used the existing function "JVM_NanoTime" instead 
of writing my own).


Thanks,

Vyom

Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Vyom Tewari]

On Wednesday 12 April 2017 11:05 PM, Vyom Tewari wrote:


Hi Roger,

thanks for review, please see my  comment inline.

Vyom


On Wednesday 12 April 2017 08:17 PM, Roger Riggs wrote:

Hi Vyom,

Thanks for taking this on.   I have a few comments and questions.

In aix_close.c line 547 The code for if (nanoTimeout >= 
NSEC_PER_MSEC) seems ineffective.


agreed, but this check was previously there(timeout > 0) and this 
check will prevent the additional call to 
"JVM_NanoTime/gettimeofday()" in modified code. I think  we leave this 
check as it is and put the corresponding else block. Please do let me 
know your thought on this.

i think you got it right, the "if" at line 547 is ineffective.
Vyom
The update of nanoTime at 549-550 ensures the timeout is > 
NSEC_PER_MSEC if it loops.
On the first pass through the code if nanoTimeout was < NSEC_PER_MSEC 
it would
poll with a timeout of zero which returns immediately, EINTR is not 
possible.


The comment at 546 would be inaccurate if nanoTimeout is too small, 
because it will loop again.


right, this is on wrong place it has to be in inner if block. I put 
this comment to avoid the confusion why "nanoTimeout < NET_NSEC_PER_MSEC".
I think the behavior would be the same if the extra condition at 547 
is removed.


Most of the other xxx_close.c classes have the same structure.

PlainSocketImpl.c has the same structure


The bsd_close.c had the same kind of before and after checking but 
did previously
and conservatively I'd keep that structure though I think it was 
ineffective in that case too.


Thanks, Roger




On 4/11/2017 9:04 AM, Vyom Tewari wrote:

Hi,

Please review the code change for below issue.

Bug: https://bugs.openjdk.java.net/browse/JDK-8165437

Webrev: 
http://cr.openjdk.java.net/~vtewari/8165437/webrev0.5/index.html


This change will replace the "gettimeofday" to use the monotonic 
increasing clock(i used the existing function "JVM_NanoTime" instead 
of writing my own).


Thanks,

Vyom

JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Vyom Tewari]

Hi,

Please review the code change for below issue.

Bug:   https://bugs.openjdk.java.net/browse/JDK-8165437

Webrev: http://cr.openjdk.java.net/~vtewari/8165437/webrev0.5/index.html

This change will replace the "gettimeofday" to use the monotonic 
increasing clock(i used the existing function "JVM_NanoTime" instead of 
writing my own).


Thanks,

Vyom

Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Vyom Tewari]

Hi Thomas,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.6/index.html) 
i incorporated all the review comments. Regarding why JVM_NanoTime is 
defined JVM_LEAF i don't know much, but all the functions which are 
defined in jvm.h used some sort of 
macro(JVM_LEAF,JVM_ENTRY,JVM_ENTRY_NO_ENV etc). You can not call 
"os::javaTimeNanos()" directly as this is not visible, i did a small 
prototype which simply export this without any JVM macro as below.


This prototype did worked but i am not sure if this is right way to do. 
I need some more input, why all jvm.h functions are defined with macro 
and if it OK to defined function as below, maybe some one from JVM team 
can give some more comment on this. I decided not to include this 
prototype as part of review because i am not sure if this is right way.


Thanks,

Vyom

##

diff -r 26d689c621f6 make/symbols/symbols-unix
--- a/make/symbols/symbols-unixWed Apr 12 19:28:47 2017 -0700
+++ b/make/symbols/symbols-unixTue Apr 18 08:40:25 2017 +0530
@@ -51,6 +51,7 @@
 JVM_CurrentLoadedClass
 JVM_CurrentThread
 JVM_CurrentTimeMillis
+JVM_CurrentTimeNano
 JVM_DefineClass
 JVM_DefineClassWithSource
 JVM_DesiredAssertionStatus
diff -r 26d689c621f6 src/share/vm/prims/jvm.cpp
--- a/src/share/vm/prims/jvm.cppWed Apr 12 19:28:47 2017 -0700
+++ b/src/share/vm/prims/jvm.cppTue Apr 18 08:40:25 2017 +0530
@@ -273,7 +273,11 @@
   JVMWrapper("JVM_NanoTime");
   return os::javaTimeNanos();
 JVM_END
-
+
+long JVM_CurrentTimeNano(){
+return os::javaTimeNanos();
+}
+
 // The function below is actually exposed by jdk.internal.misc.VM and not
 // java.lang.System, but we choose to keep it here so that it stays next
 // to JVM_CurrentTimeMillis and JVM_NanoTime
diff -r 26d689c621f6 src/share/vm/prims/jvm.h
--- a/src/share/vm/prims/jvm.hWed Apr 12 19:28:47 2017 -0700
+++ b/src/share/vm/prims/jvm.hTue Apr 18 08:40:25 2017 +0530
@@ -119,6 +119,9 @@
 JNIEXPORT jlong JNICALL
 JVM_NanoTime(JNIEnv *env, jclass ignored);

+JNIEXPORT jlong
+JVM_CurrentTimeNano();
+
 JNIEXPORT jlong JNICALL
 JVM_GetNanoTimeAdjustment(JNIEnv *env, jclass ignored, jlong offset_secs);
##


On Thursday 13 April 2017 12:40 PM, Thomas Stüfe wrote:

Hi Vyom,

Thank you for fixing this!

In addition to Rogers remarks:

aix_close.c:
Could you please also update the SAP copyright?

style nit:
+//nanoTimeout has to be >= 1 millisecond to iterate again.
I thought we use old C style comments? Could you please leave a space 
between comment and text?


net_util.h: It could be more readable if you used the "1000 * 1000..." 
notation to define the constants.


--

Apart from this, I have some small reservations about JVM_NanoTime: I 
see that JVM_NanoTime is defined using the JVM_LEAF macro. I am not 
sure why this is necessary. It has a number of disadvantages:


- we need to hand in JVMEnv*, which is not necessary for the time 
measurement itself (which is a simple os::javaTimeNanos() call). This 
requires us to funnel the JVMEnv* thru to NET_Timeout, which makes the 
caller code more complicated.


- JVM_LEAF does a number of verifications in the debug VM, which is 
not ideal for a time measure function


- Also, it blocks on VM exit. Probably not a problem, but a cause for 
potential problems.


os::javaTimeNanos is a simple time measuring function which does not 
depend on any internal VM state, as far as I see... so, I do not think 
it is necessary to wrap it with JVM_LEAF, no?


Kind Regards, Thomas


On Tue, Apr 11, 2017 at 3:04 PM, Vyom Tewari <vyom.tew...@oracle.com 
<mailto:vyom.tew...@oracle.com>> wrote:


Hi,

Please review the code change for below issue.

Bug: https://bugs.openjdk.java.net/browse/JDK-8165437
<https://bugs.openjdk.java.net/browse/JDK-8165437>

Webrev:
http://cr.openjdk.java.net/~vtewari/8165437/webrev0.5/index.html
<http://cr.openjdk.java.net/%7Evtewari/8165437/webrev0.5/index.html>

This change will replace the "gettimeofday" to use the monotonic
increasing clock(i used the existing function "JVM_NanoTime"
instead of writing my own).

Thanks,

Vyom

Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Vyom Tewari]

Hi,

please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html).


Thanks,

Vyom



On Tuesday 25 April 2017 07:34 PM, Thomas Stüfe wrote:

Hi Chris, Vyom,

I have preferences as expressed earlier, but no strong emotions. I can 
live with the fix as it is now.


Thanks all, and Kind Regards, Thomas


On Tue, Apr 25, 2017 at 3:31 PM, Chris Hegarty 
<chris.hega...@oracle.com <mailto:chris.hega...@oracle.com>> wrote:


> On Tue, Apr 18, 2017 at 7:08 AM, Vyom Tewari <vyom.tew...@oracle.com
<mailto:vyom.tew...@oracle.com>> wrote:
> ...
>
> Thanks for review, please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.6/index.html
<http://cr.openjdk.java.net/%7Evtewari/8165437/webrev0.6/index.html>)

The changes mainly look good to me, just a few comments:

1) src/java.base/unix/native/libnet/PlainSocketImpl.c

   L235 jlong nanoTimeout = timeout * NET_NSEC_PER_MSEC;

   Can you please move this to the latest block of code that
requires it, i.e..
   just after L327 if (connect_rv != 0) { …

2) src/java.base/share/native/libnet/net_util.h

   Should these definitions be moved to
src/java.base/unix/native/libnet/net_util_md.h?


Regarding JVM_NanoTime being a JVM_LEAF and/or taking a JNIEnv (
both of which are, in todays hotspot VM, effectively ignored ),
this is a
separate issue. I have raised it off list with others from the VM
team,
without much interest. I will refrain from filing a JIRA issue to
track potential
changes in the VM for this. Others are welcome to do so, if they
wish. I
suggest that we simply continue to pass a valid JNIEnv, since it
is not
much extra effort to do so. ( this can be refactored later, if the
VM interface
is ever updated ).


> On 24 Apr 2017, at 12:07, Thomas Stüfe <thomas.stu...@gmail.com
<mailto:thomas.stu...@gmail.com>> wrote:
>
> ...
> That aside, I am not a big fan of the removal of the old NET_Timeout. 
Before, there were two
functions, "NET_Timeout" just taking the timeout value,
"NET_Timeout0" taking a timeout and a start value. You removed the
first variant and therefore added the many additional
JVM_NanoTime() calls at NET_Timeout callsites. This makes the code
harder to read and also kind of exposes the internal
implementation of NET_Timeout (namely the fact that NET_Timeout
uses JVM_NanoTime for time measurement). Could you please let both
variants live, optionally with better names?

I think that it may not be worth added back the simpler variant. It
would only be used by PlainDatagramSocketImpl, correct? All other
usages would use the variant that accepts the current nano time.

-Chris.

Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Vyom Tewari]

Hi,

Even i thought the same, pass nanosecond timeout to "NET_Timeout" but if 
you see the implementation it uses " *int poll(struct pollfd **/fds/*, 
nfds_t */nfds/*, int */timeout/*);*


"  where timeout is in millisecond so we have to do conversion anyway in 
loop if we pass timeout in NS. So there will not be much difference,  
will save just one conversion.


thanks,

Vyom

On Thursday 27 April 2017 04:58 PM, Bernd Eckenfels wrote:

Hello,

It looks to me like using nanoseconds in the NET_Timeout Timeout 
Parameter would remove quite a few conversions. Callsides mostly 
already have timeoutNanoseconds for calculating reminder.


Gruss
Bernd
--
http://bernd.eckenfels.net


*From:* net-dev <net-dev-boun...@openjdk.java.net 
<mailto:net-dev-boun...@openjdk.java.net>> on behalf of Thomas Stüfe 
<thomas.stu...@gmail.com <mailto:thomas.stu...@gmail.com>>

*Sent:* Monday, April 24, 2017 1:07:52 PM
*To:* Vyom Tewari
*Cc:* net-dev
*Subject:* Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in 
Networking code

Hi Vyom,

sorry for the late response, I had vacation.

Thanks for taking my suggestions! Here some remarks:

---

I looked a little bit closer into the question why JVM_LEAF is used to 
wrap simple little functions like JVM_NanoTime or 
JVM_CurrentTimeMillis (among others). There is no hard technical 
reason why a function could not just be exported from the libjvm.so 
using JNIEXPORT, in any form one wishes too. (In fact, we do this in 
our jvm port a lot).


However, JVM_NanoTime() and JVM_CurrentTimeMillis() are used as direct 
native implementations for System.currentTimeMillis() and 
System.nanoTime() (see share/native/libjava/System.c), using 
RegisterNatives(). So, the original author saved the glue functions he 
would have to write otherwise (Java_java_lang_System_currentTimeMillis 
etc).


The comments in System.c indicate that this was done for performance 
reasons (you save one call frame by calling JVM_NanoTime directly).


Because they are used as direct JNI implementations for static java 
methods in System, JVM_NanoTime() and JVM_CurrentTimeMillis() have to 
carry around JNIEnv* and jclass parameters. But they are ignored by 
the functions - the jclass argument is even called "ignored" in jvm.h. 
And it seems to be perfectly okay to call JVM_CurrentTimeMillis() with 
NULL as JNIEnv* argument, which is done in many places in the jdk. 
Presumably this would be okay too for JVM_NanoTime(), so you could at 
least avoid the new JNIEnv* argument in NET_Timeout and just call 
JVM_NanoTime with NULL as first parameter.


-

That aside, I am not a big fan of the removal of the old NET_Timeout. 
Before, there were two functions, "NET_Timeout" just taking the 
timeout value, "NET_Timeout0" taking a timeout and a start value. You 
removed the first variant and therefore added the many additional 
JVM_NanoTime() calls at NET_Timeout callsites. This makes the code 
harder to read and also kind of exposes the internal implementation of 
NET_Timeout (namely the fact that NET_Timeout uses JVM_NanoTime for 
time measurement). Could you please let both variants live, optionally 
with better names?


-

Otherwise, I think the change looks good. Thanks for your patience!

Kind Regards, Thomas





On Tue, Apr 18, 2017 at 7:08 AM, Vyom Tewari <vyom.tew...@oracle.com 
<mailto:vyom.tew...@oracle.com>> wrote:


Hi Thomas,

Thanks for review, please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.6/index.html
<http://cr.openjdk.java.net/%7Evtewari/8165437/webrev0.6/index.html>)
i incorporated all the review comments. Regarding why JVM_NanoTime
is defined JVM_LEAF i don't know much, but all the functions which
are defined in jvm.h used some sort of
macro(JVM_LEAF,JVM_ENTRY,JVM_ENTRY_NO_ENV etc). You can not call
"os::javaTimeNanos()" directly as this is not visible, i did a
small prototype which simply export this without any JVM macro as
below.

This prototype did worked but i am not sure if this is right way
to do. I need some more input, why all jvm.h functions are defined
with macro and if it OK to defined function as below, maybe some
one from JVM team can give some more comment on this. I decided
not to include this prototype as part of review because i am not
sure if this is right way.

Thanks,

Vyom

##

diff -r 26d689c621f6 make/symbols/symbols-unix
--- a/make/symbols/symbols-unixWed Apr 12 19:28:47 2017 -0700
+++ b/make/symbols/symbols-unixTue Apr 18 08:40:25 2017 +0530
@@ -51,6 +51,7 @@
 JVM_CurrentLoadedClass
 JVM_CurrentThread
 JVM_CurrentTimeMillis
+JVM_CurrentTimeNano
 JVM_DefineClass
 JVM_DefineClassWithSource

Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Vyom Tewari]

Hi David,

I will look into the issue.

Thanks,

Vyom


On Thursday 04 May 2017 06:29 AM, David Holmes wrote:

please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html). 



This change is broken on 32-bit systems - JVM_Nanotime returns a jlong 
which is always 64-bit, but the code uses long for the nanotimeout 
values, which will be 32-bit on 32-bit systems!


This change appears to be causing massive testing failures due to 
jtreg agentvm failing due to sock connection issues. Most obvious with 
32-bit linux builds. However we also see a some failures on OSX which 
is not explained by the long vs jlong issue.


David
-

RFR 8179905: Remove the use of gettimeofday in Networking code

[Vyom Tewari]

Hi ,

Please review the code change for below issue.

Webrev  : http://cr.openjdk.java.net/~vtewari/8179905/webrev0.0/index.html

BugId : https://bugs.openjdk.java.net/browse/JDK-8179905

This issue is duplicate of "JDK-8165437", where pushed code change 
failed on 32 bit platforms so we revert back code changes as part of 
"JDK-8179602".


Please find below is the webrev that was pushed as part of "JDK-8165437".

Webrev  : http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html

Thanks,

Vyom

Re: RFR (xxs): 8180424: Another build issue on AIX after 8034174

[Vyom Tewari]

Hi Thomas,

fix look good to me, but i am not jdk10 reviewer.

Thanks,

Vyom


On Tuesday 16 May 2017 06:20 PM, Thomas Stüfe wrote:

Hi all,

may I have a review for this tiny fix:

Issue: https://bugs.openjdk.java.net/browse/JDK-8180424
webrev: 
http://cr.openjdk.java.net/~stuefe/webrevs/8180424-another-build-issue-on-aix-after-8034174/webrev.00/webrev/ 



The prototypes for NET_RecvFrom and NET_Accept do not match their 
implementations for AIX since 8034174. This did not lead to an error 
in jdk9 because there, the header (net_util_md.h) was not included by 
aix_close.c. In JDK10, it is included and therefore does not build.


I believe this did not lead to a runtime error on jdk9, at least not 
for the typical values involved; the mismatch is between int* and 
unsigned int* (native socklen_t).


Kind Regards, Thomas

Re: RFR 8179905: Remove the use of gettimeofday in Networking code

[Vyom Tewari]

Hi Claes,

thanks for review, timeout is "int" so even if you set max(2147483647) 
value that int data type can hold, there will not be any overflow. If 
you try to set very large number like "0x7fff" to int data 
type you will get compile time error(integer number too large: 
7fff).


Thanks,
Vyom

On Tuesday 09 May 2017 11:20 PM, Claes Redestad wrote:

Hi,

doesn't this need to consider numerical overflows, e.g., what happens 
if someone sets a timeout value near 0x7fffL before and 
after this change?


/Claes

On 2017-05-09 11:55, Vyom Tewari wrote:

Hi ,

Please review the code change for below issue.

Webrev  : 
http://cr.openjdk.java.net/~vtewari/8179905/webrev0.0/index.html


BugId : https://bugs.openjdk.java.net/browse/JDK-8179905

This issue is duplicate of "JDK-8165437", where pushed code change 
failed on 32 bit platforms so we revert back code changes as part of 
"JDK-8179602".


Please find below is the webrev that was pushed as part of 
"JDK-8165437".


Webrev  : 
http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html


Thanks,

Vyom

Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Vyom Tewari]

Hi Sean,

with your patch as well your test case is failing on my 
laptop(Ubuntu16.04), when i tried to run  on jdk8 i am getting below error.


java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR) failed)
at java.net.NetworkInterface.getMacAddr0(Native Method)
at 
java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457)

at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)

at java.lang.Thread.run(Thread.java:745)
java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR) failed)
at java.net.NetworkInterface.getMacAddr0(Native Method)
at 
java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457)

at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)

at java.lang.Thread.run(Thread.java:745)
Exception in thread "main" java.lang.RuntimeException: Failed
at com.java.test.GetMacAddress.main(GetMacAddress.java:96)
mac id is null for interface cscotun0- Thread0
Testing: cscotun0
mac id is null for interface cscotun0- Thread3
Testing: cscotun0

Thanks,

Vyom


On Thursday 22 June 2017 09:59 PM, Seán Coffey wrote:
JDK 10 fix required to correct a race issue in NetworkInterface. I 
don't believe the ifreq struct needs to be static in any case. New 
auto unit testcase also. I propose to skip this fix for JDK 9 and fix 
in an update release for that family. I also plan to port this to 
jdk8u-dev.


https://bugs.openjdk.java.net/browse/JDK-8182672
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10/webrev/

regards,
Sean.

Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Vyom Tewari]

Hi Sean,

i  debug the failure and it look like you need to modify your test and 
make it more robust. As Christoph suggested first call 
"getHardwareAddress" and check if is not null then only iterate 100 
time. This way your test will run on any platform and one minor comment 
make your global variables(count ) volatile.


I did the changes at my local env and it did worked for me  and your 
modified test did not failed after fix.


Thanks,

Vyom


On Thursday 22 June 2017 11:46 PM, Seán Coffey wrote:

Hi Vyom,

thanks for testing. Null is a valid value for some interfaces. I've 
excluded the loopback interface from being testing. Perhaps there's 
other issues at play here.  We know that getHardwareAddress() can 
throw SocketException if I/O fails. For this particular scenario we're 
not interested in that and perhaps that can be ignored.


I'll take another look.

regards,
Sean.

On 22/06/2017 18:50, Vyom Tewari wrote:

Hi Sean,

with your patch as well your test case is failing on my 
laptop(Ubuntu16.04), when i tried to run  on jdk8 i am getting below 
error.


java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR) failed)
at java.net.NetworkInterface.getMacAddr0(Native Method)
at 
java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457)

at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)

at java.lang.Thread.run(Thread.java:745)
java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR) failed)
at java.net.NetworkInterface.getMacAddr0(Native Method)
at 
java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457)

at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)

at java.lang.Thread.run(Thread.java:745)
Exception in thread "main" java.lang.RuntimeException: Failed
at com.java.test.GetMacAddress.main(GetMacAddress.java:96)
mac id is null for interface cscotun0- Thread0
Testing: cscotun0
mac id is null for interface cscotun0- Thread3
Testing: cscotun0

Thanks,

Vyom


On Thursday 22 June 2017 09:59 PM, Seán Coffey wrote:
JDK 10 fix required to correct a race issue in NetworkInterface. I 
don't believe the ifreq struct needs to be static in any case. New 
auto unit testcase also. I propose to skip this fix for JDK 9 and 
fix in an update release for that family. I also plan to port this 
to jdk8u-dev.


https://bugs.openjdk.java.net/browse/JDK-8182672
webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10/webrev/


regards,
Sean.

Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Vyom Tewari]

Hi Sean,

latest test code looks good, one minor bit, in MT program 
"ex.printStackTrace();" will not help as output will be screwed. I will 
suggested to pass the original exception to line 106.


throw new RuntimeException(originalException);

Thanks,

Vyom


On Friday 23 June 2017 03:26 PM, Seán Coffey wrote:

Thanks to Christoph, Vyom and Mark for the reviews.

I've improved the testcase as per feedback. Hope this meets all 
requests :


http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10.v2/webrev/

Regards,
Sean.

On 22/06/17 22:36, Langer, Christoph wrote:

Hi Sean,

I played with it a bit more and now really understand Vyoms 
observation. So, what he sees is not the original concurrency issue 
but he encounters a SocketException on some interface, where this is 
supposed to occur upon calling getHardwareAddress().


So, to enable the testcase to run robustly on any platform, any 
hardware and any network configuration, I suggest to modify the test 
like this:


1. In main, enumerate all interfaces once
2. Iterate over all interfaces, exlude loopback and see if a single 
call to getHardwareAddress() won't yield null or an Exception.
3. For each interface where a valid mac address could be obtained 
once, start the executor threads to stress getHardwareAddress() in 
parallel, e.g. like 5 threads doing it 100 times in parallel. I would 
also suggest to use per thread counters instead of one global 'count' 
as we have right now.


Furthermore, the test output could be improved a bit, e.g. when it 
comes to an exception, the thread name could be mentioned, too.


Best regards
Christoph


-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
Seán Coffey
Sent: Donnerstag, 22. Juni 2017 20:17
To: Vyom Tewari <vyom.tew...@oracle.com>; net-dev 
Subject: Re: RFR : 8182672: Java 8u121 on Linux intermittently 
returns null for

MAC address

Hi Vyom,

thanks for testing. Null is a valid value for some interfaces. I've
excluded the loopback interface from being testing. Perhaps there's
other issues at play here.  We know that getHardwareAddress() can throw
SocketException if I/O fails. For this particular scenario we're not
interested in that and perhaps that can be ignored.

I'll take another look.

regards,
Sean.

On 22/06/2017 18:50, Vyom Tewari wrote:

Hi Sean,

with your patch as well your test case is failing on my
laptop(Ubuntu16.04), when i tried to run  on jdk8 i am getting below
error.

java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR) failed)
 at java.net.NetworkInterface.getMacAddr0(Native Method)
 at


java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457
)

 at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
 at

java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.jav 


a:1142)

 at

java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.ja 


va:617)

 at java.lang.Thread.run(Thread.java:745)
java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR) failed)
 at java.net.NetworkInterface.getMacAddr0(Native Method)
 at


java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457
)

 at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
 at

java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.jav 


a:1142)

 at

java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.ja 


va:617)

 at java.lang.Thread.run(Thread.java:745)
Exception in thread "main" java.lang.RuntimeException: Failed
 at com.java.test.GetMacAddress.main(GetMacAddress.java:96)
mac id is null for interface cscotun0- Thread0
Testing: cscotun0
mac id is null for interface cscotun0- Thread3
Testing: cscotun0

Thanks,

Vyom


On Thursday 22 June 2017 09:59 PM, Seán Coffey wrote:

JDK 10 fix required to correct a race issue in NetworkInterface. I
don't believe the ifreq struct needs to be static in any case. New
auto unit testcase also. I propose to skip this fix for JDK 9 and fix
in an update release for that family. I also plan to port this to
jdk8u-dev.

https://bugs.openjdk.java.net/browse/JDK-8182672
webrev :
http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10/webrev/

regards,
Sean.

Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Vyom Tewari]

Hi Chris,

test looks good, one minor comment can be ignored, in failing case it 
print stack and "Exception in thread "main" java.lang.RuntimeException: 
Failed" both. I can see main is declared to throw Exception. Is is 
possible to throw exception instead of  printing trace or pass this 
exception to last line ?


Thanks,

Vyom


On Friday 23 June 2017 06:58 PM, Chris Hegarty wrote:



On 23/06/17 10:56, Seán Coffey wrote:

Thanks to Christoph, Vyom and Mark for the reviews.

I've improved the testcase as per feedback. Hope this meets all 
requests :


http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10.v2/webrev/


The change looks good.

Sean and I did a live coding session and arrived at the following
version of the test.

-Chris.

/*
 * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.
 *
 * This code is distributed in the hope that it will be useful, but 
WITHOUT

 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file 
that

 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License 
version

 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 
USA

 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */

/*
 * @test
 * @bug 8182672
 * @summary Java 8u121 on Linux intermittently returns null for MAC 
address

 */

import java.net.NetworkInterface;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.Phaser;
import java.util.function.Predicate;
import java.util.stream.Collectors;

public class GetMacAddress implements Callable {
static final int NUM_THREADS = 5;
static final int NUM_ITERS = 100;
static volatile boolean failed; // false

final String threadName;
final NetworkInterface ni;
final Phaser startingGate;

public GetMacAddress(NetworkInterface ni, String name, Phaser 
phaser) {

this.ni = ni;
this.threadName = name;
this.startingGate = phaser;
}

@Override
public Exception call() {
int count = 0;
startingGate.arriveAndAwaitAdvance();
try {
for (int i = 0; i < NUM_ITERS; i++) {
ni.getMTU();
byte[] addr = ni.getHardwareAddress();
if (addr == null) {
System.out.println(threadName + ". mac id is null");
failed = true;
}
count = count + 1;
if (count % 100 == 0) {
System.out.println(threadName + ". count is " + 
count);

}
}
} catch (Exception ex) {
System.out.println(threadName + ". Not expecting 
exception:" + ex.getMessage());

failed = true;
return ex;
}
return null;
}

static final Predicate hasHardwareAddress = ni -> {
try {
if (ni.getHardwareAddress() == null) {
System.out.println("Not testing null addr: " + 
ni.getName());

return false;
}
} catch (Exception ex) {
System.out.println("Not testing: " + ni.getName() +
" " + ex.getMessage());
}
return true;
};

public static void main(String[] args) throws Exception {
List toTest = 
NetworkInterface.networkInterfaces()

.filter(hasHardwareAddress)
.collect(Collectors.toList());

ExecutorService executor = 
Executors.newFixedThreadPool(NUM_THREADS);


for (NetworkInterface ni : toTest) {
Phaser startingGate = new Phaser(NUM_THREADS);
System.out.println("Testing: " + ni.getName());
List list = new ArrayList<>();
for (int i = 0; i < NUM_THREADS; i++)
list.add(new GetMacAddress(ni, ni.getName() + 
"-Thread-" + i, startingGate));

List futures = executor.invokeAll(list);
for (Future f : futures) {
if (f.get() != null)
f.get().printStackTrace(System.out);
}
if (failed)
break;
}
executor.shutdownNow();
if (!failed) {
System.out.println("PASSED 

RFR 8179602: Fix for JDK-8165437 is broken on 32-bit Linux

[Vyom Tewari]

Hi All,

Please  review the below change.

Webrev: http://cr.openjdk.java.net/~vtewari/8179602/webrev0.0/index.html

Bugid: https://bugs.openjdk.java.net/browse/JDK-8179602

This issue is because of side effect of "JDK-8165437" where we are using 
"JVM_NanoTime" which returns a 64 bit jlong and return value was getting 
assigned to long type. On 32 bit OS long is 4 byte, which leads to 
integer overflow.


Our internal test JPRT is  still running.

Thanks,

Vyom

Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Vyom Tewari]

pushed the 
code(http://hg.openjdk.java.net/jdk10/jdk10/jdk/rev/7cdde79d6a46).


Vyom


On Friday 28 April 2017 03:26 AM, Langer, Christoph wrote:


Hi Vyom,

I’ve just got a small formatting remark about the order of includes:

Generally I tried to follow the rule 1. Common os headers, 2. Platform 
os headers,  3. Jvm/jdk headers, 4. JNI headers in my latest 
refactorings. So, to keep this up, can you move #include “jvm.h” in 
the line before #include “net_util.h” in each file? And pull it before 
the JNI headers. Like, e.g. in net_util_md.c you should move #include 
"jvm.h" to line 52.


Thanks & Best regards

Christoph

*From:*net-dev [mailto:net-dev-boun...@openjdk.java.net] *On Behalf Of 
*Vyom Tewari

*Sent:* Donnerstag, 27. April 2017 06:16
*To:* Thomas Stüfe <thomas.stu...@gmail.com>; Chris Hegarty 
<chris.hega...@oracle.com>

*Cc:* net-dev <net-dev@openjdk.java.net>
*Subject:* Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in 
Networking code


Hi,

please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8165437/webrev0.7/index.html>).


Thanks,

Vyom

On Tuesday 25 April 2017 07:34 PM, Thomas Stüfe wrote:

Hi Chris, Vyom,

I have preferences as expressed earlier, but no strong emotions. I
can live with the fix as it is now.

Thanks all, and Kind Regards, Thomas

On Tue, Apr 25, 2017 at 3:31 PM, Chris Hegarty
<chris.hega...@oracle.com <mailto:chris.hega...@oracle.com>> wrote:

> On Tue, Apr 18, 2017 at 7:08 AM, Vyom Tewari
<vyom.tew...@oracle.com <mailto:vyom.tew...@oracle.com>> wrote:
> ...
>
> Thanks for review, please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.6/index.html
<http://cr.openjdk.java.net/%7Evtewari/8165437/webrev0.6/index.html>)

The changes mainly look good to me, just a few comments:

1) src/java.base/unix/native/libnet/PlainSocketImpl.c

   L235 jlong nanoTimeout = timeout * NET_NSEC_PER_MSEC;

   Can you please move this to the latest block of code that
requires it, i.e..
   just after L327 if (connect_rv != 0) { …

2) src/java.base/share/native/libnet/net_util.h

   Should these definitions be moved to
src/java.base/unix/native/libnet/net_util_md.h?


Regarding JVM_NanoTime being a JVM_LEAF and/or taking a JNIEnv (
both of which are, in todays hotspot VM, effectively ignored
), this is a
separate issue. I have raised it off list with others from the
VM team,
without much interest. I will refrain from filing a JIRA issue
to track potential
changes in the VM for this. Others are welcome to do so, if
they wish. I
suggest that we simply continue to pass a valid JNIEnv, since
it is not
much extra effort to do so. ( this can be refactored later, if
the VM interface
is ever updated ).


> On 24 Apr 2017, at 12:07, Thomas Stüfe
<thomas.stu...@gmail.com <mailto:thomas.stu...@gmail.com>> wrote:
>
> ...
> That aside, I am not a big fan of the removal of the old
NET_Timeout. Before, there were two functions, "NET_Timeout"
just taking the timeout value, "NET_Timeout0" taking a timeout
and a start value. You removed the first variant and therefore
added the many additional JVM_NanoTime() calls at NET_Timeout
callsites. This makes the code harder to read and also kind of
exposes the internal implementation of NET_Timeout (namely the
fact that NET_Timeout uses JVM_NanoTime for time measurement).
Could you please let both variants live, optionally with
better names?

I think that it may not be worth added back the simpler
variant. It
would only be used by PlainDatagramSocketImpl, correct? All other
usages would use the variant that accepts the current nano time.

-Chris.

Re: RFR 8179602: Fix for JDK-8165437 is broken on 32-bit Linux

[Vyom Tewari]

Hi All,

please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8179602/webrev0.1/index.html) 
which fixed the Mac OS issued as well.


Michael as we we discussed off line, i incorporated the changes 
suggested by you,


Thanks,

Vyom


On Thursday 04 May 2017 03:11 PM, Michael McMahon wrote:

Hi Vyom,

I notice this doesn't seem to fix the Mac OS problem.
We may have to file a separate issue for that.

- Michael

On 04/05/2017, 09:50, Vyom Tewari wrote:

Hi All,

Please  review the below change.

Webrev: http://cr.openjdk.java.net/~vtewari/8179602/webrev0.0/index.html

Bugid: https://bugs.openjdk.java.net/browse/JDK-8179602

This issue is because of side effect of "JDK-8165437" where we are 
using "JVM_NanoTime" which returns a 64 bit jlong and return value 
was getting assigned to long type. On 32 bit OS long is 4 byte, which 
leads to integer overflow.


Our internal test JPRT is  still running.

Thanks,

Vyom

RFR[10]: 8185072 network006 times out in many configs in JDK10-hs nightly

[vyom tewari]

Hi,

Please review, small code change below.

webrev: http://cr.openjdk.java.net/~vtewari/8185072/webrev0.0/index.html

BugId:  https://bugs.openjdk.java.net/browse/JDK-8185072

this is regression because of "JDK-8179905".

Thanks,

Vyom

Re: RFR[10]: 8185072 network006 times out in many configs in JDK10-hs nightly

[vyom tewari]

ping!!

Vyom


On Friday 15 September 2017 02:59 PM, vyom tewari wrote:

Hi,

Please review, small code change below.

webrev: http://cr.openjdk.java.net/~vtewari/8185072/webrev0.0/index.html

BugId:  https://bugs.openjdk.java.net/browse/JDK-8185072

this is regression because of "JDK-8179905".

Thanks,

Vyom

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

ping!!

Vyom


On Monday 11 September 2017 09:08 PM, vyom tewari wrote:


Hi All,

As jdk.net API already moved out of java.base, Please review the below 
code change for jdk10.


Bug: https://bugs.openjdk.java.net/browse/JDK-8145635

Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html

Thanks,

Vyom


On Wednesday 24 February 2016 03:16 PM, Alan Bateman wrote:


On 24/02/2016 09:16, vyom wrote:

Hi All,

Please review my code changes for the below issue.

Bug: JDK-8145635 : Add TCP_QUICKACK socket option

Webrev: 
http://cr.openjdk.java.net/~vtewari/8145635/webrev0.0/index.html


Currently TCP_QUICKACK is only supported on Linux( since Linux 
2.4.4) so i implemented is as "ExtendedSocketOption".


Is there any urgency on this one? I'm just wondering if we should try 
to the jdk.net API moved out of java.base before we add more socket 
options. This is currently tracked as JDK-8044773 and important to 
get into JDK 9.


-Alan

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi Chris,

Thanks for review. Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.html).


Thanks,

Vyom


On Saturday 14 October 2017 02:25 AM, Chris Hegarty wrote:

Vyom,


On 12 Oct 2017, at 10:01, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Roger,

Thanks for the review, i incorporated all review comments from you except("you 
can use ExtendedSocketOptions.TCP_QUICKACK to check for the option to omit without
  embedding the name."). ExtendedSocketOptions.java is part of "jdk.net"  so we can not 
directly use it in java.base, hence i used the option name to filter "TCP_QUICKACK".

Please find the update 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.4/index.html).

This looks much better.

Some suggested wordings of the JDK-specific option:

/**
  * Disable Delayed Acknowledgements.
  *
  *  This socket option can be used to reduce or disable delayed
  * acknowledgments (ACKs).
  *
  *  The value of this socket option is a {@code Boolean} that represents
  * whether the option is enabled or disabled. The socket option is specific
  * to stream-oriented sockets using the TCP/IP protocol.
  *
  *  The exact semantics of this socket option are socket type and system
  * dependent.
  *
  *  When TCP_QUICKACK is enabled, ACKs are sent immediately, rather than
  * delayed if needed in accordance to normal TCP operation. This option is
  * not permanent, it only enables a switch to or from TCP_QUICKACK mode.
  *
  *  Subsequent operations of the TCP protocol will once again enter/leave
  * TCP_QUICKACK mode depending on internal protocol processing and factors
  * such as delayed ack timeouts occurring and data transfer.
  *
  * @since 18.3
  */

-Chris.

P.S. D’oh, sorry, of course you need the paragraph tags.



Thanks,

Vyom


On Wednesday 11 October 2017 08:46 PM, Roger Riggs wrote:

Hi Vyom,

Comments:

  - update copyright
  - use @since 18.3 instead of @since 10

- ExtendedSocketOptions.java: 265,269  include the "TCP_QUICKACK" in the 
exception messages.

 Line 144: if you are going to keep the assert, add a explanation about how 
it could happen.
 I'd remove the assert.

  - The first sentence should be a complete sentence: "TCP_QUICKACK socket option 
enables sending TCP/IP acks immediately" or similar.

  - A reference to the appropriate TCP/IP spec for quickack would be a good 
addition. At least the RFC # and section.
  - 85: space after "."  The phrasing is a bit odd in places in the javadoc.
  - line 81/82 the value is true to enable and false to disable.
  - This phrase is confusing: "it only enables a switch to or from TCP_QUICKACK 
mode";
Since it is set on a socket, it should remain set on that socket until it 
is changed.

  - 203: be consistent in using enable/disable in parameters, etc even for 
private methods.
 "on" -> "enable"

PlainDatagramSocketImpl: 89:
   Why create a new set with zero or one items just to throw it away?
   Use the iterator to add only the non-TCP_QUICKACK options to the supported 
options.
  Also, you can use ExtendedSocketOptions.TCP_QUICKACK to check for the option 
to omit without
  embedding the name.


Sockets.java:
   - The initialization of isQuickAckAvailable might be cleaner as an nested 
static class
 that initializes the value in its static initializer. That would delay the 
init til needed
 and avoid extra flags.

LinuxSocketOptions.java:
- the native methods should be static; since the instance is unused.

  - line 41: the return type should be Boolean

  - line 46: the return type of getQuickAck0 should be Boolean like the 
argument to set.

  - line 34:  using JNU_ThrowByNameWithLastError directly is sufficient; if the errno 
does not have a string unix supplies "unknown error nnn".


Regards, Roger

On 10/10/2017 2:58 PM, Chris Hegarty wrote:

Vyom,

What about suggestion 1) below, the name of the socket option?

-Chris.


On 27 Sep 2017, at 09:56, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Chris,

Thanks for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). I 
incorporated review comments from you and re-base the patch to our consolidated 
repo(jdk10/master).

Thanks,

Vyom


On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:

Vyom,


On 11 Sep 2017, at 16:38, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi All,

As jdk.net API already moved out of java.base, Please review the below code 
change for jdk10.

Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html


This looks quite good. Some comments:

1) I wonder if we should just call the option TCP_QUICKACK, rather than 
SO_QUICKACK? Similar to TCP_NODELAY.

2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1) above.

3) Java

Re: RFR(S): 8155590: Dubious collection management in sun.net.www.http.KeepAliveCache

[vyom tewari]

Hi Christoph,

Thanks for doing this, i think you don't need to synchronize the 
"remove(HttpClient h)".  This remove is get called from synchronize 
"remove (HttpClient h, Object obj)" and the underline data structure is 
which is java.util.Vector(ClientVector extends java.util.Stack) is also 
thread safe.


What do you think ?

Thanks,

Vyom


On Monday 16 October 2017 12:52 PM, Langer, Christoph wrote:


Hi,

Here is a proposal for a fix for bug 8155590. I already made this fix 
a while ago in our JDK clone and I’d like to contribute this.


Bug: https://bugs.openjdk.java.net/browse/JDK-8155590 



Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8155590.0/ 



Please review.

Thanks

Christoph

Re: RFR(XS): 8188855: Fix VS10 build after "8187658: Bigger buffer for GetAdaptersAddresses"

[vyom tewari]

Hi Goetz,

Change looks OK to me, although i am not the official reviewer.

Thanks,

Vyom


On Friday 06 October 2017 12:18 PM, Lindenmaier, Goetz wrote:

Hi,

could I please get reviews for this tiny change?
http://cr.openjdk.java.net/~goetz/wr17/8188855-winBuild/webrev/

Best regards,
   Goetz.

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi Roger,

Thanks for the review , please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.6/index.html).


Thanks,

Vyom


On Tuesday 17 October 2017 06:35 AM, Roger Riggs wrote:

Hi Vyom,

A few suggestions:

PlainDatagramSocketImpl.java:
 - line 95/96:  I think you can use just forEach, the order version is 
not necessary.
    The code will be a bit more readable if the .filter and .forEach 
are on a new line and don't wrap.

    You can also remove the extra "(" and ")

 - line 87-94: these are confusing and imply some implicit resetting 
of the option.

 - use @since 10
- 209/268: the native setQuickAck method should use boolean as its 
argument to enable/disable

  Since enable is a boolean; it does not need "== true'

LinuxSocketOptions.java/c:
  - 52: setQuickAck0 should use boolean for the 2nd argument; (The 
native code already does)


Thanks, Roger


On 10/15/17 11:58 PM, vyom tewari wrote:

Hi Chris,

Thanks for review. Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.html).


Thanks,

Vyom

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi Chris,


On Wednesday 11 October 2017 12:28 AM, Chris Hegarty wrote:

Vyom,

What about suggestion 1) below, the name of the socket option?
to be consistent with SO_FLOW_SLA in ExtendedSocketOptions.java, i 
choose the "SO" prefix. But I don't know the history behind the "SO" 
prefix  so i changed the socket option name to "TCP_QUICKACK" as suggested.


Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.3/index.html).


Thanks,
Vyom



-Chris.


On 27 Sep 2017, at 09:56, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Chris,

Thanks for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). I 
incorporated review comments from you and re-base the patch to our consolidated 
repo(jdk10/master).

Thanks,

Vyom


On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:

Vyom,


On 11 Sep 2017, at 16:38, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi All,

As jdk.net API already moved out of java.base, Please review the below code 
change for jdk10.

Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html


This looks quite good. Some comments:

1) I wonder if we should just call the option TCP_QUICKACK, rather than 
SO_QUICKACK? Similar to TCP_NODELAY.

2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1) above.

3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, rather than 
jobject, thus avoiding the need for createBoolean. The conversation can happen 
in the Java layer.  Can you please reduce the lint length in this file, by 
wrapping similar to the style of the Solaris version.

4) ExtendedSocketOptions.java
   - drop the , they are unnecessary.
   - I think, similar to TCP_NODELAY, the spec should say that the options is TCP 
specific. From TCP_NODELAY: "The socket option is specific to stream-oriented 
sockets using the TCP/IP protocol.”
   - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”

-Chris.

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi Alan,

thanks for pointing out, i am forwarding it to net-dev list.

Vyom


On Thursday 12 October 2017 03:54 PM, Alan Bateman wrote:
Best to reply on net-dev as that is where the main review should be 
going on (seems there are at two review threads going, maybe they 
could unite on net-dev).



On 12/10/2017 10:01, vyom tewari wrote:

Hi Roger,

Thanks for the review, i incorporated all review comments from you 
except("you can use ExtendedSocketOptions.TCP_QUICKACK to check for 
the option to omit without
 embedding the name."). ExtendedSocketOptions.java is part of 
"jdk.net"  so we can not directly use it in java.base, hence i used 
the option name to filter "TCP_QUICKACK".


Please find the update 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.4/index.html).


Thanks,

Vyom


On Wednesday 11 October 2017 08:46 PM, Roger Riggs wrote:

Hi Vyom,

Comments:

 - update copyright
 - use @since 18.3 instead of @since 10

- ExtendedSocketOptions.java: 265,269  include the "TCP_QUICKACK" in 
the exception messages.


    Line 144: if you are going to keep the assert, add a explanation 
about how it could happen.

    I'd remove the assert.

 - The first sentence should be a complete sentence: "TCP_QUICKACK 
socket option enables sending TCP/IP acks immediately" or similar.


 - A reference to the appropriate TCP/IP spec for quickack would be 
a good addition. At least the RFC # and section.
 - 85: space after "."  The phrasing is a bit odd in places in the 
javadoc.

 - line 81/82 the value is true to enable and false to disable.
 - This phrase is confusing: "it only enables a switch to or from 
TCP_QUICKACK mode";
   Since it is set on a socket, it should remain set on that socket 
until it is changed.


 - 203: be consistent in using enable/disable in parameters, etc 
even for private methods.

    "on" -> "enable"

PlainDatagramSocketImpl: 89:
  Why create a new set with zero or one items just to throw it away?
  Use the iterator to add only the non-TCP_QUICKACK options to the 
supported options.
 Also, you can use ExtendedSocketOptions.TCP_QUICKACK to check for 
the option to omit without

 embedding the name.


Sockets.java:
  - The initialization of isQuickAckAvailable might be cleaner as an 
nested static class
    that initializes the value in its static initializer. That would 
delay the init til needed

    and avoid extra flags.

LinuxSocketOptions.java:
   - the native methods should be static; since the instance is unused.

 - line 41: the return type should be Boolean

 - line 46: the return type of getQuickAck0 should be Boolean like 
the argument to set.


 - line 34:  using JNU_ThrowByNameWithLastError directly is 
sufficient; if the errno does not have a string unix supplies 
"unknown error nnn".



Regards, Roger

On 10/10/2017 2:58 PM, Chris Hegarty wrote:

Vyom,

What about suggestion 1) below, the name of the socket option?

-Chris.


On 27 Sep 2017, at 09:56, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Chris,

Thanks for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). 
I incorporated review comments from you and re-base the patch to 
our consolidated repo(jdk10/master).


Thanks,

Vyom


On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:

Vyom,

On 11 Sep 2017, at 16:38, vyom tewari <vyom.tew...@oracle.com> 
wrote:


Hi All,

As jdk.net API already moved out of java.base, Please review the 
below code change for jdk10.


Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
Webrev: 
http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html



This looks quite good. Some comments:

1) I wonder if we should just call the option TCP_QUICKACK, 
rather than SO_QUICKACK? Similar to TCP_NODELAY.


2) I think LinuxSocketOptions.h is largely unnecessary, if we do 
1) above.


3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, 
rather than jobject, thus avoiding the need for createBoolean. 
The conversation can happen in the Java layer.  Can you please 
reduce the lint length in this file, by wrapping similar to the 
style of the Solaris version.


4) ExtendedSocketOptions.java
   - drop the , they are unnecessary.
   - I think, similar to TCP_NODELAY, the spec should say that 
the options is TCP specific. From TCP_NODELAY: "The socket option 
is specific to stream-oriented sockets using the TCP/IP protocol.”

   - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”

-Chris.

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi All,

As jdk.net API already moved out of java.base, Please review the below 
code change for jdk10.


Bug: https://bugs.openjdk.java.net/browse/JDK-8145635

Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html

Thanks,

Vyom


On Wednesday 24 February 2016 03:16 PM, Alan Bateman wrote:


On 24/02/2016 09:16, vyom wrote:

Hi All,

Please review my code changes for the below issue.

Bug: JDK-8145635 : Add TCP_QUICKACK socket option

Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.0/index.html

Currently TCP_QUICKACK is only supported on Linux( since Linux 2.4.4) 
so i implemented is as "ExtendedSocketOption".


Is there any urgency on this one? I'm just wondering if we should try 
to the jdk.net API moved out of java.base before we add more socket 
options. This is currently tracked as JDK-8044773 and important to get 
into JDK 9.


-Alan

Re: RFR[10]: JDK-8023649 - java.net.NetworkInterface.getInterfaceAddresses() call flow clean up

[vyom tewari]

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

Re: RFR[10]: 8187658: Bigger buffer for GetAdaptersAddresses

[vyom tewari]

On Tuesday 26 September 2017 02:28 AM, Ivan Gerasimov wrote:

Thank you Roger for review!


On 9/25/17 11:47 AM, Roger Riggs wrote:

Hi Ivan,

Looks ok to me.

I don't see a reason to skimp on the additional size since it is 
allocated and then freed immediately.

The increment might as well be as big as the initial size.

Right.  Let's use the same value of 15K, which reduces the number of 
variables to operate with.


I don't see a reason to retry if the buffer gets close to ULONG_MAX; 
I'd just break the for loop
and let it fail. (but the new code is just doing what the old code 
did and retry even if the buffer did not increase).


If len is close to ULONG_MAX, it is still larger value of len returned 
by the previous call to GetAdaptersAddresses (otherwise we wouldn't 
have gotten ERROR_BUFFER_OVERFLOW.)
So, if we're in the loop, there's no way the buffer will not increase 
(unless there's a failure due to OOM, of course.)


Also, please introduce a constant for the number of retries; it will 
make it clearer

and not need to hard code it in two places.

Done!

Would you please take a look at the updated webrev:

http://cr.openjdk.java.net/~igerasim/8187658/01/webrev/

looks good to me , although i am not the reviewer.
Vyom


With kind regards,
Ivan

(I would probably have coded the retry count counting down to zero 
but its the same effect).


Regards, Roger


On 9/25/2017 1:03 PM, Ivan Gerasimov wrote:

Ping.

Please review the proposed change at your convenience.

The fix will greatly reduce the possibility of a need to reallocate 
the buffer to retrieve the results (something that the documentation 
strongly suggests to avoid), and, if such reallocation still occurs 
to be necessary, will increase number of retries.


With kind regards,
Ivan


On 9/19/17 10:50 AM, Ivan Gerasimov wrote:

Hello!

When retrieving information about network interfaces on Windows we 
make up to 2 attempts to call GetAdaptersAddresses().


It was reported that in very rare cases it may not be sufficient, 
and even the second attempt can fail with ERROR_BUFFER_OVERFLOW.


I suggest that we follow the recommendation given in MSDN [1]: 
increase the initial buffer size to 15K and do up to 3 attempts to 
call GetAdaptersAddresses().


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8187658
WEBREV: http://cr.openjdk.java.net/~igerasim/8187658/00/webrev/

Would you please help review the fix?

[1] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx

Re: RFR[10]: 8185072 network006 times out in many configs in JDK10-hs nightly

[vyom tewari]

On Monday 25 September 2017 08:08 PM, Roger Riggs wrote:

Hi Vyom,

Looks fine to me too.

My only concern was adding an intensive test (used to be a stress 
test) to the normal
test cycles.  I'd be more comfortable with it being in a 
separate/lower tier or as @manual.
thanks for review, i will make it "manual" before pushing as we already 
have similar test at lower tier.

Vyom


Roger



On 9/24/2017 1:15 PM, Chris Hegarty wrote:

On 15 Sep 2017, at 10:29, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi,

Please review, small code change below.

webrev: 
http://cr.openjdk.java.net/~vtewari/8185072/webrev0.0/index.html
The changes look good to me Vyom. ( I only took a cursory look at the 
test changes )


-Chris.

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi Chris,

Thanks for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). 
I incorporated review comments from you and re-base the patch to our 
consolidated repo(jdk10/master).


Thanks,

Vyom


On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:

Vyom,


On 11 Sep 2017, at 16:38, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi All,

As jdk.net API already moved out of java.base, Please review the below code 
change for jdk10.

Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html


This looks quite good. Some comments:

1) I wonder if we should just call the option TCP_QUICKACK, rather than 
SO_QUICKACK? Similar to TCP_NODELAY.

2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1) above.

3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, rather than 
jobject, thus avoiding the need for createBoolean. The conversation can happen 
in the Java layer.  Can you please reduce the lint length in this file, by 
wrapping similar to the style of the Solaris version.

4) ExtendedSocketOptions.java
   - drop the , they are unnecessary.
   - I think, similar to TCP_NODELAY, the spec should say that the options is TCP 
specific. From TCP_NODELAY: "The socket option is specific to stream-oriented 
sockets using the TCP/IP protocol.”
   - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”

-Chris.

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

[vyom tewari]

Hi Doychin,

Thanks for review, i will fix it before pushing.

Thanks,

Vyom


On Thursday 23 November 2017 03:01 PM, Doychin Bondzhev wrote:

Hi Vyom,

There is a typo in the comment :

whis is not applicable

Should be "which"


On 23.11.2017 г. 11:20, vyom tewari wrote:

Hi Chris,

Thanks for the review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.1/index.html) 
where i modified the code to take care of SO_FLOW for Solaris.


I updated the test code as well.

Thanks,

Vyom


On Monday 20 November 2017 08:50 PM, Chris Hegarty wrote:

Vyom,

On 16/11/17 09:03, vyom tewari wrote:

Hi All,

Please review the small code change for the below issue.

Webrev : 
http://cr.openjdk.java.net/~vtewari/8190843/webrev0.0/index.html


BugId    : https://bugs.openjdk.java.net/browse/JDK-8190843

In this code change, i removed the check(getSocket() == null) which 
will always be true for ServerSocket as in case of server socket we 
set "serverSocket" not "socket".


I think that this change is good. What will happen if SO_FLOW is set
on a ServerSocket?

-Chris.

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

[vyom tewari]

Hi Chris,

Thanks for the review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.1/index.html) 
where i modified the code to take care of SO_FLOW for Solaris.


I updated the test code as well.

Thanks,

Vyom


On Monday 20 November 2017 08:50 PM, Chris Hegarty wrote:

Vyom,

On 16/11/17 09:03, vyom tewari wrote:

Hi All,

Please review the small code change for the below issue.

Webrev : 
http://cr.openjdk.java.net/~vtewari/8190843/webrev0.0/index.html


BugId    : https://bugs.openjdk.java.net/browse/JDK-8190843

In this code change, i removed the check(getSocket() == null) which 
will always be true for ServerSocket as in case of server socket we 
set "serverSocket" not "socket".


I think that this change is good. What will happen if SO_FLOW is set
on a ServerSocket?

-Chris.

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

[vyom tewari]

Hi Chris,

Thanks for review, while my testing i discovered issue in the way we 
handle extended  socket options and standard socket options. I fixed it 
and updated one test as well.


I removed one redundant "if check" which i think not required. JPRT is 
clean with the changed code.


Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.2/index.html).


Thanks,

Vyom


On Thursday 23 November 2017 06:04 PM, Chris Hegarty wrote:

On 23 Nov 2017, at 09:20, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Chris,

Thanks for the review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.1/index.html) where 
i modified the code to take care of SO_FLOW for Solaris.

I think this is fine Vyom, thanks.

-Chris.

RFR:8190843 can not set/get extendedOptions to ServerSocket

[vyom tewari]

Hi All,

Please review the small code change for the below issue.

Webrev : 
http://cr.openjdk.java.net/~vtewari/8190843/webrev0.0/index.html


BugId    : https://bugs.openjdk.java.net/browse/JDK-8190843

In this code change, i removed the check(getSocket() == null) which will 
always be true for ServerSocket as in case of server socket we set 
"serverSocket" not "socket".


Thanks,

Vyom

RFR: 8189366: SocketInputStream.available() should check for eof

[vyom tewari]

Hi All,

Please review the simple change below.

Webrev   : http://cr.openjdk.java.net/~vtewari/8189366/webrev0.0/index.html

BugId  : https://bugs.openjdk.java.net/browse/JDK-8189366


Currently SocketInputStream.available() does not check for "eof" and 
simply delegate to the impl even when "eof" reached. I put a check  to 
return 0 if "eof" is already reached.


Thanks,

Vyom

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi All,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.5/index.html). 
Only change with the previous wrev(04) is i removed "socket type" as 
Alan suggested and used the default  constructor (Set<SocketOption> 
options = new HashSet<>();) in ExtendedSocketOptions.java


Thanks,

Vyom


On Sunday 13 May 2018 12:37 PM, Alan Bateman wrote:

On 12/05/2018 10:21, vyom tewari wrote:

:

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html).

I've skimmed through this webrev.

The spec for the new options mostly look good but all three include 
"The exact semantics of this socket option are socket type and system 
dependent". I assume "socket type" should be dropped from this as 
these are TCP options. Maybe you can borrow text from the TCP_NODELAY 
option where it has the statement "The socket option is specific to 
stream-oriented sockets using the TCP/IP protocol".


The changes to the NetworkChannel implementations look okay. The 
changes to the NIO tests looks okay too but would be good if you could 
keep the formatting consistent with the existing code if you can.


Ivan had good comments so I didn't spent as much time on that code and 
it seems very reviewed.


-Alan.

Re: RFR(XS): 8202181: Correctly specify size of hostname buffer in Unix Inet*AddressImpl_getLocalHostName implementations

[vyom tewari]

On Friday 27 April 2018 01:05 PM, Langer, Christoph wrote:

Hi all,

thanks for looking into this. Here are a few comments

First of all, there are no real life issues I have seen with this. It is just 
something that occurred to me when working with the code. But, why not fix it, 
even it is a corner case that might never happen.

@Thomas: As for the zero termination of the hostname result after the call to 
gethostname: Yes, we should unconditionally terminate the result, which we do. 
Unfortunately this part of code cannot be moved outside the solaris #ifdef 
because the part in the #ifdef contains variable declarations. And you know - 
the C compatibility issue...

I looked again into the macro definitions for for HOST_NAME_MAX and NI_MAXHOST. 
HOST_NAME_MAX is mentioned in the gethostname docs ([1] and [2]). Glibc docs 
indicate it is 64 Byte or 255 Byte. So it looks like it is a quite small 
buffer, compared to NI_MAXHOST from netdb.h, which is the value that 
getnameinfo likes to work with, as per [3]. Posix genameinfo doc ([4]) does not 
mention NI_MAXHOST but Linux doc says it is 1025 which is what we'd define if 
it is not set.

Taking this input I have updated my webrev to round things up a little bit: 
http://cr.openjdk.java.net/~clanger/webrevs/8202181.1/

it looks good to me, although i am not official reviewer.

I moved the definition of NI_MAXHOST into net_util_md.h and updated the comment 
a little bit to make clearer why it is there. In Inet4AddressImpl.c and 
Inet6AddressImpl.c I also fixed the other places where getnameinfo is called to 
use sizeof(buffer) instead of NI_MAXHOST.

good cleanup, comment will definitely help.

Thanks,
Vyom


Best regards
Christoph

[1] http://man7.org/linux/man-pages/man2/gethostname.2.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/gethostname.html
[3] http://man7.org/linux/man-pages/man3/getnameinfo.3.html
[4] http://pubs.opengroup.org/onlinepubs/9699919799/functions/getnameinfo.html



-Original Message-
From: vyom tewari [mailto:vyom.tew...@oracle.com]
Sent: Freitag, 27. April 2018 08:38
To: Thomas Stüfe <thomas.stu...@gmail.com>
Cc: Langer, Christoph <christoph.lan...@sap.com>; net-
d...@openjdk.java.net
Subject: Re: RFR(XS): 8202181: Correctly specify size of hostname buffer in
Unix Inet*AddressImpl_getLocalHostName implementations



On Friday 27 April 2018 10:58 AM, Thomas Stüfe wrote:

On Fri, Apr 27, 2018 at 5:57 AM, vyom tewari <vyom.tew...@oracle.com>

wrote:

Hi Christoph,


On Tuesday 24 April 2018 04:45 PM, Langer, Christoph wrote:

Hi Vyom,

I think, it is intentional to handle case where return "hostname" is to
large to
fit in  array.  if you see the man page(http://man7.org/linux/man-
pages/man2/gethostname.2.html) it says that it is unspecified whether
returned buffer includes a terminating null byte.

current code will put null in case of large "hostname", What do you think ?

yes, I had read the man page and saw this point of the spec. But exactly

for

this purpose there's this code:

// make sure string is null-terminated
hostname[NI_MAXHOST] = '\0';

If we only hand 'NI_MAXHOST' as size value into gethostname, then the
function might only write NI_MAXHOST - 1 characters of the hostname

into the

buffer.

doc says it will copy len bytes into buffer and will not copy null character
into the buffer.



C library/kernel differences
 The GNU C library does not employ the gethostname() system call;
 instead, it implements gethostname() as a library function that calls
 uname(2) and copies up to len bytes from the returned nodename

field

 into name.  Having performed the copy, the function then checks if
 the length of the nodename was greater than or equal to len, and if
 it is, then the function returns -1 with errno set to ENAMETOOLONG;
 in this case, a terminating null byte is not included in the returned
 name.


##
##

This is shared code, so we should refer to Posix, not linux specific man

pages.



http://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname
.html



DESCRIPTION

The gethostname() function shall return the standard host name for the
current machine. The namelen argument shall specify the size of the
array pointed to by the name argument. The returned name shall be
null-terminated, except that if namelen is an insufficient length to
hold the host name, then the returned name shall be truncated and it
is unspecified whether the returned name is null-terminated.

Host names are limited to {HOST_NAME_MAX} bytes.

RETURN VALUE

Upon successful completion, 0 shall be returned; otherwise, -1 shall
be returned.



Note that there is no indication what happens if the buffer is too
small. It may zero-terminate, it may not. It may return an error, it
may not. Decision is left to the platform imp

Re: RFR(XS): 8202181: Correctly specify size of hostname buffer in Unix Inet*AddressImpl_getLocalHostName implementations

[vyom tewari]

On Friday 27 April 2018 10:58 AM, Thomas Stüfe wrote:

On Fri, Apr 27, 2018 at 5:57 AM, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Christoph,


On Tuesday 24 April 2018 04:45 PM, Langer, Christoph wrote:

Hi Vyom,

I think, it is intentional to handle case where return "hostname" is to
large to
fit in  array.  if you see the man page(http://man7.org/linux/man-
pages/man2/gethostname.2.html) it says that it is unspecified whether
returned buffer includes a terminating null byte.

current code will put null in case of large "hostname", What do you think ?

yes, I had read the man page and saw this point of the spec. But exactly for
this purpose there's this code:

// make sure string is null-terminated
hostname[NI_MAXHOST] = '\0';

If we only hand 'NI_MAXHOST' as size value into gethostname, then the
function might only write NI_MAXHOST - 1 characters of the hostname into the
buffer.

doc says it will copy len bytes into buffer and will not copy null character
into the buffer.



C library/kernel differences
The GNU C library does not employ the gethostname() system call;
instead, it implements gethostname() as a library function that calls
uname(2) and copies up to len bytes from the returned nodename field
into name.  Having performed the copy, the function then checks if
the length of the nodename was greater than or equal to len, and if
it is, then the function returns -1 with errno set to ENAMETOOLONG;
in this case, a terminating null byte is not included in the returned
name.



This is shared code, so we should refer to Posix, not linux specific man pages.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname.html



DESCRIPTION

The gethostname() function shall return the standard host name for the
current machine. The namelen argument shall specify the size of the
array pointed to by the name argument. The returned name shall be
null-terminated, except that if namelen is an insufficient length to
hold the host name, then the returned name shall be truncated and it
is unspecified whether the returned name is null-terminated.

Host names are limited to {HOST_NAME_MAX} bytes.

RETURN VALUE

Upon successful completion, 0 shall be returned; otherwise, -1 shall
be returned.



Note that there is no indication what happens if the buffer is too
small. It may zero-terminate, it may not. It may return an error, it
may not. Decision is left to the platform implementors.

So from that, I would pass in a large-enough buffer and always
zero-terminate on success. According to Posix, a large-enough buffer
means HOST_NAME_MAX bytes.

I do not understand why we use NI_MAXHOST instead (and we we define it
to an arbitrary 1025 byte if undefined). Were there platforms where
HOST_NAME_MAX was too short? If yes, one should at least check that
NI_MAXHOST >= HOST_NAME_MAX.

Even i noticed this, why we use our own NI_MAXHOST instead HOST_NAME_MAX ?

Just for curiosity, are we facing any issues with the current code ?. Your
code change looks logical but if nothing is broken then why to change code.


If it can be proven by looking at the API description that it is
broken for some corner case, why keep it broken?
 :) Agreed, as i said Christoph change is logically correct  but i 
don't know the history behind current code, so just wanted to be sure 
that  we are not missing any corner case.


Thanks,
Vyom



Thanks, Thomas


Thanks,
Vyom

Best regards
Christoph

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html). 
I incorporated most of the review comments. Chris as you suggested in 
below mail i did not added the note for upper-bound because values are 
also OS specific.


Thanks,

Vyom


On Monday 23 April 2018 07:26 PM, Chris Hegarty wrote:


On 23/04/18 13:34, Alan Bateman wrote:

On 23/04/2018 13:05, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.html). 
I incorporated  most of the review comments.
This looks much better but I think the second paragraph of the spec 
of each option needs to be inverted so that it states clearly what 
the options does before it gets into the background of SO_KEEPALIVE. 
For example, TCP_KEEPALIVE should say that it sets the idle period 
for keep alive before it explains SO_KEEPALIVE. 


Mea culpa, I ordered the paragraphs as they are to be consistent
with TCP_QUICKACK. I don't have any objection if they are reversed.

The currently wording also begs questions as to whether the socket 
option means anything when SO_KEEPALIVE is not enabled.


It's implicit. The option can still be set and its value retrieved.


Also it probably should say something about whether it can be changed 
at any time or not.


You can set it any time. Of course, it may not be effective
immediately, depending on the exact state of the socket.

We may also want to add a note that the accepted values may
have an upper-bound. For example, the following is the largest
set of values that I can set on my Ubuntu Linux, without an
exception being thrown [*].

 TCP_KEEPIDLE = 32767
 TCP_KEEPINTERVAL = 32767
 TCP_KEEPCOUNT = 127

-Chris.

[*] "java.net.SocketException: Invalid argument" when a given
value is "too" large.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi Ivan,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html). 
Please find my answers in-line.


Thanks,

vyom


On Saturday 12 May 2018 11:15 AM, Ivan Gerasimov wrote:

Hi Vyom!

1)
src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java

Thank you for fixing ExtendingSocketOption.options0().
It may be better to make the returned set unmodifiable, and then 
Collectors.toUnmodifiableSet could be used for convenience:


return options.stream()
    .filter((option) -> !option.name().startsWith("TCP_"))
    .collect(Collectors.toUnmodifiableSet());

Also, I think Alan suggested to filter out UDP_* options for 
SOCK_STREAM here.

fixed


2)
src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java

If you static-import ExtendedSocketOptions.SOCK_DGRAM as in other 
files, then the line 80 wouldn't become too long.


The same applies to 
src/java.base/unix/classes/java/net/PlainSocketImpl.java

fixed


3)
src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java

Was it intentional that when flowSupported == true, quickAckSupported 
== false, keepAliveOptSupported == true, the TCP_KEEP* options are 
*not* added to the resulting set?


If not, then can this set population be organized in more linear way:  
Just create an ArrayList, conditionally fill it in and return 
unmodifiable set with Set.of(list.toArray()).
of course not, i  don't know but i always prefer complex nested 
"if-else" then linear ifs :)  fixed as well.


Nit:  please place the equal sign at line 172 consistently with the 
other two inits above.

fixed.


With kind regards,
Ivan


On 5/11/18 7:43 PM, vyom tewari wrote:
thanks all for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html). 
I address most of the  review comments.


Vyom


On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:

On 11 May 2018, at 01:04, Alan Bateman <alan.bate...@oracle.com> wrote:

On 10/05/2018 16:21, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html) 


...
It would be better if the channel implementation didn't static 
import ExtendedSocketOptions.getInstance as that is a very generic 
method method name. As I mentioned previously, you could simplify 
all these usages if you add the following to 
sun.net.ext.ExtendedSocketOption
    static Set<SocketOption> options(int type) { return 
getInstance().options(type)); }

+1

A minor comment on tests is that they can use List.of instead of 
Arrays.asList.

+1

Otherwise, this looks very good.

-Chris.

P.S. A separate issue, but when reviewing this it reminded me that 
we should deprecate-for-removal jdk/net/Sockets.java. It’s 
functionality is already supported by a standard API.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

On Saturday 12 May 2018 04:14 PM, Ivan Gerasimov wrote:

Thanks Vyom!

I like it much better now.

The last minorish comment:

src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java
Set<SocketOption> options = new HashSet<>(5);

Please note that the constructor of HashSet wants the initial capacity 
as the argument, not the expected number of elements.
So in this case it would be more accurate to have (7), so that 7 * 
0.75 = 5.25 > 5.
Practically, it wouldn't make any difference, as both 5 and 7 would be 
rounded up to 8 anyways.


thanks for detail explanation :) , i did not saw the code but i was 
under impression that it will use size as the nearest prime number for 
uniform distribution of elements in general.


However, I would recommend using the default constructor just to avoid 
confusion.
Or, alternatively, collect the options to an ArrayList of desired 
capacity and then make unmodifiable set with Set.copyOf(list).


No further comments from my side :)
if no further comment from others as well, i will change it to use 
default constructor at time of pushing.


Thanks,
Vyom



With kind regards,
Ivan


On 5/12/18 2:21 AM, vyom tewari wrote:

Hi Ivan,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html). 
Please find my answers in-line.


Thanks,

vyom


On Saturday 12 May 2018 11:15 AM, Ivan Gerasimov wrote:

Hi Vyom!

1)
src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java

Thank you for fixing ExtendingSocketOption.options0().
It may be better to make the returned set unmodifiable, and then 
Collectors.toUnmodifiableSet could be used for convenience:


return options.stream()
    .filter((option) -> !option.name().startsWith("TCP_"))
    .collect(Collectors.toUnmodifiableSet());

Also, I think Alan suggested to filter out UDP_* options for 
SOCK_STREAM here.

fixed


2)
src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java

If you static-import ExtendedSocketOptions.SOCK_DGRAM as in other 
files, then the line 80 wouldn't become too long.


The same applies to 
src/java.base/unix/classes/java/net/PlainSocketImpl.java

fixed


3)
src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java

Was it intentional that when flowSupported == true, 
quickAckSupported == false, keepAliveOptSupported == true, the 
TCP_KEEP* options are *not* added to the resulting set?


If not, then can this set population be organized in more linear 
way:  Just create an ArrayList, conditionally fill it in and return 
unmodifiable set with Set.of(list.toArray()).
of course not, i  don't know but i always prefer complex nested 
"if-else" then linear ifs :)  fixed as well.


Nit:  please place the equal sign at line 172 consistently with the 
other two inits above.

fixed.


With kind regards,
Ivan


On 5/11/18 7:43 PM, vyom tewari wrote:
thanks all for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html). 
I address most of the  review comments.


Vyom


On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:
On 11 May 2018, at 01:04, Alan Bateman <alan.bate...@oracle.com> 
wrote:

On 10/05/2018 16:21, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html) 


...
It would be better if the channel implementation didn't static 
import ExtendedSocketOptions.getInstance as that is a very 
generic method method name. As I mentioned previously, you could 
simplify all these usages if you add the following to 
sun.net.ext.ExtendedSocketOption
    static Set<SocketOption> options(int type) { return 
getInstance().options(type)); }

+1

A minor comment on tests is that they can use List.of instead of 
Arrays.asList.

+1

Otherwise, this looks very good.

-Chris.

P.S. A separate issue, but when reviewing this it reminded me that 
we should deprecate-for-removal jdk/net/Sockets.java. It’s 
functionality is already supported by a standard API.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

thanks all for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html). 
I address most of the  review comments.


Vyom


On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:

On 11 May 2018, at 01:04, Alan Bateman <alan.bate...@oracle.com> wrote:

On 10/05/2018 16:21, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html)
...

It would be better if the channel implementation didn't static import 
ExtendedSocketOptions.getInstance as that is a very generic method method name. 
As I mentioned previously, you could simplify all these usages if you add the 
following to sun.net.ext.ExtendedSocketOption
static Set<SocketOption> options(int type) { return 
getInstance().options(type)); }

+1


A minor comment on tests is that they can use List.of instead of Arrays.asList.

+1

Otherwise, this looks very good.

-Chris.

P.S. A separate issue, but when reviewing this it reminded me that we should 
deprecate-for-removal jdk/net/Sockets.java. It’s functionality is already 
supported by a standard API.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.6/index.html). 
I renamed the macosx files to 
"MacOSXSocketOptions.java".


Our internal tests are clean.

Thanks,
Vyom

On Tuesday 15 May 2018 03:13 PM, Alan Bateman wrote:

On 15/05/2018 08:35, Langer, Christoph wrote:


I’m asking because I’m planning to add some AIX options and will have 
to choose a name for this implementation eventually.


@Alan: What do you think?


Yes, I agree it should be renamed. Vyom has just finalized the CSR so 
I assume the final points around the implementation and naming of the 
JDK internal classes can be sorted out while the CSR is in progress.


-Alan.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

On Tuesday 15 May 2018 03:13 PM, Alan Bateman wrote:

On 15/05/2018 08:35, Langer, Christoph wrote:


I’m asking because I’m planning to add some AIX options and will have 
to choose a name for this implementation eventually.


@Alan: What do you think?


Yes, I agree it should be renamed. Vyom has just finalized the CSR so 
I assume the final points around the implementation and naming of the 
JDK internal classes can be sorted out while the CSR is in progress.


sure, i will rename the files to 
MacOSXSocketOptions.java I will send the modified 
webrev soon. Please do let me know if any buddy have better name 
suggestion than this.


Thanks,
vyom

-Alan.

Re: RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes

[vyom tewari]

On Friday 25 May 2018 11:19 AM, Ivan Gerasimov wrote:

Hi Wiijun!


On 5/24/18 10:13 PM, Weijun Wang wrote:


On May 25, 2018, at 11:58 AM, Ivan Gerasimov 
 wrote:


I also wonder whether a smart compiler might not flag code where 
the errors do infact have the same value:


if (errno == 11 || errno == 11) ...

At least gcc -O completely removes the second redundant test, so no 
observable changes is expected on supported platforms.
And it silently compiles without showing any warning, right? Good if 
yes.


--Max


Yep, all is good.
I've built/tested the patched JDK on all supported platforms with no 
issues.


And we already have places, where both EAGAIN and EWOULDBLOCK are used 
in one if clause (as I just replied to David):
java.base/unix/native/libnet/SocketInputStream.c, in 
NET_ReadWithTimeout():

    result = NET_NonBlockingRead(fd, bufP, len);
    if (result == -1 && ((errno == EAGAIN) || (errno == 
EWOULDBLOCK))) {





i  wrote this code and that time even i wanted to fix the other native 
code (check error code for both EAGAIN & EWOULDBLOCK)  as you did ,but i 
did not .

So the fix basically proposes to use this approach consistently.

we can at least fix the native part of code.

Vyom



With kind regards,
Ivan

Re: RFR 8202558 : Access to freed memory in java.base/unix/native/libnet/net_util_md.c

[vyom tewari]

Hi Ivan,

the url(http://cr.openjdk.java.net/~igerasim/8202558/00/webrev/ ) is not 
accessible to me.


Thanks,

Vyom


On Thursday 03 May 2018 01:51 AM, Ivan Gerasimov wrote:
I just realized that we've got a similar issue in the same file in the 
function initLocalIfs().


The webrev was updated in place to fix this issue as well.

With kind regards,

Ivan


On 5/2/18 12:54 PM, Ivan Gerasimov wrote:

Hello!

The function needsLoopbackRoute() calls initLoopbackRoutes() to 
initialize two global variables: loRoutes and nRoutes.


If realloc() fails at line 582 then loRoutes is freed, but nRoutes is 
left positive.


Then, in needsLoopbackRoute() the already freed memory will be accessed.

Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8202558
WEBREV: http://cr.openjdk.java.net/~igerasim/8202558/00/webrev/

Thanks!

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

On Thursday 26 April 2018 03:48 PM, Langer, Christoph wrote:

Hi Vyom,

what about my suggestions for renaming?

src/jdk.net/macosx/classes/jdk/net/UnixSocketOptions.java -> 
src/jdk.net/macosx/classes/jdk/net/MacOSXSocketOptions.java
src/jdk.net/macosx/native/libextnet/UnixSocketOptions.c -> 
src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
till now we don't have file name like MacOSX* so i choose to leave 
as it is but if people think "MacOSXSocketOption" is more appropriate, i 
will change the filename name in my next webrev.


Thanks,
Vyom


This would be more consistent as we have:
src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
src/jdk.net/linux/classes/jdk/net/LinuxSocketOptions.java
...
src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
src/jdk.net/solaris/classes/jdk/net/SolarisSocketOptions.java

Thanks
Christoph


-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
vyom tewari
Sent: Montag, 23. April 2018 14:06
To: Chris Hegarty <chris.hega...@oracle.com>; OpenJDK Network Dev list
<net-dev@openjdk.java.net>
Subject: Re: RFR:8194298 Add support for per Socket configuration of TCP
keepalive

Hi,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.htm
l).
I incorporated  most of the review comments.

Thanks,

Vyom


On Wednesday 18 April 2018 07:44 PM, Chris Hegarty wrote:

Vyom,

On 13/04/18 10:50, vyom tewari wrote:

Hi All,

Please review the below code.

BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298

webrev :
http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html

Here is some proposed wording for the JDK-specific extended socket
options specification.

1) Uses a consistent style across all three new options,
    and is in line with existing extended options.
2) Renamed the options slightly, to improve readability
    ( they don't need to conform to the native option names )
3) Reordered them so the build up is more logical
4) Removed inheritance from server sockets
5) Added standard verbiage about being "platform and
    system dependent
6) Added typical values. I think this is useful, as it
    gives an idea to the developer, but maybe it could be
    misleading. Can be dropped if there are concerns.

Can you please confirm that this is accurate, and also
will leave enough "wriggle" room when additional platform
support is added.

---

     /**
  * Keep-Alive idle time.
  *
  *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
  * SO_KEEPALIVE} option is enabled, TCP probes a connection that
has been
  * idle for some amount of time. The default value for this idle
period is
  * system dependent, but is typically 2 hours. The {@code
TCP_KEEPIDLE}
  * option can be used to affect this value for a given socket.
  *
  *  The value of this socket option is an {@code Integer} that
is the
  * number of seconds of idle time before keep-alive initiates a
probe. The
  * socket option is specific to stream-oriented sockets using the
TCP/IP
  * protocol. The exact semantics of this socket option are socket
type and
  * system dependent.
  *
  *  @since 11
  */
     public static final SocketOption TCP_KEEPIDLE
     = new ExtSocketOption("TCP_KEEPIDLE",
Integer.class);

     /**
  * Keep-Alive retransmission interval time.
  *
  *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
  * SO_KEEPALIVE} option is enabled, TCP probes a connection that
has been
  * idle for some amount of time. If the remote system does not
respond to a
  * keep-alive probe, TCP retransmits the probe after some amount
of time.
  * The default value for this retransmition interval is system
dependent,
  * but is typically 75 seconds. The {@code TCP_KEEPINTERVAL}
option can be
  * used to affect this value for a given socket.
  *
  *  The value of this socket option is an {@code Integer} that
is the
  * number of seconds to wait before retransmitting a keep-alive
probe. The
  * socket option is specific to stream-oriented sockets using the
TCP/IP
  * protocol. The exact semantics of this socket option are socket
type and
  * system dependent.
  *
  * @since 11
  */
     public static final SocketOption TCP_KEEPINTERVAL
     = new ExtSocketOption("TCP_KEEPINTERVAL",
Integer.class);

     /**
  * Keep-Alive retransmission maximum limit.
  *
  *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
  * SO_KEEPALIVE} option is enabled, TCP probes a connection that
has been
  * idle for some amount of time. If the remote system does not
respond to a
  * keep-alive probe, TCP retransmits the probe a certain number of
times
  * before a connection is considered to be broken. The default
value for
  * this keep-alive probe r