Re: RFR: JDK-8214560 - Use {@systemProperty} for definitions of system properties (httpserver)

[Seán Coffey]

Looks good Kiran. I'll sponsor this for you.

regards,
Sean.

On 08/10/2019 17:19, Kiran Ravikumar wrote:

Hi Guys,

Could you please review this minor fix:

Bug :https://bugs.openjdk.java.net/browse/JDK-8214560
Webrev :https://cr.openjdk.java.net/~coffeys/webrev.8214560/webrev/  

Explanation : Added the javadoc tag @systemProperty to system property 
definitions of HttpServer class to make it available in search bar on java docs.


Thanks,
Kiran

Re: RFR [13] 8225037: java.net.JarURLConnection::getJarEntry() throws NullPointerException

[Seán Coffey]

Looks fine to me.

Regards,
Sean.

On 31/05/19 11:58, Chris Hegarty wrote:

The implementation of java.net.JarURLConnection has a private String,
`entryName`, that is used to hold the optional jar file entry name. If
the  JAR file URL corresponding to the JarURLConnection points to a JAR
file and not a JAR file entry, then `entryName` will be `null` ( which
should be ok ).

The default implementation of JarURLConnection::getJarEntry should only
invoke `getJarEntry(entryName)` if the name is non-null. ( This is not
an issue in the built-in JarURLConnection handler implementation, as it
already correctly handles a null `entryName` )

https://cr.openjdk.java.net/~chegar/8225037/webrev.00/

-Chris.

Re: RFR: 8217364: Custom URLStreamHandler for jrt or file protocol can override default handler

[Seán Coffey]

Hi Claes,

Yes - looks like a fine suggestion.
http://cr.openjdk.java.net/~coffeys/webrev.8217364.03/webrev/

regards,
Sean.

On 02/05/2019 13:15, Claes Redestad wrote:

Hi Seán,

wouldn't it be more straightforward then to keep the logic intact and
skip the custom factory invocation in both cases if the protocol is
non-overrideable?

I.e., something like this:

diff -r 290283590646 src/java.base/share/classes/java/net/URL.java
--- a/src/java.base/share/classes/java/net/URL.java    Tue Apr 30 
23:47:00 2019 +0200
+++ b/src/java.base/share/classes/java/net/URL.java    Thu May 02 
14:10:57 2019 +0200

@@ -1403,8 +1403,9 @@

 URLStreamHandlerFactory fac;
 boolean checkedWithFactory = false;
+    boolean overrideableProtocol = isOverrideable(protocol);

-    if (isOverrideable(protocol) && 
jdk.internal.misc.VM.isBooted()) {

+    if (overrideableProtocol && jdk.internal.misc.VM.isBooted()) {
 // Use the factory (if any). Volatile read makes
 // URLStreamHandlerFactory appear fully initialized to 
current thread.

 fac = factory;
@@ -1440,7 +1441,7 @@

 // Check with factory if another thread set a
 // factory since our last check
-    if (!checkedWithFactory && (fac = factory) != null) {
+    if (overrideableProtocol && !checkedWithFactory && (fac = 
factory) != null) {

 handler2 = fac.createURLStreamHandler(protocol);
 }

Thanks!

/Claes

On 2019-05-02 12:33, Seán Coffey wrote:
with webrev: 
https://cr.openjdk.java.net/~coffeys/webrev.8217364.02/webrev/


regards,
Sean.

On 02/05/2019 11:00, Seán Coffey wrote:

Been thinking a bit more about this one.

Given that only initialization code will traverse through the "if 
(!isOverrideable(protocol))" check, I think we can add 
synchronization to eliminate any timing scenarios where the handlers 
Hashtable gets populated via another thread after we make the first 
handers.get call in getURLStreamHandler(String protocol)


regards,
Sean.

On 30/04/2019 18:19, Seán Coffey wrote:

Looking to correct an issue that arose during the JDK-8213942 fix.

https://bugs.openjdk.java.net/browse/JDK-8217364
https://cr.openjdk.java.net/~coffeys/webrev.8217364/webrev/

regards,
Sean.

Re: RFR: 8217364: Custom URLStreamHandler for jrt or file protocol can override default handler

[Seán Coffey]

with webrev: https://cr.openjdk.java.net/~coffeys/webrev.8217364.02/webrev/

regards,
Sean.

On 02/05/2019 11:00, Seán Coffey wrote:

Been thinking a bit more about this one.

Given that only initialization code will traverse through the "if 
(!isOverrideable(protocol))" check, I think we can add synchronization 
to eliminate any timing scenarios where the handlers Hashtable gets 
populated via another thread after we make the first handers.get call 
in getURLStreamHandler(String protocol)


regards,
Sean.

On 30/04/2019 18:19, Seán Coffey wrote:

Looking to correct an issue that arose during the JDK-8213942 fix.

https://bugs.openjdk.java.net/browse/JDK-8217364
https://cr.openjdk.java.net/~coffeys/webrev.8217364/webrev/

regards,
Sean.

Re: RFR: 8217364: Custom URLStreamHandler for jrt or file protocol can override default handler

[Seán Coffey]

Been thinking a bit more about this one.

Given that only initialization code will traverse through the "if 
(!isOverrideable(protocol))" check, I think we can add synchronization 
to eliminate any timing scenarios where the handlers Hashtable gets 
populated via another thread after we make the first handers.get call in 
getURLStreamHandler(String protocol)


http://gbr10227.uk.oracle.com/html/ws/jdk-jdk/open/webrev.8217364.02/webrev/

regards,
Sean.

On 30/04/2019 18:19, Seán Coffey wrote:

Looking to correct an issue that arose during the JDK-8213942 fix.

https://bugs.openjdk.java.net/browse/JDK-8217364
https://cr.openjdk.java.net/~coffeys/webrev.8217364/webrev/

regards,
Sean.

RFR: 8217364: Custom URLStreamHandler for jrt or file protocol can override default handler

[Seán Coffey]

Looking to correct an issue that arose during the JDK-8213942 fix.

https://bugs.openjdk.java.net/browse/JDK-8217364
https://cr.openjdk.java.net/~coffeys/webrev.8217364/webrev/

regards,
Sean.

Re: RFR 8179549: Typo in network properties documentation

[Seán Coffey]

Looks fine to me also!

Regards,
Sean.

On 14/03/19 19:41, Langer, Christoph wrote:


Looks good, +1

/Christoph

*From:*net-dev  *On Behalf Of *Chris 
Hegarty

*Sent:* Donnerstag, 14. März 2019 17:47
*To:* net-dev 
*Subject:* RFR 8179549: Typo in network properties documentation

Trivial typo fix.

diff --git 
a/src/java.base/share/classes/java/net/doc-files/net-properties.html 
b/src/java.base/share/classes/java/net/doc-files/net-properties.html


--- a/src/java.base/share/classes/java/net/doc-files/net-properties.html

+++ b/src/java.base/share/classes/java/net/doc-files/net-properties.html

@@ -1,6 +1,6 @@

 

 

Re: RFR: 8214295: Populate handlers while holding streamHandlerLock

[Seán Coffey]

On 26/11/18 12:59, Pavel Rappo wrote:

On 26 Nov 2018, at 12:12, Seán Coffey  wrote:

JDK-8213942 synchronization fix can be improved

Looks like a soft language for what actually is a race condition. I wonder at
what point should we just rewrite the thing as Alan mentioned using "fully
concurrent lookups"?
Yes - something that should be considered for a JDK 12+ fix perhaps. 
This fix

in its current guise is simple enough can can be backported.

regards,
Sean.


-Pavel

RFR: 8214295: Populate handlers while holding streamHandlerLock

[Seán Coffey]

JDK-8213942 synchronization fix can be improved. Details in bug report.

https://bugs.openjdk.java.net/browse/JDK-8214295

http://cr.openjdk.java.net/~coffeys/webrev.8214295/webrev/

--
Regards,
Sean.

Re: RFR: 8213942:URLStreamHandler initialization race

[Seán Coffey]

Thanks to all for the feedback. It makes sense to reduce the scope of 
the lock where possible.


I've updated the webrev :
http://cr.openjdk.java.net/~coffeys/webrev.8213942.v2/webrev/

Regards,
Sean.

On 20/11/18 20:59, Chris Hegarty wrote:

Sean,


On 20 Nov 2018, at 17:55, Seán Coffey  wrote:

A race condition recently reported by the WLS team. Access to the handlers 
Hashtable and the factory should be made while holding the streamHandlerLock 
lock.

WLS team also made efforts to create a reproducer. I've modified to jtreg 
format and reduced it down further for unit testing.

https://bugs.openjdk.java.net/browse/JDK-8213942
webrev: http://cr.openjdk.java.net/~coffeys/webrev.8213942/webrev/

The issue being observed only applies to protocols where there is a
built-in handler implementation, otherwise I don't see how it can occur.
The test uses `http`, and clearly there is a built-in `http` handler.

This is all very racy, partly by design, partly not. I can see that the
changes in JDK 9 in this area altered the behaviour in such a manner
that the test which ran successfully on JDK 8, no longer runs
successfully on JDK 9.

I see you have some other comments, and I share Pavel's and Alan's
concern. I've taken a look at the JDK 8 code again [1], and maybe the
safest thing here is to try to revert back to a variant that closely
resembles that.

One thing to try is to move the `if` statement L1396 outside of the
synchronized block, and then remove the `else` on L1400. That will allow
both the handlers cache to be rechecked and the factory ( if there is
one ) to re-consulted. I believe that should resolve the issue, more
closely resemble JDK 8, and also address the comments so far ( with
minimal change ).

-Chris.

[1] 
http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/net/URL.java

Re: RFR: 8213942:URLStreamHandler initialization race

[Seán Coffey]

On 20/11/2018 19:07, Alan Bateman wrote:

On 20/11/2018 17:55, Seán Coffey wrote:
A race condition recently reported by the WLS team. Access to the 
handlers Hashtable and the factory should be made while holding the 
streamHandlerLock lock.


WLS team also made efforts to create a reproducer. I've modified to 
jtreg format and reduced it down further for unit testing.


https://bugs.openjdk.java.net/browse/JDK-8213942
webrev: http://cr.openjdk.java.net/~coffeys/webrev.8213942/webrev/
Most URLs are created without specifying a handler so it means 
creating a URL will go through getURLStreamHandler most of the time. 
So I think we want to avoid locking on that code path where possible. 
It's sad that handlers is a Hashtable but I think it makes it worse to 
have it always synchronize on streamHandlerLock. Reading this code 
makes me want to replace handlers and replace it with fully concurrent 
lookups but I realize you might not want to go there for this issue 
(as I assume you are looking to back-port it). Assuming you don't want 
a re-write, have you considered always retrying handles.get(protocol) 
when you acquire the lock?
I'll consider reworking this area Alan. Like you say, if we're 
backporting, then less change could be better.


I tried the re-trying approach but didn't get the desired results. I 
think the factory Object needs to be read/set while holding the lock 
also. Will take a closer look tomorrow.


regards,
Sean.


-Alan

RFR: 8213942:URLStreamHandler initialization race

[Seán Coffey]

A race condition recently reported by the WLS team. Access to the 
handlers Hashtable and the factory should be made while holding the 
streamHandlerLock lock.


WLS team also made efforts to create a reproducer. I've modified to 
jtreg format and reduced it down further for unit testing.


https://bugs.openjdk.java.net/browse/JDK-8213942
webrev: http://cr.openjdk.java.net/~coffeys/webrev.8213942/webrev/

regards,
Sean.

Re: Adding SocketChannel toString to connection exception messages

[Seán Coffey]

As someone who works with alot of log files, I'd like to chime in and 
support Steven's end goal. Looking at a "Connection refused" error in 
the middle of a log file that possibly extends to millions of lines is 
near useless. In the era of cloud compute, diagnosing network issues is 
sure to become a more common task.


While we may not be able to put the sensitive information in an 
exception message, I think we could put it behind a (new?) system 
property which might be able to log this information. Logs contain all 
sorts of sensitive data. Take javax.net.debug=ssl output for example.


Regards,
Sean.

On 22/12/17 09:57, Chris Hegarty wrote:

On 22/12/17 01:27, David Holmes wrote:

cc'ing net-dev as that might be the more appropriate list.

On 22/12/2017 10:59 AM, Steven Schlansker wrote:


On Dec 21, 2017, at 4:35 PM, David Holmes  
wrote:


On 22/12/2017 10:29 AM, Steven Schlansker wrote:
On Dec 21, 2017, at 11:11 AM, Steven Schlansker 
 wrote:


What if ConnectException included the attempted hostname / IP / 
port SocketAddress?
java.net.ConnectException: Connection to 
'foo.mycorp.com[10.x.x.x]:12345' refused
Much more useful!  This could also be extended to various other 
socket exceptions.


I believe there are concerns with too much information that can be 
considered "sensitive" (like host names and IP addresses) appearing 
in error messages due to them ending up in log files and bug reports.


Unfortunately that's exactly the information that is crucial to 
someone trying to diagnose issues...


And to someone trying to discern private information about a network.


Could it be an opt-in policy?  Perhaps by a system property?


Well, you don't know where a log file or exception will end up.
Most likely on stackoverflow.

The expectation is that such information should be added by layers 
higher in the call chain, rather than the JDK libraries assuming it 
is okay to do.


Agreed. It may be some work, but if the issue is significant
enough to the user then it may be worth their effort, as well
as affording the opportunity to opt-out at any particular
point in the code.

Currently the alternative I'm faced with is going through every 
piece of user code and library that *might*
throw this exception and wrapping it to add this critical diagnostic 
information.  For an application that uses
java.net heavily, you can imagine how that is a tall task and 
possibly even not realistically achievable...


(Is there a written policy regarding this somewhere, or is it up to 
the personal feelings of the contributors?)


This is covered by the secure coding guidelines, under 'Confidential 
information':


http://www.oracle.com/technetwork/java/seccodeguide-139067.html#2

"Guideline 2-1 / CONFIDENTIAL-1: Purge sensitive information from 
exceptions"


Exactly.

I know for a fact that we'd have to scrub this information from test 
failures when putting the info into a bug report.


If net-dev folk can't expand further on this then I suggest filing a 
CSR request so that the CSR group can consider it. But I think this 
will be a no-go (I'm a member of the CSR group).


I have to agree with David here. I don't think that such a request
could be supported.

-Chris.

RFR(XS) : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Seán Coffey]

Looking to backport this to jdk8u-dev. There was a minor tweak in the 
testcase around how the interfaces are streamed. As a result, I'm 
carrying out a code review for the backport. Dropped use of 
"NetworkInterface.networkInterfaces();"


Presume it's ok  :

http://cr.openjdk.java.net/~coffeys/webrev.8182672.8u-dev/webrev/

https://bugs.openjdk.java.net/browse/JDK-8182672

--
Regards,
Sean.

Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Seán Coffey]

Nice suggestion Mark - I've taken that on board with latest webrev.

http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10.v3/webrev/

There was an issue with the hasHardwareAddress method also. A 'return 
false' was missing in the exception block.


Vyom - I discussed your suggestion with Chris - I think it's ok to have 
an exception trace printed per interface when an issue occurs. We'll 
leave that as is for now.


Final test version being run through test system now. Will push once 
finished.


Regards,
Sean.

On 23/06/17 15:36, Mark Sheppard wrote:

a minor observation:
perhaps a slight modification to the test to allow adaptation to jdk8  
(the genesis of the reported problem)
replacing the NetworkInterface.networkInterfaces() with static method 
which encapsulates the Stream

creation

public static Stream 
getNetworkInterfacesAsStream() throws Exception {

//return NetworkInterface.networkInterfaces();

return 
Collections.list(NetworkInterface.getNetworkInterfaces()).stream();

}


public static void main(String[] args) throws Exception {
//List toTest = 
NetworkInterface.networkInterfaces()

List toTest = getNetworkInterfacesAsStream()
.filter(hasHardwareAddress)
.collect(Collectors.toList());

nonetheless execution of the upgraded test produces neat  compact 
diagnostic output with

the exceptions displayed neatly - looks good

regards
Mark

On 23/06/2017 14:47, Seán Coffey wrote:
Thanks all. There were a few shouts from Chris in the office when he 
saw my Enumeration code ;)


I'm running this through our test system again and will push if I get 
green results.


Regards,
Sean.

On 23/06/17 14:40, Langer, Christoph wrote:

Hi,

looks like a great piece of modern concurrent Java coding :) Well 
done! +1


Best regards
Christoph


-Original Message-
From: Chris Hegarty [mailto:chris.hega...@oracle.com]
Sent: Freitag, 23. Juni 2017 15:28
To: Seán Coffey <sean.cof...@oracle.com>; Langer, Christoph
<christoph.lan...@sap.com>
Cc: net-dev <net-dev@openjdk.java.net>
Subject: Re: RFR : 8182672: Java 8u121 on Linux intermittently 
returns null for

MAC address



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

Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Seán Coffey]

