Re: RFR 8140472/9, java/net/ipv6tests/TcpTest.java failed intermittently with java.net.BindException: Address already in use
Thanks Chris! I will remove bug id as you suggested. TO Frank, could you sponsor the fix? Thanks, Felix On 2016/1/6 17:56, Chris Hegarty wrote: Thank Felix. On 06/01/16 08:27, Felix Yang wrote: Hi Chris, updated webrev with your suggestion: http://cr.openjdk.java.net/~xiaofeya/8140472/webrev.01/ These changes look ok to me. Trivially, I'd drop the addition of the bugId from the @bug tag, since the test is not exercising 8140472 ( it is a bug in the test itself ). -Chris. Thanks, Felix On 2016/1/6 13:49, Chris Hegarty wrote: Hi Felix, On 6 Jan 2016, at 05:09, Felix Yang wrote: Hi all, please review the fix for java/net/ipv6tests/TcpTest.java, which replaces hard-coded ports with dynamic free ports on runtime. Bug: https://bugs.openjdk.java.net/browse/JDK-8140472 Webrev: http://cr.openjdk.java.net/~xiaofeya/8140472/webrev.00 Ooh, I wasn’t aware that the getFreePort ( open and close a ServerSocket on an ephemeral port quickly, then assume that that ephemeral port is “available" ) was still in use. It has been shown to be unstable in other test areas, namely rmi. I don’t think that we should use it here. From my perspective I don’t see that the specific cases that use hard-coded ports are all that useful. Maybe they should just be removed, if they are causing problems. -Chris.
Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor
On 06/01/2016 15:37, Mark Sheppard wrote: thanks for the feedback, Alan based on suggestions, all issues have been addressed and patch has been updated http://cr.openjdk.java.net/~msheppar/8134577/webrev.07 the following should be noted: ExtensionsWithLDAP.java is added to the exclude list with JDK-8134577 - This is on the problem list as it will need to be re-written The NameService associated with this test threw UnknownHostExceptions and added the host name to a List, which was then examined later in the test as a verification. The instantiation of a NameService is as intended, i.e. if jdk.net.hosts.file set and file exist then instantiate HostsFileNameService else instantiate PlatformNameService this is in keeping with previous initialization semantics - if the no service provider NameService created then the default NameService was instantiated. I think this looks quite good. If jdk.net.hosts.file is set to a file that doesn't exist then I think it should throw an exception or else just work as if the hosts file is empty. However, it's probably not worth spending time on as this mechanism is mostly for tests. There is still a reference to the JNDI-DNS provider in HostsFileNameService and I assume that should be removed as it's essentially a historical reference now. I assume InetAddress.impl can be changed to be final. For ExtensionsWithLDAP.java then you need to create an issue to track it and use that issue number when adding it to the exclude list. I think that is all I have on this topic. -Alan
Re: RFR: 8146526: Improve java.net.URI$Parser startup characteristics
On 2016-01-06 08:43, Alan Bateman wrote: On 05/01/2016 22:47, Claes Redestad wrote: Hi, please review this patch to cleanup URI$Parser to help URI construction when run with the interpreter, mostly by inlining wrapping methods: Bug: https://bugs.openjdk.java.net/browse/JDK-8146526 Webrev: http://cr.openjdk.java.net/~redestad/8146526/webrev.01 This looks good to me. Thanks for looking at this, Alan and Chris! /Claes
Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor
thanks for the feedback, Alan based on suggestions, all issues have been addressed and patch has been updated http://cr.openjdk.java.net/~msheppar/8134577/webrev.07 the following should be noted: ExtensionsWithLDAP.java is added to the exclude list with JDK-8134577 - This is on the problem list as it will need to be re-written The NameService associated with this test threw UnknownHostExceptions and added the host name to a List, which was then examined later in the test as a verification. The instantiation of a NameService is as intended, i.e. if jdk.net.hosts.file set and file exist then instantiate HostsFileNameService else instantiate PlatformNameService this is in keeping with previous initialization semantics - if the no service provider NameService created then the default NameService was instantiated. regards Mark On 05/01/2016 13:10, Alan Bateman wrote: On 05/01/2016 00:54, Mark Sheppard wrote: Hi, as per feedback below webrev has been updated http://cr.openjdk.java.net/~msheppar/8134577/webrev.06/ change summary: * List nameServices replaced with Nameservice nameService * references to nameServices removed * private interface NameService added, with two implementation PlatformNameService and HostsFileNameService * Scanner created with UTF-8 charset * removed StringTokenizer, replaced with String.split() * comment handling extended, handling comments on line with mapping entry * try with resources added to addMappingToHostsFile method in tests I think this is starting to look good. A few small comments: - instantiateNameService (would createNameService be nicer?) still falls back to PlatformNameService when the hosts file doesn't exist. Is that expected? - HostsFileNameService.hostsFile can be final. - in removeComments then I assume you can just use indexOf and check for -1 rather than contains + indexOf. - it would be nice if NameService had some javadoc. Also the methods don't need to be declared public. - it would be nice if PlatformnameService and HostsFileNameService could use /** */ style javadoc rather than //, just to keep the code consistent. - ExtensionsWithLDAP.java is added to the exclude list with JDK-8134577, is there is separate issue for this? - formatting. There are a few places where the indention is messed up (5 spaces instead of 4 in a few places). Also some of the throws XXX are intended in many different ways. Blank lines can help readability but seem to be inconsistently used here. There's clearly been a lot of revisions on this issue and maybe the simplest is to just reformat this section of the file in the IDE. -Alan.
Re: Patch for adding SO_REUSEPORT socket option
On 05/01/2016 18:56, Lu, Yingqi wrote: Hi Alan, Sorry for the confusion. Let me be more detailed on the issue. In previous version of the patch, I added an initializer in SocketImpl.java to load the libnet.so since the isReusePortAvailable and its native implementation were there. Then, this time, I tried to move isReusePortAvailable and its native implementation declaration down to AbstractPlainSocketImpl therefore I removed the initializer block from SocketImpl.java as well. Inside AbstractPlainSocketImpl.java, there is already an initializer there by default to load the libnet.so. I was assuming it would just load the library and the code would work. However, I found out the initializer inside the AbstractPlainSocketimpl does not execute. After I add back the initializer to SocketImpl, the code works. My question is: Is this a reasonable approach to do it by keeping both initializers? Thanks very much for your help, Lucy When you updated AbstractPlainSocketImpl then is the new initializer before or after the current initializer? They run in declaration order so I'm wondering if this is the issue you are running into. -Alan
Re: RFR 8140472/9, java/net/ipv6tests/TcpTest.java failed intermittently with java.net.BindException: Address already in use
Thank Felix. On 06/01/16 08:27, Felix Yang wrote: Hi Chris, updated webrev with your suggestion: http://cr.openjdk.java.net/~xiaofeya/8140472/webrev.01/ These changes look ok to me. Trivially, I'd drop the addition of the bugId from the @bug tag, since the test is not exercising 8140472 ( it is a bug in the test itself ). -Chris. Thanks, Felix On 2016/1/6 13:49, Chris Hegarty wrote: Hi Felix, On 6 Jan 2016, at 05:09, Felix Yang wrote: Hi all, please review the fix for java/net/ipv6tests/TcpTest.java, which replaces hard-coded ports with dynamic free ports on runtime. Bug: https://bugs.openjdk.java.net/browse/JDK-8140472 Webrev: http://cr.openjdk.java.net/~xiaofeya/8140472/webrev.00 Ooh, I wasn’t aware that the getFreePort ( open and close a ServerSocket on an ephemeral port quickly, then assume that that ephemeral port is “available" ) was still in use. It has been shown to be unstable in other test areas, namely rmi. I don’t think that we should use it here. From my perspective I don’t see that the specific cases that use hard-coded ports are all that useful. Maybe they should just be removed, if they are causing problems. -Chris.
Re: RFR 8140472/9, java/net/ipv6tests/TcpTest.java failed intermittently with java.net.BindException: Address already in use
Hi Chris, updated webrev with your suggestion: http://cr.openjdk.java.net/~xiaofeya/8140472/webrev.01/ Thanks, Felix On 2016/1/6 13:49, Chris Hegarty wrote: Hi Felix, On 6 Jan 2016, at 05:09, Felix Yang wrote: Hi all, please review the fix for java/net/ipv6tests/TcpTest.java, which replaces hard-coded ports with dynamic free ports on runtime. Bug: https://bugs.openjdk.java.net/browse/JDK-8140472 Webrev: http://cr.openjdk.java.net/~xiaofeya/8140472/webrev.00 Ooh, I wasn’t aware that the getFreePort ( open and close a ServerSocket on an ephemeral port quickly, then assume that that ephemeral port is “available" ) was still in use. It has been shown to be unstable in other test areas, namely rmi. I don’t think that we should use it here. From my perspective I don’t see that the specific cases that use hard-coded ports are all that useful. Maybe they should just be removed, if they are causing problems. -Chris.