Thanks all. There were a few shouts from Chris in the office when he saw 
my Enumeration code ;)


I'm running this through our test system again and will push if I get 
green results.


Regards,
Sean.

On 23/06/17 14:40, Langer, Christoph wrote:

Hi,

looks like a great piece of modern concurrent Java coding :) Well done! +1

Best regards
Christoph


-Original Message-
From: Chris Hegarty [mailto:chris.hega...@oracle.com]
Sent: Freitag, 23. Juni 2017 15:28
To: Seán Coffey <sean.cof...@oracle.com>; Langer, Christoph
<christoph.lan...@sap.com>
Cc: net-dev <net-dev@openjdk.java.net>
Subject: Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null for
MAC address



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

Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Seán Coffey]

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

[Seán Coffey]

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.

RFR : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Seán Coffey]

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.

RFR (xs) : 8177144 :sun/net/www/http/HttpClient/B8025710.java should run in ovm mode

[Seán Coffey]

Looking for a simple review. Test should be run in ovm mode :

https://bugs.openjdk.java.net/browse/JDK-8177144

diff --git a/test/sun/net/www/http/HttpClient/B8025710.java 
b/test/sun/net/www/http/HttpClient/B8025710.java

--- a/test/sun/net/www/http/HttpClient/B8025710.java
+++ b/test/sun/net/www/http/HttpClient/B8025710.java
@@ -37,6 +37,7 @@
  * @test
  * @bug 8025710
  * @summary Proxied https connection reuse by HttpClient can send 
CONNECT to the server

+ * @run main/othervm B8025710
  */
 public class B8025710 {

--
Regards,
Sean.

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

[Seán Coffey]

spotted an interesting blog on the MSDN timeout issue here :
https://www.frameflow.com/ping-utility-flaw-in-windows-api-creating-false-timeouts/

Regards,
Sean.

On 21/09/16 17:42, Mark Sheppard wrote:


the IcmpSendEcho series of calls come with some idiosyncrasies in that 
there is a minimum timeout that they can handle
think it is about 1000msecs. isReachable can specify a finer grained 
timeout hence the need for timeout check


regards
Mark

On 21/09/2016 17:18, 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 [9] 8041924: [TESTBUG] sun/net/www/http/ChunkedOutputStream/checkError.java fails on some systems

[Seán Coffey]

Nice clean up Chris. Looks fine.

Regards,
Sean.

On 09/06/16 20:49, Chris Hegarty wrote:

This test has been seen to fail intermittently. There is an assumption in the
test that the client-side will fill the outgoing TCP buffer by writing 1 
megabyte
of data, allowing for the server-side to close the connection out from under
it. This is the crux of the test. By slowing down the writes on the client-side
we can eliminate this assumption. The remainder of the changes just
modernise the code, and remove many unnecessary synchronization points.

http://cr.openjdk.java.net/~chegar/8041924/
https://bugs.openjdk.java.net/browse/JDK-8041924

-Chris.

Re: RFR [9] 8085785: sun/net/www/protocol/http/ZoneId.java timeout intermittently

[Seán Coffey]

Looks fine to me also Chris.

Regards,
Sean.

On 30/05/2016 22:52, Mark Sheppard wrote:

Hi Chris,
   this looks good  ... so the server now listens on wildcard and the 
client uses IPv6 loopback as the destination address.
The use of NO_PROXY, is good. I wouldn't have thought of that, and in 
the past, Tests have experienced firewall issues on

linux and macos perviously without this setting.

regards
Mark

On 26/05/2016 14:33, Chris Hegarty wrote:
ZoneId is attempting to verify the 'Host' header set by the 
HttpURLConnection implementation
when given an IPv6 literal containing a scope id. It does so by 
iterating the network interfaces
on the machine attempting to find one that is suitable to use as the 
listening interface for a
temporary test HTTP sever. Then it connects to it, and verifies the 
value of the 'Host' header.


This is problematic as some interfaces like teredo on Windows, or 
awdl on Mac, are not
suitable. Rather than all this, the test can use the IPv6 loopback 
address, which can contain

a scope id, if retrieved from the NetworkInterface API.

http://cr.openjdk.java.net/~chegar/8085785/
https://bugs.openjdk.java.net/browse/JDK-8085785

-Chris.

Re: RFR: 8071125: Improve exception messages in URLPermission

[Seán Coffey]

webrev updated in place with suggestion from Pavel (off thread) to place 
quotes around the problem values.


Regards,
Sean.

On 20/04/16 14:33, Seán Coffey wrote:
Looking for a review for these simple changes. URLPermissions is a 
class introduced for JDK 8. Exception messaging can be improved to 
capture context.


https://bugs.openjdk.java.net/browse/JDK-8071125
http://cr.openjdk.java.net/~coffeys/webrev.8071125/webrev/

RFR: 8071125: Improve exception messages in URLPermission

[Seán Coffey]

Looking for a review for these simple changes. URLPermissions is a class 
introduced for JDK 8. Exception messaging can be improved to capture 
context.


https://bugs.openjdk.java.net/browse/JDK-8071125
http://cr.openjdk.java.net/~coffeys/webrev.8071125/webrev/

--
Regards,
Sean.

Re: RFR JDK-8087113: Websocket API and implementation

[Seán Coffey]

On 29/03/2016 20:16, Andrej Golovnin wrote:

177 throw new 
IllegalArgumentException(String.valueOf(n));
>>
>>Could you please provide a better message for exception here?

>
>What would the purpose be? It's an internal implementation detail. If this
>exception is thrown, it means it's a bug in the implementation. And I highly
>doubt this error message would be of any help to an end-user. It's not 
something
>they can influence/change (most likely).
>What's important here is the stack-trace and te value. Maybe I will include 
more
>state to be printed.

A good error message is gold worth. It may save you hours of debugging.
Therefore if you can add more information to the exception message, then please
do it as it will help you to find and to fix bugs faster.

I'd have to agree with Andrej here. Please always consider a decent 
exception message and give sustaining/support/end users & other 
developers a hint at what happened.
Exception in thread "main" java.lang.IllegalArgumentException: 
Negative value encountered in Subscription.request : -1

reads better than :

Exception in thread "main" java.lang.IllegalArgumentException: -1
It'll help in searches and support forums for many years to come *if* a 
bug did exist in the implementation. Stacktraces can get lost also.


There seems to be a handful of areas where exception messages could be 
improved. Please consider enhancing all "new InternalError()" calls that 
have been added/modified :

e.g. : java/net/http/RawChannel.java

! if (!(event instanceof NonBlockingEvent)) {
  throw new InternalError();
  }

let's print the type of event class received.

Other examples for better messaging :

java/net/http/MessagePublisher.java


+if ((state & BINARY_EXPECTED) == 0) {
+throw new IllegalStateException("Unexpected binary message");
+}
Let's print the state value etc. (even if just to confirm it's a value 
we expect) - There are a few other similar areas for improvement which 
should be simple enough to find from your webrev.


regards,
Sean.

Re: Patch for adding SO_REUSEPORT socket option

[Seán Coffey]

One comment  on the supportability side from me :

java/net/AbstractPlainDatagramSocketImpl.java

> + case SO_REUSEPORT:
> + if (o == null || !(o instanceof Boolean)) {
> + throw new SocketException("bad argument for 
SO_REUSEPORT");

> + }
> + if 
(!supportedOptions().contains(StandardSocketOptions.SO_REUSEPORT)) {
> + throw new 
UnsupportedOperationException("unsupported option");


Can we improve the message ? e.g "SO_REUSEPORT is an unsupported 
option"  - line occurs twice.


Ditto for java/net/AbstractPlainSocketImpl.java and 
java/net/PlainDatagramSocketImpl.java and java/net/PlainSocketImpl.java  
(windows & unix variants)



java/net/DualStackPlainDatagramSocketImpl.java, 
java/net/DualStackPlainSocketImpl.java, java/net/PlainSocketImpl.java, 
java/net/TwoStacksPlainDatagramSocketImpl.java and 
java/net/TwoStacksPlainSocketImpl.java also need tweaking on that message.


Thanks,
Sean.

On 17/02/16 11:43, Chris Hegarty wrote:


From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of 
Lu, Yingqi

Sent: Thursday, February 11, 2016 9:47 AM
To: Alan Bateman ; Volker Simonis 
; Michael McMahon 

Cc: Kaczmarek, Eric ; Viswanathan, Sandhya 
; Kharbas, Kishor 
; Aundhe, Shirish 
; net-dev@openjdk.java.net

Subject: RE: Patch for adding SO_REUSEPORT socket option

Hi Alan,

Here is the most recent modification of the patch. The link is 
available here:

http://cr.openjdk.java.net/~mcberg/jdk/6432031/webrev.11/


I know this has gone through several rounds of review already.
I do not want to stall progress, I am reasonably happy with the
changes, but I have a few comments ( that can be addressed later,
if needed ).

 - MulticastSocket.java: I would add a javadoc link to
   {@linkplain StandardSocketOptions.SO_REUSEPORT SO_REUSEPORT}

 - Maybe the socket impl set/getOption(..) methods should check
   supportedOptions().contains(name) first.  There seems to be
   many places where SO_REUSEPORT is special-cased, that could
   be replaced with a general supported options check, no?

 - How useful is it to add SO_REUSEPORT to non-listening stream
   orientated sockets ?

-Chris.




Please let us know if everything is all right.

Again, thank you very much for your help!

Thanks,
Lucy


-Original Message-
From: Alan Bateman [mailto:alan.bate...@oracle.com]
Sent: Wednesday, February 10, 2016 10:38 AM
To: Lu, Yingqi ; Volker Simonis 
; Michael McMahon 

Cc: net-dev@openjdk.java.net; Viswanathan, Sandhya 
; Kharbas, Kishor 
; Aundhe, Shirish 
; Kaczmarek, Eric 

Subject: Re: Patch for adding SO_REUSEPORT socket option

On 05/02/2016 17:27, Lu, Yingqi wrote:

Hi Alan,

Here is the new modification of the patch. The link is available here:
http://cr.openjdk.java.net/~mcberg/jdk/6432031/webrev.10/

In this version, we make supportedOptions() in 
AbstractPlainSocketImpl and AbstractPlainDatagramSocketImpl return 
an immutable set of the socket options. In addition, we corrected 
couple formatting issues.


Please let us know your feedback and comments.


I've looked through the latest revision. Just a couple of small things:

In MulticastSocket then a small typo (in two places) where you have 
"SO_REUSPORT" instead of "SO_REUSEPORT". Also it links to
setOption(int,Object) then I assume it should be 
setOption(SocketOption,Object).


In AbstractPlainSocketImpl and AbstractPlainDatagramSocketImpl then 
supportedOptions looks technically correct but there is no need to 
create an unmodifiableSet on each call to supportedOptions. Instead 
you can simply do:


Set options = socketOptions; if (options == null) {
  if (isReusePortAvailable()) {
  options = new HashSet<>();
  options.addAll(super.supportedOptions());
  options.add(StandardSocketOptions.SO_REUSEPORT);
  options = Collections.unmodifiableSet(options);
  } else {
  options = super.supportedOptions();
  }
  socketOptions = options;
}
return options;

I don't see any other issues at this time and I'm happy to sponsor 
and help you get this in.


-Alan.

Re: [PING] RFR: JDK-8135259: InetAddress.getAllByName only reports "unknown error" instead of actual cause

[Seán Coffey]

Thanks for the test Ramanand. One possibility here is to enhance an 
existing test to check for your condition.


I think test/java/net/InetAddress/B6246242.java is a suitable edit. You 
can add your bug ID to the @bug tag. Would that work ?


Regards,
Sean.

On 11/02/2016 02:37, Ramanand Patil wrote:

Hi Sean,

Thank you. I have updated copyright year to the earlier patch and added a 
simple test case to cover this bug.
Please review the updated Webrev: 
http://cr.openjdk.java.net/~rpatil/8135259/webrev.01/

Regards,
Ramanand.

-Original Message-
From: Seán Coffey
Sent: Wednesday, February 10, 2016 7:47 PM
To: Ivan Gerasimov; Ramanand Patil; OpenJDK Network Dev list
Subject: Re: [PING] RFR: JDK-8135259: InetAddress.getAllByName only reports "unknown 
error" instead of actual cause

(dropping core-libs)

src change looks good to me also Ramanand. Will you also be adding a testcase 
to cover this ? I'm sure you could find one in the test base for editing.

Regards,
Sean.

On 10/02/16 13:44, Ivan Gerasimov wrote:

Hi Ramanand!

Your fix looks good to me.

I'm forwarding your request to net-dev@openjdk.java.net which is
probably more appropriate to review this fix.
It would be good, if net-dev people can confirm they're Okay with the
fix.

Sincerely yours,
Ivan


On 10.02.2016 10:08, Ramanand Patil wrote:

Hi all,

Please help me in getting this review done.

Regards,
Ramanand.

-Original Message-
From: Ramanand Patil
Sent: Friday, February 05, 2016 10:46 PM
To: core-libs-...@openjdk.java.net
Subject: RFR: JDK-8135259: InetAddress.getAllByName only reports
"unknown error" instead of actual cause

Hi all
   Please review the fix for bug:
https://bugs.openjdk.java.net/browse/JDK-8135259

Bug Description: Attempts to resolve a host unknown to the DNS server
cause an UnknownHostException stating "unknown error" instead of
"Name or service not known".

Webrev: http://cr.openjdk.java.net/~rpatil/8135259/webrev.00/

Fix: Using a function call directly "gai_strerror(gai_error)" instead
of using the function pointer which was declared but not initialized.
Apart from this I have removed few other unused variables and
function pointers. Brief description of how this bug was introduced
is added to the bug comment.

Regards,

Ramanand.

Re: RFR 8065076/9, test/java/net/SocketPermission/SocketPermissionTest.java failed intermittently

[Seán Coffey]

The changes look fine to me also Chris.

Regards,
Sean.

On 22/01/16 14:49, Chris Hegarty wrote:

On 21/01/16 22:55, Felix Yang wrote:

Hi Chris,
  your fix is cool. I will assign the bug to you:)
  a comment on this fix. The test changed system SecurityManager and
it is not executed with othervm mode. I think you need to rollback the
change after test.


I will revert the change and have the test run in othervm mode.

I did do a complete test run with this change and it did not cause
any problems, but then again the policy is all permissions!

Thanks Felix.

-Chris.

> Otherwise it may affect other tests.


Thanks,
Felix

On Jan 20, 2016, at 12:45 PM, Chris Hegarty > wrote:

On 20 Jan 2016, at 06:36, Chris Hegarty > wrote:


Felix,

On 14 Jan 2016, at 06:07, Felix Yang > wrote:


Hi all,
 please review the fix for
test/java/net/SocketPermission/SocketPermissionTest.java, which
fails frequently with "java.net.BindException: Address already in 
use".


Bug: https://bugs.openjdk.java.net/browse/JDK-8065076
Webrev: http://cr.openjdk.java.net/~xiaofeya/8065076/webrev.00


My preference is to avoid getFreePort. It is problematic and I
believe just obfuscates
the intermittent failures further.

In many of the test scenarios the “listening” socket can be created
before the specific
access control context and associated permission are created.

I’ll see if I can get some time to try this out.


I spent a little time on this today. I basically rewrote the test, but
kept the
same test scenarios. The use of data providers was cute, but not 
workable

since there is no common supertype for the socket classes. I decided to
just expand out the test cases manually. This will give the same test
coverage, but should be stable since it creates the sockets first, 
on an

ephemeral port, and then constructs the permissions appropriately given
that port.

http://cr.openjdk.java.net/~chegar/8065076/

The webrev diffs are almost useless, just review the new file, and 
compare

test scenarios against the what is in the old file.

-Chris.

Re: Request for review: 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation Extension

[Seán Coffey]

Hey Vinnie,

question on SSLParameters.setApplicationProtocols(String[] protocols) method

What happens if you pass an empty array into this method. Shouldn't it 
throw an IllegalArgumentException  ?




In ALPNExtension.java :

+if (listLength < 2 || listLength + 2 != len) {
+throw new SSLProtocolException(
+"Invalid " + type + " extension: incorrect list length");

Can you print the listLength value in exception message ?


+throw new SSLProtocolException(
+"Invalid " + type + " extension: too short");

Can you print the len value here ?


+if (remaining != 0) {
+throw new SSLProtocolException(
+"Invalid " + type + " extension: extra data");

Can you print remaining value here ?

Regards,
Sean.

On 01/12/2015 01:53, Xuelei Fan wrote:

Looks fine to me.  Thanks for the update.

Xuelei

On 12/1/2015 7:08 AM, Vincent Ryan wrote:

Thanks for your review. Comments in-line.


On 30 Nov 2015, at 06:30, Xuelei Fan  wrote:

Looks fine to me.  Just a few minor comments.

ServerHandshaker.java
=
There is a typo of the first line.
- /* Copyright (c) 1996, 2015, ...
+ /*
+  * Copyright (c) 1996, 2015 …


Done.


line 538-546

   String negotiatedValue = null;
   List protocols = clientHelloALPN.getPeerAPs();

   for (String ap : localApl) {
   if (protocols.contains(ap)) {
   negotiatedValue = ap;
   break;
   }
   }

I think the above order prefer the server preference order of
application protocols.  It's OK per Section 3.2 of RFC 7301.

However RFC 7301 also defines the client preference order.  Looks like
the client preference order is not necessary.  It's a little bit
confusing, and may result in different behaviors for different TLS vendors.

Maybe, we can add an option to honor server preference order in the future.

I added a comment to show that server-preference is used.
If necessary I think we can add support for controlling this via a property, 
later.



ALPNExtension.java
==
96listLength = s.getInt16(); // list length

The code would throws SSLException if the remaining input is not
sufficient.  I was wondering, SSLProtocolException may be more suitable
here.  May be nice to check the "len" argument value if sufficient for
the list length.  Otherwise, a SSLProtocolException would be thrown.

if (len >= 2) {
listLength = s.getInt16(); // list length
...
} else {
throw new SSLProtocolException(...);
}

I added the exception and more detailed message.


---
104   byte[] bytes = s.getBytes8();
105   String p =
106   new String(bytes, StandardCharsets.UTF_8); // app protocol

Do you want to check that the app protocol cannot be empty?

 // opaque ProtocolName<1..2^8-1>;  // RFC 7301
 byte[] bytes = s.getBytes8();
+if (bytes.length == 0) {
+throw new SSLProtocolException("Empty ...");
+}

Done.



-
Personally, I would prefer to use unmodifiableList for the protocolNames
variable.

It is accessible only to classes in this package.




HelloExtensions.java

line 34-37:
* This file contains all the classes relevant to TLS Extensions for the
* ClientHello and ServerHello messages. The extension mechanism and
* several extensions are defined in RFC 3546. Additional extensions are
* defined in the ECC RFC 4492.The ALPN extension is defined in RFC7301.

I may put the "Additional extensions ..." at the end.  BTW, as you are
here already, would you mind update RFC 3546 to RFC 6066?  RFC 6066 is
the newest revision of RFC 3546.

I concatenated the final sentence.



* This file contains all the classes relevant to TLS Extensions for the
* ClientHello and ServerHello messages. The extension mechanism and
* several extensions are defined in RFC 6066. The ALPN extension is
* defined in RFC 7301.  Additional extensions are defined in the ECC
* RFC 4492.

SSLEngineImpl.java
==

line 1411-1412:
private Ciphertext writeRecord(ByteBuffer[] appData,
int offset, int length, ByteBuffer netData) throws IOException {
...
  if (...) {
...
// set connection ALPN value
applicationProtocol =
handshaker.getHandshakeApplicationProtocol();
...
  }
}

Is it redundant to set applicationProtocol here.  I was wondering, the
value should set in the processInputRecord() method.

My understanding is the the wrapping is performed here and the unwrapping
performed in processInputRecord(). Isn’t the assignment required in both places?




Cheers,
Xuelei

On 11/30/2015 8:08 AM, Vincent Ryan wrote:

Hello,

Following on from Brad’s recent email, here is the full webrev of the
API and the implementation classes for ALPN:
  http://cr.openjdk.java.net/~vinnie/8144093/webrev.00/

In adds the implementation classes (sun/security/ssl) to the public API
classes (javax/net/ssl) which have already 

Re: Request for review & approval: 8141260: isReachable crash in windows xp

[Seán Coffey]

Looks ok to me Rob and provides a re-introduction of the old 
Java_java_net_Inet4AddressImpl_isReachable0 function for XP systems 
where necessary. Reviewed.


Approved for jdk8u-dev also.

Regards,
Sean.

On 25/11/15 14:00, Rob McKenna wrote:

forgot to cc net-dev

-Rob

On 24/11/15 16:37, Rob McKenna wrote:

Hi folks,

The recently updated ICMP (8133015) code fails on Windows XP due to a
missing api. This fix allows XP to fall back to the old tcp based 
method:


https://bugs.openjdk.java.net/browse/JDK-8141260
http://cr.openjdk.java.net/~robm/8141260/webrev.01/

 -Rob

Re: [9] RFR 8138953: HttpURLConnection doesn't fallback to another auth scheme if negotiate process failed

[Seán Coffey]

Thanks for handling Artem. I'll leave the main review to someone more 
knowledgeable with http authentication schemes but can I suggest that 
your print the AuthenticationHeader.authPref string out with the 
"Negotiate process failed, fallback" logger message. It's a useful 
variable to capture.


Regards,
Sean.

On 07/10/2015 12:19, Artem Smotrakov wrote:

Hello,

Please review this for 9.

According to [1], an HTTP client should try to use another HTTP 
authentication scheme if negotiate process failed for some reason, and 
a user didn't specify SPNEGO or Kerberos in "http.auth.preference" 
system property. But no fallback happens if, for example:
- an HTTP server supports both Negotiate (via Kerberos) and Basic 
authentication schemes
- first, a user provides correct Kerberos credentials, and a 
connection is successfully established with Negotiate scheme
- then, a user provides wrong Kerberos credentials, but correct Basic 
credentials


This fix updates HttpURLConnection to try another authentication 
scheme negotiate process failed, and SPNEGO and Kerberos schemes are 
not preferred. The fix may be shorter, for example:


if ( serverAuthentication != null || inNegotiate && 
!"negotiate".equals(AuthenticationHeader.authPref)) {


, but I thought that some logging might be helpful.

Also added a test which checks this and a couple of other scenarios 
work fine.


Bug: https://bugs.openjdk.java.net/browse/JDK-8138953
Webrev: http://cr.openjdk.java.net/~asmotrak/8138953/webrev.00/

[1] 
https://docs.oracle.com/javase/8/docs/technotes/guides/net/http-auth.html


Artem

Re: [8u-dev] Request for review and for approval to backport: 8072466: Deadlock when initializing MulticastSocket and DatagramSocket

[Seán Coffey]

These changes look fine to me Ivan. Approved for jdk8u-dev.

Regards,
Sean.

On 09/09/2015 17:39, Ivan Gerasimov wrote:

Hello!

I'd like to backport this recent fix from jdk9 to jdk8u-dev.
The fix applies *almost* cleanly after unshuffling.
The only manual editing I had to do was creating 
Java_java_net_DualStackPlainDatagramSocketImpl_initIDs() function in 
DualStackPlainDatagramSocketImpl.c, which didn't exist in jdk8.

All the other changes are the same as in jdk9.

Bug: https://bugs.openjdk.java.net/browse/JDK-8072466
Jdk9 change: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/3884ca98c792
Jdk9 review: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035053.html


Jdk8 webrev: http://cr.openjdk.java.net/~igerasim/8072466/01/webrev/

Would you please review/approve it?

Sincerely yours,
Ivan

RFR [8u-dev]:8072384:Setting IP_TOS on java.net sockets not working on unix

[Seán Coffey]

Michael,

I'd like to backport this fix to JDK 8u60. The port is pretty much the 
same as 9 except for modular path changes. The OptionsTest modified in 
jdk9 fix does not exist in jdk8u (test was for new API in JDK 9)


As a result, I was able to modify the jdk/net/Sockets/Test.java test 
instead. Only real way to test this in any case is via network snoops 
and they look good.


bug report : https://bugs.openjdk.java.net/browse/JDK-8072384
jdk 8u webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.8072384.8u/webrev/


--
Regards,
Sean.

Re: RFR : 8072384 : Setting IP_TOS on java.net sockets not working on unix

[Seán Coffey]

Jumping back on this issue...Michael identified a regression on linux 
with my last webrev.  It was connected with IPv6 sockets connecting to 
IPv4 interfaces. Linux seems to cater for IP_TOS settings in this area. 
I've modified my last webrev so that we only 'promote' IP_TOS requests 
to IPV6_TCLASS requests if dealing with solaris or macosx.


I discovered one bug where setting IP_TOS on the server (remote) socket 
in an IPv6 - IPv6 environment wasn't working (on the server SYN reply 
packet) So overcome that, setting both the TOS and TCLASS values works 
and setsockopt on linux doesn't complain with such an approach. A simple 
server side test using the new Sockets API might be :


jdk.net.Sockets.setOption(ss, 
java.net.StandardSocketOptions.IP_TOS, 128);
ss.bind(new 
InetSocketAddress(2606:b400:818:8f::::, 19001));

ss.accept();

This is the server socket setsockopt strace with new fix :

16788 setsockopt(4, SOL_IPV6, IPV6_V6ONLY, [0], 4) = 0
16788 setsockopt(4, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
16788 setsockopt(4, SOL_IPV6, 0x21 /* IPV6_??? */, [1], 4) = 0
16788 setsockopt(4, SOL_IPV6, 0x43 /* IPV6_??? */, [128], 4) = 0
16788 setsockopt(4, SOL_IP, IP_TOS, [128], 4) = 0

Note :
0x43 == IPV6_TCLASS  67
0x21 == IPV6_FLOWINFO_SEND   33

latest webrev : http://cr.openjdk.java.net/~coffeys/webrev.tos-may22/webrev/

Regards,
Sean.

On 25/03/15 17:21, Seán Coffey wrote:
I didn't see any review on this request yet. I've modified the 
OptionsTest to test for IP_TOS on ServerSockets. The 
-Djava.net.preferIPv4Stack=true addition also allows the code to test 
both stacks where applicable.


http://cr.openjdk.java.net/~coffeys/webrev.8072384.jdk9.v2/webrev/

regards,
Sean.

On 27/02/15 11:37, Seán Coffey wrote:
It looks like setting and getting the IP_TOS values on the java.net 
Sockets is currently broken for some scenarios. It's not possible to 
set the value on a ServerSocket via the jdk.net.Sockets.setOption 
API. See below for a webrev I'm proposing. It basically makes best 
efforts to set the IP_TOS value by mapping it to the IPV6_TCLASS 
option in an IPV6 enabled environment. NIO follows a similar approach.


Some of the comments in NET_SetSockOpt seem to be legacy and I've 
removed the no-op that was in place for IP_TOS. A corner case of 
setting of IP_TOS was found on Solaris. It doesn't seem to support 
setting the value once the socket is connected. I've handled this via 
catching of exception and we should be ok with this since the spec 
doesn't make any promises in this area if the socket is connected.


I've been testing various fixes across IPv4/v6 sockets on Solaris, 
Linux and macosx. So far, the proposed changes seem to work and 
wireshark traces help to confirm that TOS/TCLASS values are being set.


I still need to see if I can improve the getOpt logic for IP_TOS. At 
the moment, it can return a cached value which may not represent the 
true value in place at kernel socket level. I'll also improve test 
coverage in this area.


bug report : https://bugs.openjdk.java.net/browse/JDK-8072384
webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.8072384.jdk9.v1/webrev/


regards,
Sean.

RFR : 8072384 : Setting IP_TOS on java.net sockets not working on unix

[Seán Coffey]

It looks like setting and getting the IP_TOS values on the java.net 
Sockets is currently broken for some scenarios. It's not possible to set 
the value on a ServerSocket via the jdk.net.Sockets.setOption API. See 
below for a webrev I'm proposing. It basically makes best efforts to set 
the IP_TOS value by mapping it to the IPV6_TCLASS option in an IPV6 
enabled environment. NIO follows a similar approach.


Some of the comments in NET_SetSockOpt seem to be legacy and I've 
removed the no-op that was in place for IP_TOS. A corner case of setting 
of IP_TOS was found on Solaris. It doesn't seem to support setting the 
value once the socket is connected. I've handled this via catching of 
exception and we should be ok with this since the spec doesn't make any 
promises in this area if the socket is connected.


I've been testing various fixes across IPv4/v6 sockets on Solaris, Linux 
and macosx. So far, the proposed changes seem to work and wireshark 
traces help to confirm that TOS/TCLASS values are being set.


I still need to see if I can improve the getOpt logic for IP_TOS. At the 
moment, it can return a cached value which may not represent the true 
value in place at kernel socket level. I'll also improve test coverage 
in this area.


bug report : https://bugs.openjdk.java.net/browse/JDK-8072384
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8072384.jdk9.v1/webrev/

regards,
Sean.

RFR: 7178362: Socket impls should ignore unsupported proxy types rather than throwing

[Seán Coffey]

This issue relates to another bug that I own : JDK-8062305

It seems to be an area that causes alot of issue (for plugin mainly) and 
the suggestion from Chris in 7178362 bug report to return a direct 
connection type for bad ProxySelector implementations seems appropriate.


webrev : http://cr.openjdk.java.net/~coffeys/webrev.7178362/webrev/
bug report : https://bugs.openjdk.java.net/browse/JDK-7178362

regards,
Sean.

Re: RFR: 7178362: Socket impls should ignore unsupported proxy types rather than throwing

[Seán Coffey]

Thanks for suggested edits Chris. Taken them in. Shouldn't we also keep 
check for possibility of (p == null) ? After all, we could be dealing 
with bad proxy selector. I added  checks for null and also updated test 
case to test for same. It should help preserve behaviour in that area.


updated webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.7178362.v2/webrev/


regards,
Sean.

On 24/02/2015 17:41, Chris Hegarty wrote:

Sean,

Thanks for moving this forward. I think it might be possible to simplify this 
by flipping the logic, like:

diff --git a/src/java.base/share/classes/java/net/SocksSocketImpl.java 
b/src/java.base/share/classes/java/net/SocksSocketImpl.java
--- a/src/java.base/share/classes/java/net/SocksSocketImpl.java
+++ b/src/java.base/share/classes/java/net/SocksSocketImpl.java
@@ -388,14 +388,13 @@
  }
  while (iProxy.hasNext()) {
  p = iProxy.next();
-if (p == null || p == Proxy.NO_PROXY) {
+if (p.type() != Proxy.Type.SOCKS) {
  super.connect(epoint, remainingMillis(deadlineMillis));
  return;
  }
-if (p.type() != Proxy.Type.SOCKS)
-throw new SocketException(Unknown proxy type :  + 
p.type());
+
  if (!(p.address() instanceof InetSocketAddress))
-throw new SocketException(Unknow address type for proxy: 
 + p);
+throw new SocketException(Unknown address type for proxy: 
 + p);
  // Use getHostString() to avoid reverse lookups
  server = ((InetSocketAddress) p.address()).getHostString();
  serverPort = ((InetSocketAddress) p.address()).getPort();
@@ -707,13 +706,12 @@
  }
  while (iProxy.hasNext()) {
  p = iProxy.next();
-if (p == null || p == Proxy.NO_PROXY) {
+if (p.type() != Proxy.Type.SOCKS) {
  return;
  }
-if (p.type() != Proxy.Type.SOCKS)
-throw new SocketException(Unknown proxy type :  + 
p.type());
+
  if (!(p.address() instanceof InetSocketAddress))
-throw new SocketException(Unknow address type for proxy: 
 + p);
+throw new SocketException(Unknown address type for proxy: 
 + p);
  // Use getHostString() to avoid reverse lookups
  server = ((InetSocketAddress) p.address()).getHostString();
  serverPort = ((InetSocketAddress) p.address()).getPort();


-Chris.

On 24 Feb 2015, at 17:24, Seán Coffey sean.cof...@oracle.com wrote:


This issue relates to another bug that I own : JDK-8062305

It seems to be an area that causes alot of issue (for plugin mainly) and the 
suggestion from Chris in 7178362 bug report to return a direct connection type 
for bad ProxySelector implementations seems appropriate.

webrev : http://cr.openjdk.java.net/~coffeys/webrev.7178362/webrev/
bug report : https://bugs.openjdk.java.net/browse/JDK-7178362

regards,
Sean.

Re: Backporting the TCP loopback fast path (Windows) improvement to OpenJDK v7

[Seán Coffey]

Thanks all - Agreed changes pushed to 7u85 and I'll make a request to 
see if we can include this in 7u80 code base.


regards,
Sean.

On 30/01/2015 16:00, Alan Bateman wrote:

On 30/01/2015 12:18, Seán Coffey wrote:

Alan,

hadn't spotted the race issue. I guess we could delay setting 
ver_initialized until OSVERSIONINFO is populated.
Alternatively - like you say, I think we can revert back to the 
simpler edit :


http://cr.openjdk.java.net/~coffeys/webrev.8060170.7u.v2/webrev/
I'll run with this version unless I hear objections.

I think this better so I'd suggest go with this version.

-Alan

Re: Backporting the TCP loopback fast path (Windows) improvement to OpenJDK v7

[Seán Coffey]

Alan,

hadn't spotted the race issue. I guess we could delay setting 
ver_initialized until OSVERSIONINFO is populated.
Alternatively - like you say, I think we can revert back to the simpler 
edit :


http://cr.openjdk.java.net/~coffeys/webrev.8060170.7u.v2/webrev/
I'll run with this version unless I hear objections.

regards,
Sean.

On 29/01/2015 17:31, Alan Bateman wrote:

On 26/01/2015 13:48, Seán Coffey wrote:

Valeriy,

those changes look fine to me and I've verified that they run through 
testing fine also. webrev here for reference :

http://cr.openjdk.java.net/~coffeys/webrev.8060170.7u.v3/webrev/

Can I have a jdk7u reviewer for this change ?

There is a race in socket0 with the OS version check. Would it be 
simpler to drop that just have WASOPNOTSUPP and WSAEINVAL cause the 
option to be disabled?


-Alan

Re: Backporting the TCP loopback fast path (Windows) improvement to OpenJDK v7

[Seán Coffey]

Valeriy,

those changes look fine to me and I've verified that they run through 
testing fine also. webrev here for reference :

http://cr.openjdk.java.net/~coffeys/webrev.8060170.7u.v3/webrev/

Can I have a jdk7u reviewer for this change ?

Regards,
Sean.

On 26/01/15 10:26, Valery Kopylov (Akvelon) wrote:


Sean,

Thank you for your changes! However, instead of checking for WSAEINVAL 
we propose to check for Windows version.


There are two potential issues our approach helps to avoid:

1.On Win8 the only error code WSAEOPNOTSUPP means that loopback can’t 
be set. All other errors should throw an exception.


2.Since Windows 7 returns WSAEOPNOTSUPP and WinXP returns WSAEINVAL, 
we can’t be completely sure that all older versions that use jdk7 
return one of these two codes.


Please find a patch with implemented Windows version check in attachment.

Are such changes suitable for you?

Best regards,

Valeriy Kopylov,

Software Engineer,

Akvelon,

Skype: Valeriy.Kopylov

*From:*Martin Sawicki (MS OPEN TECH)
*Sent:* Saturday, January 24, 2015 03:21
*To:* Seán Coffey; Valery Kopylov (Akvelon); jdk7u-...@openjdk.java.net
*Cc:* Kirk Shoop (MS OPEN TECH); OpenJDK Network Dev list
*Subject:* RE: Backporting the TCP loopback fast path (Windows) 
improvement to OpenJDK v7


Sorry about the delay. Valeryi has prepared a patch – he’ll be 
responding soon.


*From:*Seán Coffey [mailto:sean.cof...@oracle.com]
*Sent:* Friday, January 23, 2015 5:55 AM
*To:* Valery Kopylov (Akvelon); Martin Sawicki (MS OPEN TECH); 
jdk7u-...@openjdk.java.net mailto:jdk7u-...@openjdk.java.net

*Cc:* Kirk Shoop (MS OPEN TECH); OpenJDK Network Dev list
*Subject:* Re: Backporting the TCP loopback fast path (Windows) 
improvement to OpenJDK v7


Valeriy,

I didn't see a follow up on this. I've modified the patch so that we 
cater for a possible WSAEINVAL error to be returned from the 
NET_EnableFastTcpLoopback call. Latest webrev :
http://cr.openjdk.java.net/~coffeys/webrev.8060170.7u.v2/webrev/ 
http://cr.openjdk.java.net/%7Ecoffeys/webrev.8060170.7u.v2/webrev/


changes only in 
http://cr.openjdk.java.net/~coffeys/webrev.8060170.7u.v2/webrev/src/windows/native/sun/nio/ch/Net.c.cdiff.html 
http://cr.openjdk.java.net/%7Ecoffeys/webrev.8060170.7u.v2/webrev/src/windows/native/sun/nio/ch/Net.c.cdiff.html


Good to push ? I'll try and get this into 7u80 but can't guarantee 
that now given that we've entered the rampdown/stabilization phase.


regards,
Sean.

On 15/01/15 12:09, Seán Coffey wrote:

Valeriy,

I used the jdk7u patch that was attached to the mail request. It
applied cleanly. I've posted webrev here for reference (patch
there also):
http://cr.openjdk.java.net/~coffeys/webrev.8060170.7u/webrev/
http://cr.openjdk.java.net/%7Ecoffeys/webrev.8060170.7u/webrev/

The new WSAIoctl call would appear to be failing with WSAEINVAL.
The NET_EnableFastTcpLoopback call already tests for WSAEOPNOTSUPP
- maybe we should test for WSAEINVAL also and perhaps return
silently if windows don't support the option. Or maybe - we state
this as desired behaviour - i.e this option is suitable for modern
windows kernels - your note below would contradict what I'm seeing
though.

The test passes for me on Windows 7 and fails on Windows XP.

regards,
Sean.

On 15/01/2015 11:15, Valery Kopylov (Akvelon) wrote:

Hi Sean,

Our TCP loopback implementation should process the option
correctly on older windows kernels. I tried to reproduce the
issue, but it works fine on my side. So I have an assumption that
part of the code present on jdk9 patch has been lost and this
causes such errors.
Did you use the patch for jdk7 sent by Martin or perform the
backporting from jdk8 or 9 by yourself?
Could you please send me output of hg diff command in jdk folder?


Best regards,
Valeriy Kopylov

-Original Message-
From: Seán Coffey [mailto:sean.cof...@oracle.com]
Sent: Tuesday, January 13, 2015 22:31
To: Martin Sawicki (MS OPEN TECH); jdk7u-...@openjdk.java.net
mailto:jdk7u-...@openjdk.java.net
Cc: Valery Kopylov (Akvelon); Kirk Shoop (MS OPEN TECH); Alan Bateman
Subject: Re: Backporting the TCP loopback fast path (Windows)
improvement to OpenJDK v7

Hi Martin,

I've run into a problem in backporting this to JDK 7u. The test
fails on our build/test systems. (windows XP) -

reason: User specified action: run main/othervm
-Djdk.net.useFastTcpLoopback StressLoopback elapsed time (seconds):
0.64
STDOUT:
STDERR:
java.net.SocketException: Invalid argument: fastLoopback
at sun.nio.ch.Net.socket0(Native Method)
at sun.nio.ch.Net.serverSocket(Net.java:445)
at

sun.nio.ch.AsynchronousServerSocketChannelImpl.init(AsynchronousServerSocketChannelImpl.java:71)
at

sun.nio.ch.WindowsAsynchronousServerSocketChannelImpl.init(WindowsAsynchronousServerSocketChannelImpl.java

Re: [8u-dev] Request for approval: JDK-6933879: URISyntaxException when non-alphanumeric characters are present in scope_id

[Seán Coffey]

Konstantin,

can you hold off pushing this fix to jdk8u for the moment ? It's a P4 
and could have behavioural consequences (something we try and avoid in 
update releases). I see JDK-8071458 was logged to track IPv6 scope 
specifications. Let's see how this soaks into JDK 9 over coming days and 
let's see how JDK-8071458 progresses.


regards,
Sean.

On 23/01/15 12:56, Konstantin Shefov wrote:

Hello,

Please approve the direct backport of the bug fix to 8u-dev

Patch applies cleanly to JDK 8u.

The bug:https://bugs.openjdk.java.net/browse/JDK-6933879
The webrev: http://cr.openjdk.java.net/~kshefov/6933879/webrev.01/

JDK 9 changeset: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/801eb37fc6c4
Review thread: 
http://mail.openjdk.java.net/pipermail/net-dev/2015-January/008834.html


Thanks
-Konstantin

Re: Backporting the TCP loopback fast path (Windows) improvement to OpenJDK v7

[Seán Coffey]

Valeriy,

I didn't see a follow up on this. I've modified the patch so that we 
cater for a possible WSAEINVAL error to be returned from the 
NET_EnableFastTcpLoopback call. Latest webrev :

http://cr.openjdk.java.net/~coffeys/webrev.8060170.7u.v2/webrev/

changes only in 
http://cr.openjdk.java.net/~coffeys/webrev.8060170.7u.v2/webrev/src/windows/native/sun/nio/ch/Net.c.cdiff.html


Good to push ? I'll try and get this into 7u80 but can't guarantee that 
now given that we've entered the rampdown/stabilization phase.


regards,
Sean.

On 15/01/15 12:09, Seán Coffey wrote:

Valeriy,

I used the jdk7u patch that was attached to the mail request. It 
applied cleanly. I've posted webrev here for reference (patch there 
also):

http://cr.openjdk.java.net/~coffeys/webrev.8060170.7u/webrev/

The new WSAIoctl call would appear to be failing with WSAEINVAL. The 
NET_EnableFastTcpLoopback call already tests for WSAEOPNOTSUPP - maybe 
we should test for WSAEINVAL also and perhaps return silently if 
windows don't support the option. Or maybe - we state this as desired 
behaviour - i.e this option is suitable for modern windows kernels - 
your note below would contradict what I'm seeing though.


The test passes for me on Windows 7 and fails on Windows XP.

regards,
Sean.

On 15/01/2015 11:15, Valery Kopylov (Akvelon) wrote:

Hi Sean,

Our TCP loopback implementation should process the option correctly 
on older windows kernels. I tried to reproduce the issue, but it 
works fine on my side. So I have an assumption that part of the code 
present on jdk9 patch has been lost and this causes such errors.
Did you use the patch for jdk7 sent by Martin or perform the 
backporting from jdk8 or 9 by yourself?

Could you please send me output of hg diff command in jdk folder?


Best regards,
Valeriy Kopylov

-Original Message-
From: Seán Coffey [mailto:sean.cof...@oracle.com]
Sent: Tuesday, January 13, 2015 22:31
To: Martin Sawicki (MS OPEN TECH); jdk7u-...@openjdk.java.net
Cc: Valery Kopylov (Akvelon); Kirk Shoop (MS OPEN TECH); Alan Bateman
Subject: Re: Backporting the TCP loopback fast path (Windows) 
improvement to OpenJDK v7


Hi Martin,

I've run into a problem in backporting this to JDK 7u. The test fails 
on our build/test systems. (windows XP) -

reason: User specified action: run main/othervm
-Djdk.net.useFastTcpLoopback StressLoopback elapsed time (seconds):
0.64
STDOUT:
STDERR:
java.net.SocketException: Invalid argument: fastLoopback
at sun.nio.ch.Net.socket0(Native Method)
at sun.nio.ch.Net.serverSocket(Net.java:445)
at 
sun.nio.ch.AsynchronousServerSocketChannelImpl.init(AsynchronousServerSocketChannelImpl.java:71)
at 
sun.nio.ch.WindowsAsynchronousServerSocketChannelImpl.init(WindowsAsynchronousServerSocketChannelImpl.java:69)
at 
sun.nio.ch.WindowsAsynchronousChannelProvider.openAsynchronousServerSocketChannel(WindowsAsynchronousChannelProvider.java:83)
at 
java.nio.channels.AsynchronousServerSocketChannel.open(AsynchronousServerSocketChannel.java:140)
at 
java.nio.channels.AsynchronousServerSocketChannel.open(AsynchronousServerSocketChannel.java:161)

at StressLoopback.main(StressLoopback.java:42)
This seems to be from the fact that the new SIO_LOOPBACK_FAST_PATH 
IOCTL code is only supported on more modern windows systems. I'm 
wondering if we should change the src code to not attempt to use this 
option on older windows kernels or if I should just modify the 
testcase to not run the test on older windows systems ?


regards,
Sean.



On 09/01/2015 16:25, Martin Sawicki (MS OPEN TECH) wrote:

Sean, thank you for the update and your assistance. Looking forward.

-Original Message-
From: Seán Coffey [mailto:sean.cof...@oracle.com]
Sent: Friday, January 09, 2015 8:24 AM
To: Martin Sawicki (MS OPEN TECH); jdk7u-...@openjdk.java.net
Cc: Valery Kopylov (Akvelon); Kirk Shoop (MS OPEN TECH)
Subject: Re: Backporting the TCP loopback fast path (Windows)
improvement to OpenJDK v7

Hey Martin,

I can help in getting this enhancement ported to jdk7u-dev. I'm just 
waiting on approval for use of new system property in jdk7u release.

Once I have that, I can get this in.

regards,
Sean.

On 08/01/15 20:01, Martin Sawicki (MS OPEN TECH) wrote:

Hello again,
We'd like to propose to back-port our TCP loopback fast path 
performance improvement for Windows that was recently accepted into 
OpenJDK v9 and v8 into v7.
Just for reference, the original OpenJDK v9 fix just for reference 
can be found here: 
http://hg.openjdk.java.net/jdk9/dev/jdk/rev/26e6402772c8.


Our suggested webrev patch file for OpenJDK *v7* is attached to 
this message, based on our understanding of the back-porting steps.


Your review and acceptance of this contribution would be appreciated.

Best regards,
Martin Sawicki (and Kirk Shoop, and Valeriy Kopylov) Microsoft Open
Technologies, Inc.
A subsidiary of Microsoft Corp.

RFR: 8065994: HTTP Tunnel connection to NTLM proxy reauthenticates instead of using keep-alive

[Seán Coffey]

Looking for a review around this issue that came in as a reported 
performance regression in NTLM proxy authentication. It turned out that 
HttpsClients were being discarded after Proxy SocketAddress equality 
tests failed. Lack of caching is expensive in terms for performance for 
TLS and needless handshakes. The 2nd round of NTLM authentication was 
passing in a Proxy which had a resolved SocketAddress. The previous 
Proxy creation for the same connection (via DefaultProxySelector) 
constructs Proxy using unresolved socketAddress.  Proposed fix is to 
compare like with like and have Proxy construct with unresolved Address.


I captured more details in bug report. I'm also using this opportunity 
to adding some extra logging to the HttpsClient class and to correct a 
bad null versus NO_PROXY test that existed (line 339)


bug report : https://bugs.openjdk.java.net/browse/JDK-8065994
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8065994/webrev/

regards,
Sean.

RFR : 8046588 8047187

[Seán Coffey]

Following bugs need to be backported to jdk7u-dev to correct issues 
where SO_FLOW_SLA availability might not be available or where 
sufficient permissions are not in place.


https://jbs.oracle.com/bugs/browse/JDK-8046588
https://jbs.oracle.com/bugs/browse/JDK-8047187

Testcase changes keep to the same theme as the JDK 8u fix but are in 
non-lambda format.
webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.8046588.8047187.7u/webrev/


Re-tested on a solaris box with SO_FLOW_SLA enabled and all was green. 
JPRT results are green also.


regards,
Sean.

RFR : 8032808, 8047186, 8029607, 8048212 - SO_FLOW_SLA support for solaris, Set TOS in IPv6 header

[Seán Coffey]

I'd like to backport these fixes into the jdk7u-dev codeline. They're 
already in jdk8u-dev.


https://bugs.openjdk.java.net/browse/JDK-8032808 - SO_FLOW_SLA support 
for solaris
https://bugs.openjdk.java.net/browse/JDK-8047186 - Exception handling 
change linked to above


https://bugs.openjdk.java.net/browse/JDK-8029607 - Set TOS in IPv6 header
https://bugs.openjdk.java.net/browse/JDK-8048212 - Testcase failure with 
above fix


some of the changes are quite different from jdk8u in that the make 
system is different. I've put some of the main differences in bullet 
points :


* new makefiles to capture the jdk/net classes
* Different javadoc makefile layout for 7u
* static src/share/native/java/net/jdk_net_SocketFlow.h header file 
(dynamic in jdk8u)

* Alter src/share/classes/jdk/net/Sockets.java :
  - throw new Error(e); instead of throw new InternalError(e); (older API)
* removal of lambda use in testcase

webrev : http://cr.openjdk.java.net/~coffeys/webrev.sla_tos/webrev/
CCC for 7u has been submitted.

Regards,
Sean.

Re: RFR : 8032808, 8047186, 8029607, 8048212 - SO_FLOW_SLA support for solaris, Set TOS in IPv6 header

[Seán Coffey]

Not sure how the SocksProxy.java change occurred. I'll fix that up 
before pushing. Thanks for catching.


Good point about the NetworkPermission changes for policytool - I don't 
see it in jdk 9 either (I see NetPermission being catered for) 
jdk/src/jdk.runtime/share/classes/sun/security/tools/policytool/PolicyTool.java


Will look into that as a follow up.

regards,
Sean.

On 22/08/14 17:05, Michael McMahon wrote:
There doesn't seem to be any change to SocksProxy.java apart from the 
copyright date.
Otherwise, the change looks fine. It just occurred to me that an 
update to policytool
was made to know about the new NetworkPermission class. I don't think 
we backported it to
8 even. But, for completeness sake we might want to backport it to 
both of these releases

at some point in the future.

Michael

On 22/08/14 16:24, Seán Coffey wrote:
I'd like to backport these fixes into the jdk7u-dev codeline. They're 
already in jdk8u-dev.


https://bugs.openjdk.java.net/browse/JDK-8032808 - SO_FLOW_SLA 
support for solaris
https://bugs.openjdk.java.net/browse/JDK-8047186 - Exception handling 
change linked to above


https://bugs.openjdk.java.net/browse/JDK-8029607 - Set TOS in IPv6 header
https://bugs.openjdk.java.net/browse/JDK-8048212 - Testcase failure 
with above fix


some of the changes are quite different from jdk8u in that the make 
system is different. I've put some of the main differences in bullet 
points :


* new makefiles to capture the jdk/net classes
* Different javadoc makefile layout for 7u
* static src/share/native/java/net/jdk_net_SocketFlow.h header file 
(dynamic in jdk8u)

* Alter src/share/classes/jdk/net/Sockets.java :
  - throw new Error(e); instead of throw new InternalError(e); (older 
API)

* removal of lambda use in testcase

webrev : http://cr.openjdk.java.net/~coffeys/webrev.sla_tos/webrev/
CCC for 7u has been submitted.

Regards,
Sean.

Re: RFR: 8044766 New jdk.net classes have @since 1.9 tags in 8u20

[Seán Coffey]

Just on a technical point Michael - these classes came in for JDK 8u20. 
@since 1.8 tag might be confusing. I'd suggest that we just omit the 
@since tags for 8u20. (can be documented in release notes)


Maybe we should seek clarification around whether a tag like @since 
1.8.0_20 could be used. If allowed, you could introduce that at a later 
stage ?


regards,
Sean.

On 05/06/2014 11:34, Chris Hegarty wrote:

The changes look ok to me Michael.

-Chris.

On 4 Jun 2014, at 11:58, Michael McMahon michael.x.mcma...@oracle.com wrote:


I'd like to make the same change in 8u-dev

http://cr.openjdk.java.net/~michaelm/8044766/webrev.1/

Thanks,
Michael.

Re: RFR : 7076487 : (sctp) SCTP API classes does not exist in JDK for Mac

[Seán Coffey]

thanks Michael. Will update copyrights before pushing.

regards,
Sean.

On 26/02/14 16:12, Michael McMahon wrote:
Looks good to me. The copyright date on the new src/macosx files could 
be updated though.


Thanks
Michael

On 26/02/14 15:45, Seán Coffey wrote:
Looking to backport this one to jdk7u-dev. Given the different build 
system, the make logic is different.


bug ID : https://bugs.openjdk.java.net/browse/JDK-7076487
webrev : http://cr.openjdk.java.net/~coffeys/webrev.7076487.7u/webrev/

The fix is same as the windows approach where we basically throw 
UnsupportedOperationException for SCTP operations. The sctp tests on 
Mac OSX run ok and throw the exception as expected.


regards,
Sean.

Re: RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl

[Seán Coffey]

Taken feedback on board. New webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.8024952.2/webrev/


I've managed to get confirmation from original submitter that this works 
ok for them.


regards,
Sean.

On 20/09/2013 11:29, Seán Coffey wrote:

Dmitry,

You're right. I was cautious in moving the code up but since we're 
pointing at FileDescriptor Objs, we should be ok.


Daniel Fuchs has pointed out another issue. Null delegate being passed 
into impl.accept if dealing with a custom socketImpl! It should just 
be impl.accept(s); I'll get this corrected and tested.


Thanks for pointers.
Sean.

On 20/09/13 10:20, Dmitry Samersoff wrote:

Sean,

It might be possible to set s.fd to delegate.fd before call to
impl.accept and therefore merge if instanceOf block.

-Dmitry

On 2013-09-20 00:21, Seán Coffey wrote:

Looking for review on recently reported issue. Issue seen on windows
when a custom socketImpl is in use.

bug report : https://bugs.openjdk.java.net/browse/JDK-8024952
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8024952/webrev/

Regards,
Sean.

Re: RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl

[Seán Coffey]

On 01/10/2013 13:37, Daniel Fuchs wrote:


Hi Seán,

This looks simpler and better :-)

However I wonder, do you still need to catch NPE
in CustomSocketImplFactory.main ?

Daniel,

Since I'm only creating a dummy socketImpl to test the 
classcastexception, no real networking stack is in place here. I'm 
catching the NPE that would be thrown from the native 
Java_java_net_TwoStacksPlainSocketImpl_socketAccept function since the 
underlying socket passed to it is null.


C:\tmpjava CustomSocketImplFactory
Created Socket[addr=null,port=0,localport=0]
Exception in thread main java.lang.NullPointerException: socket is null
at java.net.TwoStacksPlainSocketImpl.socketAccept(Native Method)

Regards,
Sean.



Or is that going to hide future bugs?

best regards

-- daniel (not a reviewer)

Re: RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl

[Seán Coffey]

Dmitry,

You're right. I was cautious in moving the code up but since we're 
pointing at FileDescriptor Objs, we should be ok.


Daniel Fuchs has pointed out another issue. Null delegate being passed 
into impl.accept if dealing with a custom socketImpl! It should just be 
impl.accept(s); I'll get this corrected and tested.


Thanks for pointers.
Sean.

On 20/09/13 10:20, Dmitry Samersoff wrote:

Sean,

It might be possible to set s.fd to delegate.fd before call to
impl.accept and therefore merge if instanceOf block.

-Dmitry

On 2013-09-20 00:21, Seán Coffey wrote:

Looking for review on recently reported issue. Issue seen on windows
when a custom socketImpl is in use.

bug report : https://bugs.openjdk.java.net/browse/JDK-8024952
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8024952/webrev/

Regards,
Sean.

RFR : 8024952 : ClassCastException in PlainSocketImpl.accept() when using custom socketImpl

[Seán Coffey]

Looking for review on recently reported issue. Issue seen on windows 
when a custom socketImpl is in use.


bug report : https://bugs.openjdk.java.net/browse/JDK-8024952
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8024952/webrev/

Regards,
Sean.

Re: RFR [8022584] Memory leak in some NetworkInterface methods

[Seán Coffey]

On the regression testcase theme, it got me thinking as to whether we've 
any java APIs which can query what the native heap usage of the JVM. Is 
that available over JMX ?


I tried the MBeanServer approach but didn't get the expected results. 
Queried the NonHeapMemoryUsage object.


something like.

MBeanServer server = ManagementFactory.getPlatformMBeanServer();
ObjectName memory = new ObjectName(MEMORY_MXBEAN_NAME);

Object ob = server.getAttribute(memory, NonHeapMemoryUsage);
MemoryUsage mu = MemoryUsage.from((CompositeData) ob);
System.out.println(Nonheap :  + mu.getUsed());

What exactly is JMX measuring in NonHeapMemoryUsage mode ? Any pointers ?

regards,
Sean.

On 08/08/13 23:19, Michael McMahon wrote:

Ivan,

Right, it's not worth trying to do the equivalent, whatever it is, for 
Windows.

The test is fine with me.

Thanks
Michael

On 08/08/13 23:15, Ivan Gerasimov wrote:

Michael,

The test uses /proc/self/stat file to detect changes in virtual 
memory usage.

This approach is specific for Linux.
That's why I included the check of OS in the test.

Sincerely yours,
Ivan

On 09.08.2013 1:38, Michael McMahon wrote:
Yes, definitely othervm would be required for the test. Also, why 
skip other OS'es?

The fix is only for Linux, but it might catch future leaks on Windows.

The trouble with tests like this, is they sometimes don't age well.
Future changes in OS kernel behavior could cause problems but I 
guess 32MB is a fairly large difference.

So, it should be okay

Michael

On 08/08/13 22:15, David Holmes wrote:

Main fix looks good to me.

Regression test may need some tweaking eg I think othervm will be 
needed.


Also this:

 System.out.println(WARNING: Cannot perform memory leak detection 
on this OS);


should probably just say something like Test skipped on this OS - 
there are other tests that do this so just check if there is common 
terminology. (In the future we may have keyword tags to flag this 
as linux only etc.)


Thanks,
David

On 9/08/2013 4:41 AM, Ivan Gerasimov wrote:
Chris, if it's not too late, I'd like to include a regtest in the 
fix.


Here's webrev that includes the test:
http://cr.openjdk.java.net/~igerasim/8022584/3/webrev/

The test gets past with the fixed jdk8 and fails with jdk8-b101 
and jdk7

as expected.

Sincerely yours,
Ivan

On 08.08.2013 17:50, Chris Hegarty wrote:

Looks good to me. Thanks Ivan.

-Chris.

On 08/08/2013 02:33 PM, Ivan Gerasimov wrote:

Hello Chris!

Here's the update:
http://cr.openjdk.java.net/~igerasim/8022584/2/webrev/

Thanks for offering the sponsorship!
Here's the hg-export
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch 





Sincerely yours,
Ivan

On 08.08.2013 12:43, Chris Hegarty wrote:

Thanks for taking this Ivan.

Can you please make the changes suggested by both David and Alan (
simply return NULL/-1/JNI_FALSE, as appropriate, if 
GetStringUTFChars
fails ( returns NULL ), then I will sponsor this change into 
jdk8 for

you.

Please post an update webrev to cr.openjdk.java.net.

-Chris.

On 08/08/2013 06:01 AM, David Holmes wrote:

Ivan,

On 8/08/2013 2:05 PM, Ivan Gerasimov wrote:

David, Alan,

I added checking for NULL  results and throwing OOMException if
necessary.


You don't need to throw it yourself:

   JNU_ThrowOutOfMemoryError(env, NULL);

Assuming a correct VM implementation if NULL is returned then 
an OOME
should already be pending and will be thrown as soon as your 
native

code
returns to Java.

Thanks,
David

I've also added some spaces along the code to improve 
indentation.


Would you please review the updated webrev?
http://washi.ru.oracle.com/~igerasim/webrevs/8022584/1/webrev/

Sincerely yours,
Ivan


On 08.08.2013 5:35, David Holmes wrote:

On 8/08/2013 11:20 AM, Ivan Gerasimov wrote:

Thanks Alan!

I've checked that and it turns out that GetStringUTFChars 
cannot

return
NULL.
For allocation of the result string it calls AllocateHeap() 
with

the
default EXIT_OOM fail strategy.
Thus, in case of being unable to allocate memory it simply 
stops

VM.


Ouch! That is a hotspot bug. GetStringUTFChars is supposed 
to throw

OutOfMemoryError if it has memory issues, not abort the VM!

I recommend that you check for NULL and/or a pending exception.

David


Sincerely yours,
Ivan

On 08.08.2013 5:05, Alan Bateman wrote:

(cc'ing net-dev).

The change looks okay to me. One suggestion (while you are
there) is
to check the return from GetStringUTFChars so that the name
returns
when it fails with NULL.

-Alan.

On 07/08/2013 17:46, Ivan Gerasimov wrote:

Hello everybody!

Some methods of NetworkInterface class were reported to leak
memory.
http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet
available.)

Examples are isUp() and isLoopback().

Affected versions of jdk are 6, 7 and 8

Would you please review a simple fix that removes the 
unnecessary

allocation?
http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/

Sincerely 

RFR : 8007315 HttpURLConnection.filterHeaderField method returns null where empty string is expected

[Seán Coffey]

8007315 deals with an issue in SAAJ code where a NullPointerException is 
seen.


The SAAJ code makes the assumption that non-null keys obtained from a 
httpURLConnection header will contain non-null values :


294 key = httpConnection.getHeaderFieldKey(i);
295 value = httpConnection.getHeaderField(i);  -- 
assumed to be non-null


A behavioural change came into jdk7u4 with the 7095980 fix. With the 
filtering of httpOnly cookies, when cookieHandler is set, the code 
returns null if no other values are present in the values returned for a 
header key. I'm proposing to change that to be an empty string instead ().


I've added extra scenarios to the original 7095980 unit testcase also to 
cover this scenario.


bug : http://bugs.sun.com/view_bug.do?bug_id=8007315
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8007315.jdk8/

regards,
Sean.

RFR: 7181793: Socket getInputStream/getOutputStream create streams that cannot be GC'ed until Socket is closed

[Seán Coffey]

Requesting a review for this bug which cropped up whilst cleaning up the 
FileDescriptor  associated streams some time back (7105952)


Turns out that each call to a socket.getOutputStream() creates a new 
instance of SocketOutputStream. I'm not seeing any reason to why that 
code exists. One instance should be ok here. The getInputStream() method 
doesn't have this issue.


bug report : http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7181793
webrev : http://cr.openjdk.java.net/~coffeys/webrev.7181793.jdk8/ 
http://cr.openjdk.java.net/%7Ecoffeys/webrev.7181793.jdk8/


Regards,
Sean.

Re: [7u8] Request for Approval for CR 7188755: Crash due to missing synchronization on gconf_client in DefaultProxySelector.c

[Seán Coffey]

Approved.

regards,
Sean.

On 28/08/2012 10:00, Chris Hegarty wrote:

Hi,

This is a request for approval to backport the fix for 7188755 from 
JDK8 to 7u8.


Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7188755

7188755: Crash due to missing synchronization on gconf_client in 
DefaultProxySelector.c


JDK8 changeset:
 http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e7d125f2575b

The fix is a contribution from Christian Schulte and has been reviewed 
by me ( chegar ).


Thanks,
Chris.

Re: jprt build failures on windows

[Seán Coffey]

New changes look fine Chris.

regards,
Sean.

On 11/10/2011 11:51, Chris Hegarty wrote:
There was a cut'n'paste error in the original change for CR 7098719. 
super.create(stream) should be reinstated.


After the above, now the (closed) regression test amended for CR 
7098719 fails. The reason is that TwoStacksPlainSocketImpl overrides 
the close() method, from AbstractPlainSocketImpl, and does not 
decrements the ResourceManagers count. This problem existed before CR 
7098719, it is just now being seen as the amended test catches this 
situation. The solution is to simply invoke 
ResourceManager.afterUdpClose() for non-stream sockets, just as is 
done in AbstractPlainSocketImpl.close().


Webrev for CR 7099488:
  http://cr.openjdk.java.net/~chegar/7099488/webrev.00/webrev/

Thanks,
-Chris.

On 10/11/11 10:36 AM, Chris Hegarty wrote:

Thanks Max,

I filed the following for this:
CR 7099488: TwoStacksPlainSocketImpl should invoke
super.create(stream), typo in fix for 7098719

Too many changes in too many repos!

-Chris.

On 10/11/11 06:32 AM, Weijun Wang wrote:
http://prt-web.us.oracle.com//archive/2011/10/2011-10-11-051159.ww155710.jdk//JobStatus.txt 





where T is a type-variable:
T extends Object declared in class Constructor
..\..\..\src\windows\classes\java\net\TwoStacksPlainSocketImpl.java:85:
error: no suitable method found for create()
super.create();
^
method AbstractPlainSocketImpl.create(boolean) is not applicable
(actual and formal argument lists differ in length)

Looks like something changed in a recent change

http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1e89a13d9d8f

- super.create(stream);
+ try {
+ super.create();

-Max