Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown

[Michael McMahon]

On 01/10/13 19:59, Brian Burkhalter wrote:

On Oct 1, 2013, at 11:50 AM, Alan Bateman wrote:


On 01/10/2013 11:31, Brian Burkhalter wrote:

Hello net-dev members,

Please review this proposed fix at your convenience.

Summary
When looking up a host and an EAGAIN error is encountered, throw an instance of 
the new HostLookupException subclass of UnknownHostException.

Issue
https://bugs.openjdk.java.net/browse/JDK-8010371

Webrev
http://cr.openjdk.java.net/~bpb/8010371



So is getaddrinfo returning EAGAIN or is it failing with EAI_SYSTEM and errno 
set to EAGAIN?

It seems a bit unclear to me and to depend on which system one is on. There is 
also the possibility apparently of it returning EAI_AGAIN. It might be best to 
test both the return value and if that is EAI_SYSTEM to test errno.


I also wonder if the EAGAIN means the underlying syscall has been interrupted, 
in which case the normal thing to do is to retry.

The approach I initially was going to take was to retry once after a short wait 
and then fail with the proposed exception if the retry failed.


I'm not sure about the short wait. I know the submitter claims that it 
worked after 50ms, but I don't
think we could generalise that behavior in the library. Maybe, there is 
value in repeating immediately, though

it is less likely the temporary condition will have resolved.

Michael


This webrev was intended to provoke discussion such as this is the first time I 
have looked at this part of the code base and it is not clear how to actually 
test this situation.

Brian

Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown

[Michael McMahon]

On 02/10/13 15:40, Chris Hegarty wrote:

On 02/10/2013 04:44, Alan Bateman wrote:

On 01/10/2013 12:46, Brian Burkhalter wrote:

:

I updated the webrev

http://cr.openjdk.java.net/~bpb/8010371/

with changes in the test of the return value of getaddrinfo for Unix
Inet 4 and 6 and Windows Inet 6. The usual testing is in progress.

Brian

This looks better, although I think I would reverse re-write the
expressions to "if (error = ...)".

One thing to consider is whether this condition is really worth
introducing HostLookupException, particularly when it doesn't include


I am also not convinced of the merits of adding a new public checked 
Exception type for this situation. Do we expect all callers of API's 
that can throw UKE to now have to catch this Exception and provide 
their own try logic? Otherwise, what do we expect them to do with it.




It's proposed as a subclass of UnknownHostException. So, nobody would 
have to catch it.


I suppose another approach, which is a variant of the first one 
suggested, would be
retry logic built in, which is switched off by default and configurable 
somehow.


Michael

RFR: 8014719: HttpClient/ProxyTest.java failing with IAE HttpURLPermission.parseURI (and spec changes to HttpURLPermission)

[Michael McMahon]

Hi,

This is primarily a spec. change to the new HttpURLPermission class.

The main changes are:

1. Rename java.net.HttpURLPermission to java.net.URLPermission.

Even though the class is clearly intended for use with Http/s URLs it 
isn't restricted

to those URL schemes.

2. Add some of the capabilities from SocketPermission to this class too, 
namely:

- support for wildcards eg "*.foo.com"
- support for port ranges eg "4000-5000", or "5000-"

There are a couple of other minor changes, such as switching to use
the limited doPrivileged() API.

Webrev at: http://cr.openjdk.java.net/~michaelm/8014719/01/webrev/

Thanks,
Michael.

Re: RFR: 8014719: HttpClient/ProxyTest.java failing with IAE HttpURLPermission.parseURI (and spec changes to HttpURLPermission)

[Michael McMahon]

On 02/10/13 18:16, Andreas Rieber wrote:

Hi Michael,

there are a few more lines commented out in HttpURLConnection.java

//}, null, p -- replace line above, when limited 
doPriv ready


I guess they should be activated as well (line 1192, 1376 and 2517). I 
was already waiting for that change.




Right Andreas. I made the first change just before sending the webrev.
Thanks for pointing it out.

Michael.


- Andreas


On 02.10.2013 18:12, Michael McMahon wrote:

Hi,

This is primarily a spec. change to the new HttpURLPermission class.

The main changes are:

1. Rename java.net.HttpURLPermission to java.net.URLPermission.

Even though the class is clearly intended for use with Http/s URLs it 
isn't restricted

to those URL schemes.

2. Add some of the capabilities from SocketPermission to this class 
too, namely:

- support for wildcards eg "*.foo.com"
- support for port ranges eg "4000-5000", or "5000-"

There are a couple of other minor changes, such as switching to use
the limited doPrivileged() API.

Webrev at: http://cr.openjdk.java.net/~michaelm/8014719/01/webrev/

Thanks,
Michael.

RFR: 8025734: Use literal IP address where possible in SocketPermission generated by HttpURLPermission

[Michael McMahon]

Hi,

This change updates HttpURLPermission to grant a SocketPermission with a 
literal IP address
rather than the String hostname when the IP address is available (this 
gets looked up anyway.
So, we're not adding any additional name service lookups). By granting a 
permission with
a literal IP address it avoids the complicated isUntrusted() logic in 
SocketPermission
where possible. The change also enables the use of the limited 
doPrivileged logic.
I thought it would be better to enable this in a smaller change than the 
bigger API change

coming later.

This change includes a test of HttpURLPermission using a dummy 
nameservice. It is
a basic smoke test that checks SecurityException is being thrown (or 
not) when expected

as opposed to an IOException for unknown hostnames.

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

Thanks
Michael.

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

[Michael McMahon]

Can I get the following change for jdk 8 reviewed please?

It's a simple build change to enable compilation of the dummy
SCTP API layer on macosx, plus the dummy implementation
used on windows.

The existing jdk_sctp tests cover this.

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

Thanks,
Michael.

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

[Michael McMahon]

On 10/10/13 12:01, Alan Bateman wrote:

On 10/10/2013 11:10, Michael McMahon wrote:

Can I get the following change for jdk 8 reviewed please?

It's a simple build change to enable compilation of the dummy
SCTP API layer on macosx, plus the dummy implementation
used on windows.

The existing jdk_sctp tests cover this.

http://cr.openjdk.java.net/~michaelm/7076487/webrev.1/
This looks okay to me (I didn't look at the Sctp* sources closely as I 
assume they are just a copy of the dummy implementation from 
src/windows).




Yes. That's what I meant to say. The sources are copied from the windows 
tree. They just throw
UnsupportedOperationException, but it means that Sctp tests can be 
compiled on all the reference platforms now.


A minor comment (a question) is whether there is a convention to sort 
the values of EXFILES or not. If there is then you might have to 
re-order them.




Right. I'll defer to someone from build on that question.

Thanks
Michael

Re: RFR 8020758: HttpCookie constructor does not throw IAE when name contains a space

[Michael McMahon]

I agree. The change looks fine to me.

Thanks
Michael

On 23/10/13 12:09, Chris Hegarty wrote:

Mark, Michael,

java.net.HttpCookie, rightly or wrongly, supports three different 
Cookie specifications. Some of these are conflicting, and there have 
been many many bugs reported against various "special" characters 
being accepted.


This one is hopefully straight forward. The issue is specifically 
complaining about a space in the cookie name. Looking at the Netscape 
cookie draft [1] and RFC 2965 [2], they both seem to restrict a space 
in the cookie name, so I see no conflict in specs here. It seems 
reasonable to add this restriction.


http://cr.openjdk.java.net/~chegar/8020758/webrev.00/

Additionally, since we are now adding a further restriction on the 
accepted characters in the cookie name, there is a slight 
compatibility risk. I believe this risk to be very small, and cannot 
find any evidence of cookies with spaces in there names.


Thanks,
-Chris.

[1] 
http://web.archive.org/web/20020803110822/http://wp.netscape.com/newsref/std/cookie_spec.html

[2] http://www.ietf.org/rfc/rfc2965.txt

RFR: 8027570 NullPointerException in URLPermission.hashCode()

[Michael McMahon]

Interesting little bug this one. The precedence rules
were overlooked and the expected result of an expression evaluation
wasn't what was expected. The webrev is below, but the diff is

diff -r 9a5048dc7c0d src/share/classes/java/net/URLPermission.java
--- a/src/share/classes/java/net/URLPermission.javaWed Oct 30 
14:41:42 2013 +
+++ b/src/share/classes/java/net/URLPermission.javaWed Oct 30 
16:33:15 2013 +

@@ -353,7 +353,7 @@
 return getActions().hashCode()
 + scheme.hashCode()
 + authority.hashCode()
-+ path == null ? 0 : path.hashCode();
++ (path == null ? 0 : path.hashCode());
 }


Without the parentheses:

 scheme.hashCode() + authority.hashCode() + path

is evaluated as a string concatenation. The result is compared with 
null, which always fails,

meaning that path.hashCode() is always called, even when path is null 

webrev is at http://cr.openjdk.java.net/~michaelm/8027570/webrev.1/

Thanks
Michael

Re: RFR: 8027570 NullPointerException in URLPermission.hashCode()

[Michael McMahon]

Thanks Chris. I'll add the bug number

Michael

On 30/10/13 17:52, Chris Hegarty wrote:

Looks good to me Michael.

Trivially, the test could include this bug number in its @bug tag.

-Chris


On 30 Oct 2013, at 16:51, Michael McMahon  wrote:

Interesting little bug this one. The precedence rules
were overlooked and the expected result of an expression evaluation
wasn't what was expected. The webrev is below, but the diff is

diff -r 9a5048dc7c0d src/share/classes/java/net/URLPermission.java
--- a/src/share/classes/java/net/URLPermission.javaWed Oct 30 14:41:42 2013 
+
+++ b/src/share/classes/java/net/URLPermission.javaWed Oct 30 16:33:15 2013 
+
@@ -353,7 +353,7 @@
 return getActions().hashCode()
 + scheme.hashCode()
 + authority.hashCode()
-+ path == null ? 0 : path.hashCode();
++ (path == null ? 0 : path.hashCode());
 }


Without the parentheses:

 scheme.hashCode() + authority.hashCode() + path

is evaluated as a string concatenation. The result is compared with null, which 
always fails,
meaning that path.hashCode() is always called, even when path is null 

webrev is at http://cr.openjdk.java.net/~michaelm/8027570/webrev.1/

Thanks
Michael

RFR 8027687: The constructors of URLPermission class do not behave as described in javadoc

[Michael McMahon]

Simple bug fix to new URLPermission class, caused by insufficient 
parameter checking

of the constructor.

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

Thanks,
Michael

Re: RFR 8027687: The constructors of URLPermission class do not behave as described in javadoc

[Michael McMahon]

On 01/11/13 18:06, Mike Duigou wrote:

A couple minor quibbles

- Since the length is know the StringBuildiler can be created with a size.


Right, 255 is probably a good size to use.


- sb.toString() is probably more efficient than new String(sb)


Since Chris also suggests it, I'm curious why this is. Is there some 
clever sharing going on between

StringBuilder and String?


- I would like to see some IDN URL cases in the tests.


The first version of this class doesn't support Unicode strings in the 
hostname labels.

So, I'm guessing you mean cases of IDNs that have been already converted
into the ascii encoded form  (eg xn--blahblah.xn-blah.com). Something 
I'd like to do
for JDK 9 will be to allow transparent Unicode in classes like 
URLPermission with

automatic IDN conversions taking place in the http protocol handler.
So, I can add some cases of encoded IDNs in the test okay.

Thanks!

Michael


Mike

On Nov 1 2013, at 07:46 , Michael McMahon  wrote:


Simple bug fix to new URLPermission class, caused by insufficient parameter 
checking
of the constructor.

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

Thanks,
Michael

Re: RFR 8027687: The constructors of URLPermission class do not behave as described in javadoc

[Michael McMahon]

Vitaly,

Good points, I agree. I think the optimisation is worthwhile
and not costly. I also found a small casting issue with
StringBuilder.append(char)
being interpreted as StringBuilder.append(int)

Here is the updated webrev:

http://cr.openjdk.java.net/~michaelm/8027687/webrev.2/

Thanks,
Michael


On 02/11/13 01:03, Vitaly Davidovich wrote:


The size to use is the length of the argument, which you're already 
using for the loop.


On a separate note, is toLowerCase in a perf sensitive area? It makes 
an assumption that the lowering will need to happen (by always 
allocating the stringbuilder) but is that a common case? If this isn't 
perf sensitive then disregard.


Thanks

Sent from my phone

On Nov 1, 2013 4:28 PM, "Michael McMahon" 
mailto:michael.x.mcma...@oracle.com>> 
wrote:


On 01/11/13 18:06, Mike Duigou wrote:

A couple minor quibbles

- Since the length is know the StringBuildiler can be created
with a size.


Right, 255 is probably a good size to use.

- sb.toString() is probably more efficient than new String(sb)


Since Chris also suggests it, I'm curious why this is. Is there
some clever sharing going on between
StringBuilder and String?

- I would like to see some IDN URL cases in the tests.


The first version of this class doesn't support Unicode strings in
the hostname labels.
So, I'm guessing you mean cases of IDNs that have been already
converted
into the ascii encoded form  (eg xn--blahblah.xn-blah.com
<http://xn--blahblah.xn-blah.com>). Something I'd like to do
for JDK 9 will be to allow transparent Unicode in classes like
URLPermission with
automatic IDN conversions taking place in the http protocol handler.
So, I can add some cases of encoded IDNs in the test okay.

Thanks!

Michael

    Mike

On Nov 1 2013, at 07:46 , Michael McMahon
mailto:michael.x.mcma...@oracle.com>> wrote:

Simple bug fix to new URLPermission class, caused by
insufficient parameter checking
of the constructor.

webrev:
http://cr.openjdk.java.net/~michaelm/8027687/webrev.1/
<http://cr.openjdk.java.net/%7Emichaelm/8027687/webrev.1/>

Thanks,
Michael

Re: hg: jdk8/tl/jdk: 8027687: The constructors of URLPermission class do not behave as described in javad

[Michael McMahon]

On 04/11/13 18:05, mark.reinh...@oracle.com wrote:

2013/11/4 1:49 -0800, michael.x.mcma...@oracle.com:

Changeset: 48449b5390fa
Author:michaelm
Date:  2013-11-04 17:47 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/48449b5390fa

8027687: The constructors of URLPermission class do not behave as described in 
javad
Reviewed-by: chegar, mduigou

What is "javad"?

- Mark


Mark,

Sorry. That was an incomplete copy/paste. It should say "as described in 
javadoc"


Michael.

Re: RFR: Inet[4|6]Address native initializing code should check fieldID values

[Michael McMahon]

On 05/11/13 19:38, Chris Hegarty wrote:

Another installment of checks for return values from
GetFieldID, and friends, to follow up on last weeks work [1].

http://cr.openjdk.java.net/~chegar/netNullChecks/webrev/

There are more cleanups to come along the same lines, but I'd like to 
keep these changes small and confined for now.


-Chris.

[1] 
http://mail.openjdk.java.net/pipermail/net-dev/2013-October/007698.html


Just curious about the CHECK_NULLs that are removed. Is that due to some 
warning

about redundant code?

Michael

Re: RFR: Inet[4|6]Address native initializing code should check fieldID values

[Michael McMahon]

On 05/11/13 20:49, Chris Hegarty wrote:

On 5 Nov 2013, at 20:39, Michael McMahon  wrote:


On 05/11/13 19:38, Chris Hegarty wrote:
Another installment of checks for return values from
GetFieldID, and friends, to follow up on last weeks work [1].

http://cr.openjdk.java.net/~chegar/netNullChecks/webrev/

There are more cleanups to come along the same lines, but I'd like to keep 
these changes small and confined for now.

-Chris.

[1] http://mail.openjdk.java.net/pipermail/net-dev/2013-October/007698.html

Just curious about the CHECK_NULLs that are removed. Is that due to some warning
about redundant code?

No, just caught my eye when going through this. As you said they are redundant.

I have more, similar, changes to come in this area but would to move these 
through increamantally.

-Chris.


Okay. Looks fine to me.

Thanks
Michael


Michael

RFR: 8027221 test/java/net/URLPermission/nstest/LookupTest.java failing intermittently, output insufficient

[Michael McMahon]

This is a test fix. The test currently uses a fixed port number
(in the policy file and Java test)

We need to change to use a shell script so that a port can be
chosen dynamically and the appropriate policy file created.

The change also adds the correct bug number to a different test
change I pushed on Monday.

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

Thanks
Michael

Re: RFR: 8022213 Intermittent test failures in java/net/URLClassLoader (Add jdk/testlibrary/FileUtils.java)

[Michael McMahon]

Chris,

Would it be useful to add some instrumentation/logging (to System.err) 
if it's taking

more than one iteration to delete a file? We could end up with degraded
test performance if there is a general problem deleting files, and 
otherwise would have
no way of understanding what the problem is (eg. up to 7.5 seconds are 
allowed to delete the file)


Also, just curious what is the value in throwing InterruptedException 
out of the delete() method?

It seems onerous for calling code to have to catch it.

Michael

On 07/11/13 10:05, Chris Hegarty wrote:
Virus checkers and, on more modern Windows OSes, Windows Experience 
Service, and other external processes, have been causing trouble when 
it comes to tests deleting files.


On a number of occasions in the past retry logic has been added to 
tests that need to delete files. Also, the jtreg harness has similar 
logic when cleaning up temporary files.


This has reared its head again with:
  8027902: TEST_BUG: java/net/URLClassLoader/closetest/CloseTest.java 
and java/net/URLClassLoader/closetest/GetResourceAsStream.java failed 
due to dir operation failed.

  8022213: Intermittent test failures in java/net/URLClassLoader

I'd like to add file utility functions to the common jdk/testlibrary, 
starting with a "more reliable" delete. I've made minimal changes to 
the URLClassloader tests to use this ( more could be done, but this is 
just a start ).


http://cr.openjdk.java.net/~chegar/fileUtils/webrev/

The important part here for review is the "API" and impl in FileUtils.

Thanks,
-Chris.

Re: RFR: 8022213 Intermittent test failures in java/net/URLClassLoader (Add jdk/testlibrary/FileUtils.java)

[Michael McMahon]

On 07/11/13 11:34, Chris Hegarty wrote:

On 11/07/2013 11:19 AM, Michael McMahon wrote:

Chris,

Would it be useful to add some instrumentation/logging (to System.err)
if it's taking
more than one iteration to delete a file? We could end up with degraded
test performance if there is a general problem deleting files, and
otherwise would have
no way of understanding what the problem is (eg. up to 7.5 seconds are
allowed to delete the file)


I had an optional PrintStream param on delete/deleteTree, but removed 
it. If the file fails to delete, then you will see all the suppressed 
exceptions, but I agree if it fails a number of times and then 
succeeds you do not see this.


I'm not sure that it is worth adding PrintStream ( and this is why I 
removed it ) since the external interference occurs infrequently and 
does not appear to last long. I just wanted to keep this as simply as 
possible, but I could be convinced to reintroduce it.




Personally, I would have thought System.err would be okay, but maybe 
there are tests that need to run
without this kind of noise. I agree adding a PrintStream arg is not 
worth doing.



Also, just curious what is the value in throwing InterruptedException
out of the delete() method?
It seems onerous for calling code to have to catch it.


Right handling this is a pain. Since we may now be possibly sleeping 
during a delete I wanted jtreg to be able to interrupt the test and 
have the test take correct action, rather than just swallowing it. It 
is really annoying to see hung tests in logs that do not respond to 
jtregs attempts to stop them.




I wasn't suggesting to swallow the exception - rather to wrap it in an 
unchecked exception. The issue
is really only whether callers need to distinguish this situation from 
the various other errors and environmental

problems  that will all probably cause a test to be interrupted/stopped.

Michael

I've also received another comment offline about the method names. 
They should be more descriptive, and highlight the difference between 
these methods and regular delete. So maybe move to public 
deleteWithRetry?


-Chris.



Michael

On 07/11/13 10:05, Chris Hegarty wrote:

Virus checkers and, on more modern Windows OSes, Windows Experience
Service, and other external processes, have been causing trouble when
it comes to tests deleting files.

On a number of occasions in the past retry logic has been added to
tests that need to delete files. Also, the jtreg harness has similar
logic when cleaning up temporary files.

This has reared its head again with:
  8027902: TEST_BUG: java/net/URLClassLoader/closetest/CloseTest.java
and java/net/URLClassLoader/closetest/GetResourceAsStream.java failed
due to dir operation failed.
  8022213: Intermittent test failures in java/net/URLClassLoader

I'd like to add file utility functions to the common jdk/testlibrary,
starting with a "more reliable" delete. I've made minimal changes to
the URLClassloader tests to use this ( more could be done, but this is
just a start ).

http://cr.openjdk.java.net/~chegar/fileUtils/webrev/

The important part here for review is the "API" and impl in FileUtils.

Thanks,
-Chris.

Re: 8028074: InetAddress.getByName fails with UnknownHostException: invalid IPv6 address if host name starts with a-f

[Michael McMahon]

Ah, I noticed this today. Thanks for fixing it so quickly.

Michael

On 08/11/13 20:43, Chris Hegarty wrote:

The change looks good to me.

Thanks for spotting this and jumping on it so quickly.

-Chris.

On 08/11/2013 20:39, Alan Bateman wrote:


There's a small problem with the fix for JDK- 8019834 that was pushed to
jdk8/tl today. The patch means that UHE is thrown when doing a lookup of
hosts that start with a-f. I ran into it because I happen to be on a
machine that starts with "a" :-)

I'd like to get a patch into jdk8/tl quickly so that others don't run
into this. The proposed patch is attached.

-Alan

diff --git a/src/share/classes/java/net/InetAddress.java
b/src/share/classes/java/net/InetAddress.java
--- a/src/share/classes/java/net/InetAddress.java
+++ b/src/share/classes/java/net/InetAddress.java
@@ -1144,7 +1144,7 @@
ifname = host.substring (pos+1);
}
}
- if ((addr = IPAddressUtil.textToNumericFormatV6(host)) == null) {
+ if ((addr = IPAddressUtil.textToNumericFormatV6(host)) == null &&
host.contains(":")) {
throw new UnknownHostException(host + ": invalid IPv6 address");
}
} else if (ipv6Expected) {

RFR: 8028060 test/java/net/URLPermission/nstest/lookup.sh failing (win)

[Michael McMahon]

This is a fix to a test case that is failing on Windows.
A couple of problems:

1) cygwin doesn't like the newline at the end of the port number that 
gets printed to System.out


2) the calling script was not setting the classpath separator correctly 
(';' for windows)


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

Thanks
Michael.

RFR 8028581: [TESTBUG] java/net/Socket/LingerTest.java failing

[Michael McMahon]

This is a test that just failed recently on Windows, probably due to a 
timing issue.
The critical time delay in the test is increased from 5 seconds to 10 
seconds
and some extra printlns are added to show the exact sequence in case it 
fails again.


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

Thanks,
Michael

RFR: 8029354: URLPermission. throws llegalArgumentException: Invalid characters in hostname

[Michael McMahon]

Hi,

java.net.URLPermission does not currently take account of the "userinfo" 
component
in the authority of a URL. So, it does not accept URLs of the form 
"http://username@host/blah";


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

which includes a small spec change to account for this. Userinfo needs 
to be case-sensitive

for comparison, hashCode() etc.

Thanks,
Michael

RFR: 8029127: A redirect POST request does not work and illegalStateException on HttpURLConnection.getInputStream

[Michael McMahon]

This is another regression in JDK 8. A new "connecting" flag is getting 
confused

when a redirect occurs and a POST gets converted to a GET.

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

Thanks
Michael.

Re: RFR: 8029354: URLPermission. throws llegalArgumentException: Invalid characters in hostname

[Michael McMahon]

Max,

Thanks for reviewing.

On 02/12/13 08:29, Weijun Wang wrote:

Hi Michael

 492 boolean implies(Authority other) {
 493 return userinfo.equals(other.userinfo) &&
 494impliesHostrange(other) && 
impliesPortrange(other);

 495 }

This means http://example.com does not imply 
http://some...@example.com. Is this intended?




It was intended, though I admit the point is debatable. My conclusion 
was it would be
better to have an explicit syntax like http://*@example.com to mean all 
users @ example.com
rather than just assuming that the commonly used syntax 
http://example.com means (implies)
all users. But, I decided against adding the '*' syntax in the initial 
version of the class.

as I'm not sure how commonly used the userinfo actually is.


Also,

  68  * userinfo is optional and has no special significance in 
this class.


It seems that field has some significance.



The point I was trying to convey was that userinfo is just treated as an 
opaque component

but the wording is too vague, I agree.

I need to think about your first point some more and will come back with 
a proposal.


Thanks
Michael


Thanks
Max



On 11/30/13, 1:06, Michael McMahon wrote:

Hi,

java.net.URLPermission does not currently take account of the "userinfo"
component
in the authority of a URL. So, it does not accept URLs of the form
"http://username@host/blah";

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

which includes a small spec change to account for this. Userinfo needs
to be case-sensitive
for comparison, hashCode() etc.

Thanks,
Michael

Re: RFR: 8029127: A redirect POST request does not work and illegalStateException on HttpURLConnection.getInputStream

[Michael McMahon]

On 29/11/13 20:38, Alan Bateman wrote:

On 29/11/2013 18:03, Michael McMahon wrote:
This is another regression in JDK 8. A new "connecting" flag is 
getting confused

when a redirect occurs and a POST gets converted to a GET.

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

The change looks okay but I do have a few comments on the  test.

Copyright dates make it look like it came from 2006 :-)



The test it's based on came from 2006 :) But, will fix that
Is the @compile tag right, shouldn't it compile RedirectOnPost too? 
Might be easier to use /com/sun/...  as / indicates the root of the 
test tree.




It doesn't seem to need to be included, but I can add it for extra safety


Import of javax.xml.soap, I can't see this being used.



Right. The test needs some cleanup. On the SSL question, I would like
to keep that, in order to exercise HttpsURLConnection and ensure
that it works also.

Thanks
Michael

Does the test really need to use SSL? I guess I don't like seeing 
tests need to grab files from other locations in the test tree.


Is "error" used?

Should the statics be moved into main?

runTest (L72-74), looks like the formatting is messed up here.

runTest - would be nice if this used try-with-resources.

Otherwise I think the test seems to cover the redirect okay, it just 
needs a bit of polishing so that it can be maintained.


-Alan.

Re: RFR: 8029354: URLPermission. throws llegalArgumentException: Invalid characters in hostname

[Michael McMahon]

It looks like userinfo is not permitted in http URLs anyway (in rfc 
2616). And even if clients
are permissive about allowing it, any userinfo would most likely not be 
seen by a server

since the request URI only contains the path component of the original URI.

I need to look at the bug report, to see how this situation arose in the 
first place.


Michael

On 02/12/13 10:41, Weijun Wang wrote:
Is it possible to just ignore the userinfo part? I wonder if people 
will complain why "user:pass" is not the same as "user".


--Max

On 12/2/13, 18:00, Michael McMahon wrote:

This means http://example.com does not imply
http://some...@example.com. Is this intended?



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

Re: RFR: 8029354: URLPermission. throws llegalArgumentException: Invalid characters in hostname

[Michael McMahon]

Okay. I think the best approach is to recognise the userinfo but just 
remove it when constructing

URLPermissions thereby effectively ignoring it.

This is what the http protocol handler (and other support classes) have 
been doing all the time
since the field is not directly of interest to http itself. That doesn't 
prevent higher level software from
using it, as in the case here that provoked the bug report (the Java GIT 
client used in netbeans and eclipse)


Michael

On 02/12/13 11:00, Michael McMahon wrote:
It looks like userinfo is not permitted in http URLs anyway (in rfc 
2616). And even if clients
are permissive about allowing it, any userinfo would most likely not 
be seen by a server
since the request URI only contains the path component of the original 
URI.


I need to look at the bug report, to see how this situation arose in 
the first place.


Michael

On 02/12/13 10:41, Weijun Wang wrote:
Is it possible to just ignore the userinfo part? I wonder if people 
will complain why "user:pass" is not the same as "user".


--Max

On 12/2/13, 18:00, Michael McMahon wrote:

This means http://example.com does not imply
http://some...@example.com. Is this intended?



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

Re: RFR: 8029354: URLPermission. throws llegalArgumentException: Invalid characters in hostname

[Michael McMahon]

On 02/12/13 12:17, Weijun Wang wrote:



On 12/2/13, 19:00, Michael McMahon wrote:

It looks like userinfo is not permitted in http URLs anyway (in rfc
2616). And even if clients
are permissive about allowing it, any userinfo would most likely not be
seen by a server
since the request URI only contains the path component of the 
original URI.


Of course not, password should no be sent to the server in this way. 
My understanding is that it will be used by the browser when server 
requests for authentication.




Browsers seem to discourage this usage: IE doesn't allow it at all. 
Firefox helpfully asks

if you are sure about it, in case of URLs like

http:://where.you.think.you.are.going@bad.place/

Michael


--Max



I need to look at the bug report, to see how this situation arose in the
first place.

Michael

On 02/12/13 10:41, Weijun Wang wrote:

Is it possible to just ignore the userinfo part? I wonder if people
will complain why "user:pass" is not the same as "user".

--Max

On 12/2/13, 18:00, Michael McMahon wrote:

This means http://example.com does not imply
http://some...@example.com. Is this intended?



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

Re: RFR: 8029354: URLPermission. throws llegalArgumentException: Invalid characters in hostname

[Michael McMahon]

On 02/12/13 12:40, Alan Bateman wrote:

On 02/12/2013 12:22, Michael McMahon wrote:
Okay. I think the best approach is to recognise the userinfo but just 
remove it when constructing

URLPermissions thereby effectively ignoring it.

This is what the http protocol handler (and other support classes) 
have been doing all the time
since the field is not directly of interest to http itself. That 
doesn't prevent higher level software from
using it, as in the case here that provoked the bug report (the Java 
GIT client used in netbeans and eclipse)


Michael
I think this makes the most sense. Even more so when you consider 
configuring the policy to grant permission to GET or POST to a 
specific HTTP URL then this grant is independent of whether of any 
authentication that might be required.


-Alan.


The second webrev is at link below. This is somewhat simpler now.

I think it still needs a spec change though. So, I would like to propose
that to the CCC asap.

http://cr.openjdk.java.net/~michaelm/8029354/webrev.2/

I added a test that uses the protocol handler. it should be reliable enough
as it does not actually go as far as opening an actual socket connection.

Thanks
Michael

Re: RFR: 8029354: URLPermission. throws llegalArgumentException: Invalid characters in hostname

[Michael McMahon]

On 02/12/13 15:21, Alan Bateman wrote:

On 02/12/2013 14:33, Michael McMahon wrote:


The second webrev is at link below. This is somewhat simpler now.

I think it still needs a spec change though. So, I would like to propose
that to the CCC asap.

http://cr.openjdk.java.net/~michaelm/8029354/webrev.2/

I added a test that uses the protocol handler. it should be reliable 
enough
as it does not actually go as far as opening an actual socket 
connection.



This does look much simpler and I think the approach is good.

A minor point on the wording, maybe "when creating a URLPermission" 
rather than "at object creation".


Also maybe "of this this" would be better as "defined by this class".



That's fine. I've updated the docs as suggested.


Does the userinfo = "" case need special handling now?



No. But, I've added a test for empty userinfo now like http://@foo.com/
is the same as http://foo.com

Michael.


-Alan.

Re: RFR: JDK-8025437 - Check DefaultProxySelector for JNI pending exception issues

[Michael McMahon]

Yes, looks good Dan. I presume this is going into 9 rather than 8 though...?

Michael.

On 12/12/13 05:14, Dan Xu wrote:

Hi All,

Please review the fix for the JNI pending exception issue in 
src/solaris/native/sun/net/spi/DefaultProxySelector.c. I have 
refactored the code so that JNI calls will only be run when previous 
JNI calls succeed. Therefore, it is impossible for an unsafe JNI call 
to happen. Please help review it. Thanks!


Bug: https://bugs.openjdk.java.net/browse/JDK-8025437
Webrev: http://cr.openjdk.java.net/~dxu/8025437/webrev.01/ 



-Dan 

Re: RFR:JDK-7102702 - java/net/PortUnreachableException/OneExceptionOnly.java failing

[Michael McMahon]

On 18/12/13 16:27, Alan Bateman wrote:

On 18/12/2013 16:19, Mark Sheppard wrote:

potentially the same issue with

TwoStacksPlainDatagramSocketImpl

for an IPV6 address.

as I have only ever enabled TwoStack via -Djava.net.preferIPv4Stack=true
then the sockaddr_in is sufficient to hold an IPV4 address

If we think it worthwhile,
I can make the equivalent change in TwoStack, and re-execute the 
regression suites.


Out of curiosity, is TwoStack* even needed nowadays? I would think 
that once Windows XP is gone that it shouldn't be needed.


-Alan



Nice catch with this bug all right. We probably should get rid of the 
TwoStack stuff at this stage

given XP's support status over JDK 9's timeframe.

Michael.

RFR: 8029354: URLPermission. throws llegalArgumentException: Invalid characters in hostname

[Michael McMahon]

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

This is the forward port to JDK 9. The patch from 8 applies cleanly.

Thanks,
Michael

Re: JDK 9 RFR of 8031326: Use Class rather than Class in java.net method signatures

[Michael McMahon]

On 07/01/14 17:44, Alan Bateman wrote:

On 07/01/2014 17:36, Joe Darcy wrote:

Hello,

Please review these changes to remove use of raw Class in some method 
signatures in java.net:


JDK-8031326: Use Class rather than Class in java.net 
method signatures


Patch below.

This looks okay to me.

-Alan.


I was thinking originally, there must be something more specific than 
Class that
applies to these types (related to the ContentHandler types). But, it 
looks like there isn't.

So, it looks ok to me too.

Michael.

Re: RFR: (8030875) Macros for checking and returning on exceptions

[Michael McMahon]

On 10/01/14 15:37, roger riggs wrote:

Please review:

To enable native code checking consistently for thrown exceptions,
the macros in net_util.h and java/util/jar/pack/coding.cpp are
made consolidated and promoted to jni_util.h

webrev:
http://cr.openjdk.java.net/~rriggs/webrev-check-exception-8030875/

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


Looks fine to me

Michael.

Re: RFR [9] 7150539: HttpURLConnection.getResponseMessage() doesn't throw IOException on server error (OS X)

[Michael McMahon]

Looks good!

Michael.

On 31/01/14 10:52, Chris Hegarty wrote:
This is a trivial test only change to make it agnostic of system proxy 
setting on OS X. The test requires that it makes a direct connection 
to the self contained trivial test HTTP server, it should not be 
influenced by system proxy settings.


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

--- a/test/sun/net/www/http/HttpClient/RetryPost.java
+++ b/test/sun/net/www/http/HttpClient/RetryPost.java
@@ -55,8 +55,8 @@
 void doClient() {
 try {
 InetSocketAddress address = httpServer.getAddress();
-URL url = new URL("http://"; + address.getHostName() + ":" 
+ address.getPort() + "/test/");
-HttpURLConnection uc = 
(HttpURLConnection)url.openConnection();
+URL url = new URL("http://localhost:"; + address.getPort() 
+ "/test/");
+HttpURLConnection uc = 
(HttpURLConnection)url.openConnection(Proxy.NO_PROXY);

 uc.setDoOutput(true);
 uc.setRequestMethod("POST");
 uc.getResponseCode();

-Chris.

RFR: 8033425 Delay loading of net library in PortConfig initialization (workaround for for 8033367)

[Michael McMahon]

Could I get this change reviewed for JDK 8 please?

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

The problem affects appletviewer, but it doesn't seem to
be reproducible with the applet support in jtreg. So,
hence there is no regression test.

Michael

RFR: 8028725: [Parfait] warnings from b116 for jdk.src.solaris.native.java.net: JNI pending exceptions

[Michael McMahon]

Adding some checks for pending exceptions in the InetAddress native code.

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

Thanks
Michael

Re: RFR [9] Inet[4|6]Address class and fieldID initialization in networking native code

[Michael McMahon]

Nice cleanup. Looks good, and thanks for catching the CHECK_NULL problem

Michael

On 04/02/14 16:12, Chris Hegarty wrote:

And a link to the webrev that can people can access:

 http://cr.openjdk.java.net/~chegar/nativeInetCleanup/webrev/

-Chris.

On 02/04/2014 04:01 PM, Chris Hegarty wrote:

Hi,

This change is mainly about clean up of the networking native code.
There are a number of places that cache JNI global references to the
Inet[4|6]Address classes, as well as caching their respective field IDs.
It is also difficult to correctly handle propagation of possible JNI
method invocation failures, from the (get|set)Inet[4|6]Address_XXX 
methods.


As it happens Inet[4|6]Address.c already makes these classes and field
IDs available [1] [2] [3]. It is then a small matter of ensuring that
they get initialized from any entry point that may use them, in the
various socket/InetAddress/network interface implementation, as well as
NIO and SCTP.

Webrev:

http://chhegar.ie.oracle.com/chhegar/repos/jdk9/dev/serial/jdk/nativeInetCleanup.01/webrev/ 




-Chris.

[1]
http://hg.openjdk.java.net/jdk9/dev/jdk/file/tip/src/share/native/java/net/InetAddress.c 



[2]
http://hg.openjdk.java.net/jdk9/dev/jdk/file/tip/src/share/native/java/net/Inet4Address.c 



[3]
http://hg.openjdk.java.net/jdk9/dev/jdk/file/tip/src/share/native/java/net/Inet6Address.c 

Re: RFR (XS) [9] PlainDatagramSocketImpl missing returns after “throwing an exception”

[Michael McMahon]

Looks good.

- Michael.

On 05/02/14 09:58, Chris Hegarty wrote:

Some very minor clean up in PlainDatagramSocketImpl to add some missing returns 
after “throwing an exception”, as well as checking for a pending exception 
after calling setTTL ( that can throw ).

http://cr.openjdk.java.net/~chegar/nativeInetCleanup/webrev.02/webrev/src/solaris/native/java/net/PlainDatagramSocketImpl.c.sdiff.html

-Chris.

P.S. This is not a comprehensive audit of this file, just some issues I noticed 
while refactoring some of he InetAddress fieldIDs yesterday.

Re: RFR for JDK-8031661 java/net/Authenticator/B4769350.java failed intermittently

[Michael McMahon]

That seems reasonable Amanda. I've run the updated test for a while and
I can't see any problem with it.

Thanks,
Michael

On 10/02/14 08:56, Amanda Jiang wrote:

Hi All,

Please review the fix for JDK-8031661

http://cr.openjdk.java.net/~tyan/amandaj/JDK-8031661/webrev.01/

Description:
Root Cause:
In line 195 and 196, the two actions can be ordered by a 
happens-before relationship, which will cause synchronization fail.

t1cond1latch.countDown();
t1cond2latch.await();

Suggested Fixes:
Using only one CyclicBarrier ("t1Cond1") sync c1,c2 and main thread

Draft JEP for Http client in JDK 9

[Michael McMahon]

Hi,

You might remember a new Http client API was proposed for JDK 8, but the 
work
was dropped from that project. So, we would like to return to the idea 
for JDK 9.

The following is the text of a draft JEP for the work. Comments welcome.

Thanks,
Michael


Title: New HTTP Client
Author: Michael McMahon

Summary
---
Define and implement a new Http client API to eventually replace the legacy
HttpURLConnection and which also implements HTTP 2.0 and websockets.

Motivation
--
Problems with the existing API and implementation:

+ URLConnection based API was designed with multiple protocols in mind,
  nearly all of which are defunct now (ftp, gopher, etc.)

+ predates HTTP 1.1 and is too abstract

+ hard to use (much behavior undocumented)

+ works in blocking mode only (one thread per request/response)

+ very hard to maintain

Goals
-
+ API must be easy to use for common cases including a simple blocking mode.

+ also need an asynchronous mode which decouples requests and responses
  and which allows for notification of events like "headers received", 
errors,

  "response body received"

+ modernize HTTP support (up to Http 2.0)

+ allow for alternative implementations

+ simple and concise API which caters for 80-90% of application needs

+ must expose all relevant aspects of the HTTP protocol request to a server
  and response from a server (headers, body, status codes etc)

+ must include standard and common authentication mechanisms

+ must be able to easily set up the web sockets handshake

+ must support HTTP 2.0 (application level semantics of 2.0 are mostly the
  same as 1.1, though wire protocol completely different)
   + ability to negotiate upgrade from 1.1 to 2.0 (or not), or select 2.0
 from start
   + server push (ability of server to push resources to client without 
being

 requested by client)

+ must perform security checks consistent with existing networking classes

+ should be friendly towards new language features like lambdas


Non goals
-
This work is targeted for JDK 9 only. It is not intended to be back 
portable
to previous releases. While this API is intended to replace the 
URLConnection
API for new code, it is not intended to immediately re-implement the old 
API

using the new API/implementation. This may happen as future work.

These are also some requirements which had been considered in JDK 8, but
in the interests of keeping the API as simple as possible, are being 
left out
for JDK 9. Some of these will assume less importance with the gradual 
adoption

of http 2.0 eg. connection caching because there is less requirement for
multiple TCP connections with Http 2.0.

+ request/response filtering
+ API for pluggable connection cache
+ general upgrade mechanism

Description
---
Some prototyping work was done during JDK 8 and separate classes were 
defined

for the http client, requests and responses. The builder pattern was used
to separate mutable entities from the immutable products. The prototype was
built on NIO AsynchronousSocketChannels and that API will be the 
starting point
for this work. It is expected that the API will undergo a number of 
iterations

before being finalised. The API/implementation was also standalone,
ie. existing stack will be left as is to ensure compatibility
and allow a phased approach (don't have to support everything at once).

The prototype API also included support for:

+ separate request/response (like servlet and http server API)

+ Asynchronous notification handlers will be defined for following events:
+ response headers received
+ response error
+ response body received
+ server push (2.0 only) means "here is a request for which the server
  will send a response"

+ https via SSLEngine
+ proxying
+ cookies
+ authentication


Alternatives


A number of existing Http client API and implementations exist: for example
Jetty (org.mortbay.jetty) and the Apache Http client.

Both of these are both rather heavy-weight in terms of numbers of packages
and classes and don't take advantage of newer language features like 
lambdas.


Risks and Assumptions
-
Https 2.0 support depends on TLS (Application Layer Negotiation Extension)
which is not currently supported in JDK.

The Http 2.0 spec itself is still in internet-draft form, but is 
expected to

be submitted as a draft standard Nov 2014.

API surface could grow if additional features added.

Impact
--

+ Other JDK components: no impact
+ Compatibility: no impact on existing API, but new tests required for 
new API

+ Security: will need careful audit
+ Portability: will be no native code in implementation
+ User Interface: no impact
+ Documentation: no impact
+ Internationalization: no impact
+ Localization: no impact
+ Legal: no impact
+ Other: no impact

Testing
---

Will need new tests. The internal http server will provide a suitable test
harness

Re: Add IDN support to existing net APIs

[Michael McMahon]

I think it's a good idea. Before changing anything though,
we would need to:

1. identify all APIs that could potentially be affected and figure out
   what is
   needed for each. For instance:
1. InetAddress
2. SocketPermission
3. URLPermission
4. HttpURLConnection
5. URL/URI
6. ...

2. understand how it relates to URIs and IRIs. I haven't looked into
   this much
   but it seems that there might be potential for confusion given that
   these entities
   have their own encoding schemes already. I'm sure the issues are all
   well thrashed out
   already, but I'd like to see exactly how it will work in Java before
   we start the work

Maybe, the work is within the scope of a small JEP, if nothing else to 
define the scope

of the work.

Michael.

On 19/02/14 04:00, Jonathan Lu wrote:

Hi net-dev,

If a Java application tries to support International Domain Names 
(IDN) [1], which was firstly brought in Java6,
 it has to write additional code or wrap java.net.IDN API [2] to make 
the conversion each time it tries to resolve
a non-ASCII domain name, and use the converted punycode to make 
connections to remote host, like:


java.net.IDN.toASCII("电脑.info ");

It is easier for newly writen applications, since a wrappers can be 
made in early design stage, but if
it comes to existing Java applications and third-party libraries, IDN 
support would involve much more effort
like modifying the existing code base, inspecting libraries 
interfaces/implementations, or even

re-implement some of the functions.

I'm wondering if it is possible to add IDN support into existing Java 
APIs, like
java.net.InetAddress.getByName(), if JDK itself supports IDN, any 
existing applications or libraries would
benefit from supporting IDN automatically without any source code 
modifications.


Of course there's security risks regarding IDN homograph attacks 
[1][2], so it may not be a
good idea to change the default behavior right now. But it would still 
work if a simple switch can be

added into current JDK.
For example, by defining following the property in command like 
options like


-Djava.net.AutoIDN=true

Any existing Java applications who wishes to support IDN feature will 
be able to support IDN

without any changes to its source code or re-compilation.

What's your opinion on this ? any comment is welcome.

Thank you

- Jonathan Lu

---

[1] http://en.wikipedia.org/wiki/Internationalized_domain_name
[2] http://download.java.net/jdk8/docs/api/java/net/IDN.html
[3] http://www.unicode.org/reports/tr36/
[4] http://en.wikipedia.org/wiki/IDN_homograph_attack

Re: Add IDN support to existing net APIs

[Michael McMahon]

On 20/02/14 03:15, Jonathan Lu wrote:


Hi Michael,

Thanks a lot for your comments.

On Wed, Feb 19, 2014 at 6:28 PM, Michael McMahon 
mailto:michael.x.mcma...@oracle.com>> 
wrote:


I think it's a good idea. Before changing anything though,
we would need to:

 1. identify all APIs that could potentially be affected and
figure out what is
needed for each. For instance:
 1. InetAddress
 2. SocketPermission
 3. URLPermission
 4. HttpURLConnection
 5. URL/URI
 6. ...

Right, I assume all  APIs which accept string-represented host names 
would be affected.

It that may not be a big change from point of view.

1.


 1. understand how it relates to URIs and IRIs. I haven't looked
into this much
but it seems that there might be potential for confusion given
that these entities
have their own encoding schemes already. I'm sure the issues
are all well thrashed out
already, but I'd like to see exactly how it will work in Java
before we start the work

For the potential confusion, were you meaning a confusion between URI 
encoding (RFC-2396 [1]) and IDN ?


Right. As I said, it may just be a matter of checking RFC2396 (or its 
successor RFC 3986).

I think RFC 3986 references IDNs.

Michael


Maybe, the work is within the scope of a small JEP, if nothing
else to define the scope
of the work.

Michael.


Copying core-libs-dev and security-dev to get more comments.

Many thanks

- Jonathan

[1] http://www.ietf.org/rfc/rfc2396.txt



On 19/02/14 04:00, Jonathan Lu wrote:

Hi net-dev,

If a Java application tries to support International Domain Names
(IDN) [1], which was firstly brought in Java6,
 it has to write additional code or wrap java.net.IDN API [2] to
make the conversion each time it tries to resolve
a non-ASCII domain name, and use the converted punycode to make
connections to remote host, like:

java.net.IDN.toASCII("电 脑.info <http://xn--wnyy6w.info>");

It is easier for newly writen applications, since a wrappers can
be made in early design stage, but if
it comes to existing Java applications and third-party libraries,
IDN support would involve much more effort
like modifying the existing code base, inspecting libraries
interfaces/implementations, or even
re-implement some of the functions.

I'm wondering if it is possible to add IDN support into existing
Java APIs, like
java.net.InetAddress.getByName(), if JDK itself supports IDN, any
existing applications or libraries would
benefit from supporting IDN automatically without any source code
modifications.

Of course there's security risks regarding IDN homograph attacks
[1][2], so it may not be a
good idea to change the default behavior right now. But it would
still work if a simple switch can be
added into current JDK.
For example, by defining following the property in command like
options like

-Djava.net.AutoIDN=true

Any existing Java applications who wishes to support IDN feature
will be able to support IDN
without any changes to its source code or re-compilation.

What's your opinion on this ? any comment is welcome.

Thank you

- Jonathan Lu

---

[1] http://en.wikipedia.org/wiki/Internationalized_domain_name
[2] http://download.java.net/jdk8/docs/api/java/net/IDN.html
[3] http://www.unicode.org/reports/tr36/
[4] http://en.wikipedia.org/wiki/IDN_homograph_attack

Re: RFR [9]: 8034174 Remove use of JVM_* functions from java.net code

[Michael McMahon]

On 23/02/14 08:55, Chris Hegarty wrote:

On 22 Feb 2014, at 17:23, Dmitry Samersoff  wrote:


Chris,

Didn't look to windows part. Unix part looks good for me. See also below.

I'm a bit concerned because of mixing NET_* abstractions and direct call
to OS functions. It might be better to create NET_socket etc.

Me too. It is already a mess. System calls should be used directly, unless 
there is a reason not to do so.


We use NET_GetSockOpt/NET_SetSockOpt in one places and plain os
functions in other ones it have to be unified.

If there is no reason to call the NET_ variant, then the system call should be 
used.


Seems like the big #ifdef in net_util_md.h on this is more or less 
redundant now

since the #define of NET_xxx to JVM_xxx was its only purpose.

I wonder would it also be useful to expand the comment just above those 
definitions
that currently just relates to AIX and say which other operating systems 
it applies to
and if we could identify which system calls it affects, and which mean 
the NET_xx
functions must be used. Or maybe this is going beyond what you wanted to 
do here?


Michael

Re: RFR [9]: 8034174 Remove use of JVM_* functions from java.net code

[Michael McMahon]

On 24/02/14 14:09, Chris Hegarty wrote:

On 24/02/14 10:42, Michael McMahon wrote:

On 23/02/14 08:55, Chris Hegarty wrote:

On 22 Feb 2014, at 17:23, Dmitry Samersoff
 wrote:


Chris,

Didn't look to windows part. Unix part looks good for me. See also
below.

I'm a bit concerned because of mixing NET_* abstractions and direct 
call

to OS functions. It might be better to create NET_socket etc.

Me too. It is already a mess. System calls should be used directly,
unless there is a reason not to do so.


We use NET_GetSockOpt/NET_SetSockOpt in one places and plain os
functions in other ones it have to be unified.

If there is no reason to call the NET_ variant, then the system call
should be used.


Seems like the big #ifdef in net_util_md.h on this is more or less
redundant now
since the #define of NET_xxx to JVM_xxx was its only purpose.


The only difference between these now is that the bsd/linux variant 
are defined in a separate file ( extern ), bsd_close/linux_close. I'm 
not sure, but I think the use of extern is still required here.




I think extern would be okay in the other case though. All C functions 
are extern unless

declared static afaik.


I wonder would it also be useful to expand the comment just above those
definitions
that currently just relates to AIX and say which other operating systems
it applies to
and if we could identify which system calls it affects, and which mean
the NET_xx
functions must be used. Or maybe this is going beyond what you wanted to
do here?


Beyond ;-) There is still a lot of cleanup that I want to make to this 
code, but I'd like to do it incrementally, starting with breaking the 
dependency on the VM interface. This makes it easier, certainly from a 
review point of view.


-Chris.



Michael

RFR: 8035653: InetAddress.getLocalHost crash

[Michael McMahon]

Could I get the following change reviewed please?

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

We overlooked one place where JNI native field initialization
is required in Windows Vista+

Thanks
Michael.

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

[Michael McMahon]

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 [9] 8035897 : FD_SETSIZE should be set on macosx

[Michael McMahon]

On 28/02/14 14:40, Chris Hegarty wrote:
[ Volker: there are some trivial AIX changes here, maybe you could 
verify them? ]


JDK-8021820 adds -D_DARWIN_UNLIMITED_SELECT to the build, but the 
fd_set struct is still limited to 1024.


Snippet from man select(2):

   "Although the provision of getdtablesize(2) was intended to allow 
user programs to be written
 independent of the kernel limit on the number of open files, the 
dimension of a sufficiently
 large bit field for select remains a problem. The default size 
FD_SETSIZE (currently 1024) is
 somewhat smaller than the current kernel limit to the number of 
open files. However, in order
 to accommodate programs which might potentially use a larger 
number of open files with select,
 it is possible to increase this size within a program by 
providing a larger definition of

 FD_SETSIZE before the inclusion of .

Either:
 1) FD_SETSIZE needs to be set to a larger value, but what value, the
kernel limit, or other? This is wasteful for most typical apps that
don't use large numbers of file descriptors. Or,
 2) If fd is greater than 1024, then an appropriate amount of memory
could be allocated and cast to an fd_set. The FD_SET macro will
write past FD_SETSIZE.

I think option 2 is preferable:
 http://cr.openjdk.java.net/~chegar/8035897/webrev.00/webrev/

I'm still checking to see it an automatic regression test is possible, 
but I wanted to circulate the changes for comment first.


Thanks,
-Chris.


I agree option 2 sounds better since adding (say) 4k * sizeof(fd_set) to 
the stack is quite

significant given that it would rarely be required.

On the change itself I noticed one path in bsd_close.c NET_Timeout where 
the malloc'd buffer

is not freed (line 418)

Is it potentially more efficient to use calloc() rather than malloc() + 
memset()?


Maybe a test might need a shell script to call "ulimit -n 4096" or 
something to be able to test it.


Michael

Re: RFR JDK-8035158: Remove dependency on sun.misc.RegexpPool and friends

[Michael McMahon]

On 21/03/14 09:58, Chris Hegarty wrote:
Thanks for doing this Pavel. The changes, and test, look good to me. I 
can sponsor this for you.


-Chris.



+1. Looks good here too.

Michael


On 19/03/14 18:03, Pavel Rappo wrote:

Hi everyone,

could you please review my change for JDK-8035158?

DefaultProxySelector uses sun.misc.RegexpPool to parse properties that
configure the proxy settings. This code should be changed to use
java.util.regex public API so that the classes in sun.misc.Regex* can be
removed.

http://cr.openjdk.java.net/~chegar/8035158/webrev.00/

The main idea was to get rid of dependency on private regex engine and
to use public API which is available since jdk4 -- 
java.util.regex.Pattern.


Thanks
Pavel

P.S. During this refactoring I may have found an erroneous behavior. In
the current implementation when a duplicate disjunct is added into
http.nonProxyHosts (as well as into ftp.nonProxyHosts) property,
sun.misc.RegexpPool throws an exception:

throw new java.misc.REException(re + " is a duplicate");

Later while slicing pattern into a sequence of disjuncts
sun.net.spi.DefaultProxySelector silently swallows this exception:

StringTokenizer st = new StringTokenizer(nphosts, "|", false);
try {
 while (st.hasMoreTokens()) {
 pool.add(st.nextToken().toLowerCase(), Boolean.TRUE);
 }
} catch (sun.misc.REException ex) {} // ***

That leads to a situation where resulting pattern is broken. Moreover
the final result depends on a position of the duplicate in the property
-- as the while loop abrupted as a whole, not just a single iteration.

In my case I was testing with http.nonProxyHosts=localhost when noticed
this issue. In the list of default loopback aliases localhost happens to
sit first:

String defStringVal = "localhost|127.*|[::1]|0.0.0.0|[::0];

So when you set a value of http.nonProxyHosts to a "localhost", the only
host you'll be able to go directly to will be that localhost. Because
nothing else will have chance to be added into the pool after the
exception is thrown.

That's the only behavior I've modified by now.

Re: RFR: JDK-8035631 - JNI exception pending in jdk/src/windows/native/java/net/NetworkInterface_winXP.c

[Michael McMahon]

Hi Mark,

I think in the normal case, the memory is freed by the calling
function. It looks like the other cases aren't consistent though.

Michael

On 21/03/14 14:04, Mark Sheppard wrote:

Hi Chris,
   thanks for the review ... yes, the question is intentional.
the freeing of netaddrP is inconsistent on the NULL returns, so I just 
flagged it to

solicit opinion from those more familiar with this code, to
see if netaddrP should be freed prior to return ... L555 is another 
case as you have pointed out.

netaddrP is obtained from the ifs in parameter.
Interestingly, a "normal" return doesn't seem to free netaddrP.
so its a minor conundrum

regards
Mark


On 21/03/2014 13:43, Chris Hegarty wrote:

This looks ok to me Mark.

You have added a question/comment on L514. Is this intentional?

L555. Not directly related to your changes, but should netaddrP be 
freed there before returning NULL?


-Chris.

On 14/03/14 19:04, Mark Sheppard wrote:

Hi

   Please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8035631/webrev/

which address the issues raised in
https://bugs.openjdk.java.net/browse/JDK-8035631

Summary:
added CHECK_NULL_RETURN after JNI calls

regards
Mark

Re: Review Request of JDK Enhancement Proposal: DTLS

[Michael McMahon]

My understanding is that the original PMTU discovery spec RFC 1191
is not very effective due to its reliance on ICMP messages that are often
filtered out by routers. There was an update in RFC 4821 which removes
the dependency on ICMP and that seems to be effective

I'm just wondering then how DTLS expects it to be done?

Michael.


On 22/03/14 06:48, Matthew Hall wrote:

The following bug and source code [1] are present in OpenSSL:

http://rt.openssl.org/Ticket/Display.html?id=1929

I think something similar could be done in the JDK, by making some tweaks to
the SocketOptions classes to expose some more options. I'm pretty sure
something relating to this will work in POSIX JDK for Linux, Solaris, OS X,
but not sure what different magic would be needed for Windows.

Then there could be some kind of way to generate the right discovery
datagrams, figure out the result, and feed it to the DTLSEngine for
packetization purposes.

Also, there is some DTLS capability in Bouncy Castle, we could see what they
allow in terms of packetization as well, though I doubt they'll have what
OpenSSL has, since IP_MTU_DISCOVER is probably not available to them either.

I am glad you guys are working on this... I already have some use cases in
mind for it! :-D

Matthew.

Reference [1]:

 case BIO_CTRL_DGRAM_MTU_DISCOVER:
#if defined(OPENSSL_SYS_LINUX) && defined(IP_MTU_DISCOVER) && 
defined(IP_PMTUDISC_DO)
 addr_len = (socklen_t)sizeof(addr);
 memset((void *)&addr, 0, sizeof(addr));
 if (getsockname(b->num, &addr.sa, &addr_len) < 0)
 {
 ret = 0;
 break;
 }
 switch (addr.sa.sa_family)
 {
 case AF_INET:
 sockopt_val = IP_PMTUDISC_DO;
 if ((ret = setsockopt(b->num, IPPROTO_IP, 
IP_MTU_DISCOVER,
 &sockopt_val, sizeof(sockopt_val))) < 0)
 perror("setsockopt");
 break;
#if OPENSSL_USE_IPV6 && defined(IPV6_MTU_DISCOVER) && defined(IPV6_PMTUDISC_DO)
 case AF_INET6:
 sockopt_val = IPV6_PMTUDISC_DO;
 if ((ret = setsockopt(b->num, IPPROTO_IPV6, 
IPV6_MTU_DISCOVER,
 &sockopt_val, sizeof(sockopt_val))) < 0)
 perror("setsockopt");
 break;
#endif


On Fri, Mar 21, 2014 at 07:46:22PM -0400, Christos Zoulas wrote:

On Mar 22,  7:38am, xuelei@oracle.com (Xuelei Fan) wrote:
-- Subject: Re: Review Request of JDK Enhancement Proposal: DTLS

| Networking experts, any suggestion?

I have not seen pmtu exposed at the application layer before. Has anyone
else?

christos

|
| Xuelei
|
| On 3/21/2014 8:28 AM, Matthew Hall wrote:
| > On Fri, Mar 21, 2014 at 06:58:50AM +0800, Xuelei Fan wrote:
| >> here. Although MTU is not PMTU, but it is normally "correct".
| >
| > I would state, not "normally correct", but "frequently correct".
| >
| > In case of IPSEC, SSL VPN, IPv6, GRE, etc. this will not be true. Many of
| > these are used for Site-to-Site VPN, which will appear often in the context 
of
| > RTP packets and SRTP packets, which happen to travel over VPNs.
| >
| >> It would be great if there is PMTU discovery API in Java, which can
| >> simplify the implementation of DTLS.
| >
| > Without it, I think there will be a lot of odd bugs occurring.
| >
| > Matthew.
| >
|
|
| --090406030702020009070402
| Content-Type: message/rfc822;
|  name="Attached Message"
| Content-Transfer-Encoding: 7bit
| Content-Disposition: attachment;
|  filename="Attached Message"
|
| Message-ID: <532a3b53.6000...@oracle.com>
| Date: Thu, 20 Mar 2014 08:50:27 +0800
| From: Xuelei Fan 
| User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 
Thunderbird/24.3.0
| MIME-Version: 1.0
| To: Matthew Hall 
| CC: OpenJDK 
| Subject: Re: Review Request of JDK Enhancement Proposal: DTLS
| References: <532a25ea.7040...@oracle.com> 
<20140320003158.ga5...@mhcomputing.net>
| In-Reply-To: <20140320003158.ga5...@mhcomputing.net>
| Content-Type: text/plain; charset=ISO-8859-1
| Content-Transfer-Encoding: 7bit
|
| PMTU is a key point of the design.  I was wondering to expose this
| application layer as a configurable parameter.  If it is too big (or not
| configured), DTLSEngine(let call it temporarily) will downgrade the size
| automatically, just as the previous messages get lost.
|
| It's good point that need a separate spec to determine the PMTU. I will
| see what we can do here.
|
| Thanks,
| Xuelei
|
| On 3/20/2014 8:31 AM, Matthew Hall wrote:
| > Xuelei,
| >
| > Is there an existing method for determining valid PMTU from inside of Java? 
If
| > not then supplying correct segment size to whatever DTLSEngine (or however
| > it's named) class would be non-trivial and could 

Re: Add IDN support to existing net APIs

[Michael McMahon]

On 26/03/14 08:43, Alan Bateman wrote:

On 26/03/2014 02:58, Jonathan Lu wrote:

:

Only for the system property to configure the use of IDNs and some 
related code to do the checking and conversion, no plan to add new 
methods to InetAddress.


Thanks, I'm just wondering whether it would make sense to add APIs. 
Michael - do you have any views on this?


-Alan


I think perhaps a net property (as opposed to a system property) would 
be sufficient. An explicit API would
probably need to be at a similar (global) scope. In other words, you 
would want all uses of Strings as hostnames
to be converted to/from IDNs transparently and without needing 
additional API invocations.
So, may be a property is just simpler. I assume the default initially 
would be for the property to be

disabled by default?

I know I raised the idea of filing a JEP for this, but maybe that's not 
necessary. If you are interested
to do this work Jonathan, then I'd suggest maybe doing a survey first, 
to identify the relevant specifications (eg RFCs)
that need to be complied with and then identify the places that we need 
to make changes (or a webrev

of the changes if you like). And we could take it from there.

Michael.

Re: RFR JDK-8037396: URI getQuery() and getFragment() don't decode properly

[Michael McMahon]

In my view, it's the current behavior that is surprising. You would 
really expect
those methods to return the un-encoded strings. It's odd we haven't come 
across
this issue before now, which probably indicates use of '[]' in those 
components

is uncommon.

Michael

On 04/04/14 16:01, Chris Hegarty wrote:

Pavel,

The code changes and test update look good to me.

I think I agree with the approach here, but just to clarify the change 
in behavior, that will be visible after the changes.


$ cat Test.java
public class Test {
public static void main(String[] args) throws Exception {
 java.net.URI u = new java.net.URI("http", "example.org", "/a 
b[c d]", "a b[c d]", "a b[c d]");

 System.out.println("path:" + u.getPath());
 System.out.println("path:" + u.getQuery());
 System.out.println("path:" + u.getFragment());
}
}

-- without fix --
$ java Test
path:/a b[c d]
path:a b[c%20d]
path:a b[c%20d]

-- with fix ---
$ java Test
path:/a b[c d]
path:a b[c d]
path:a b[c d]

So the question now is; Will this surprise anyone? possibly embedding 
URLs/URIs in query strings? I expect not, but just want to spell out 
the change in behavior so that we can make an informed decision.


-Chris.


On 04/04/14 15:45, Pavel Rappo wrote:

Hi everyone,

could you please review my change for JDK-8037396: 
http://cr.openjdk.java.net/~michaelm/8037396/webrev/


As per API, getQuery() and getFragment() should return the decoded 
string. But this seems not to work properly if there are squared 
brackets around them.


The result of analysis can be seen here:

https://bugs.openjdk.java.net/browse/JDK-8037396?focusedCommentId=13479678&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13479678 



I think we should go the least intrusive way here (excluding 'do 
nothing' option).


Thanks
-Pavel

RFR: 8036979: Support java.net.SocketOption<> in java.net socket types

[Michael McMahon]

Could I get the following reviewed please?

In addition to providing generic support for java.net.SocketOption,
it also includes 8032808, which implements a platform specific
socket option SO_FLOW_SLA (in jdk.net)

There are changes to two repos:

http://cr.openjdk.java.net/~michaelm/8036979/jdk/01/

and

http://cr.openjdk.java.net/~michaelm/8036979/top/01/webrev/

Thanks
Michael

Re: RFR [9] 8039470:URLConnection.getContent incorrectly specifies the default location of content handler classes

[Michael McMahon]

Is there potential for confusion there?  really just
means an "implementation defined" package rather than the Java 
language's "default package"?


Michael

On 08/04/14 20:03, Chris Hegarty wrote:

java.net.URLConnection.getContent() incorrectly specifies the default location 
of content handler classes as sun.net.www.content. ( this location is 
implementation specific )

   "If no content handler factory has yet been set up, or if the factory's 
createContentHandler method
 returns null, then the application loads the class named:
  sun.net.www.content. "

This should be changed to something like:
 ., similar to what is done in URL [1]
   
Trivial specDiff:

   http://cr.openjdk.java.net/~chegar/8039470/URLConnection-report.html

-Chris.

[1] 
http://docs.oracle.com/javase/8/docs/api/java/net/URL.html#URL-java.lang.String-java.lang.String-int-java.lang.String-

Re: RFR [9] 8039470:URLConnection.getContent incorrectly specifies the default location of content handler classes

[Michael McMahon]

Chris,

Okay, I think it's fine then. The term "default package" is used, but I 
accept

it's not referred to as such, in the JLS.

Thanks,
Michael

On 09/04/14 12:42, Chris Hegarty wrote:

Thanks for looking at this Michael,

On 09/04/14 11:51, Michael McMahon wrote:

Is there potential for confusion there?  really
just


I just took the wording from the URL spec. I guess it was agreed upon 
at some point.



means an "implementation defined" package rather than the Java
language's "default package"?


Just on this, there is no such term as "default package" in the JLS, 
it is the "unnamed package". But I accept that this is probably a 
subtle difference that may not be widely known.


I'm happy with the wording as is, but if you think we should change it 
then we should probably change URL too.


-Chris.




Michael

On 08/04/14 20:03, Chris Hegarty wrote:

java.net.URLConnection.getContent() incorrectly specifies the default
location of content handler classes as sun.net.www.content. ( this
location is implementation specific )

   "If no content handler factory has yet been set up, or if the
factory's createContentHandler method
 returns null, then the application loads the class named:
  sun.net.www.content.<contentType> "

This should be changed to something like:
 ., similar to what is done
in URL [1]
Trivial specDiff:
http://cr.openjdk.java.net/~chegar/8039470/URLConnection-report.html

-Chris.

[1]
http://docs.oracle.com/javase/8/docs/api/java/net/URL.html#URL-java.lang.String-java.lang.String-int-java.lang.String- 

Re: RFR: 8036979: Support java.net.SocketOption<> in java.net socket types

[Michael McMahon]

Thanks Chris.

Michael.

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

This is a nice addition, and looks very good to me.

Just a few small comments:

1) SocketImpl getOption/setOption need javadoc to describe how they can
   throw IOException. Maybe even the same, of similar javdoc to the
   socket type equivalent methods?

2) New protected methods in SocketImpl need an @since tag.

-Chris.

On 08/04/14 18:49, Michael McMahon wrote:

Could I get the following reviewed please?

In addition to providing generic support for java.net.SocketOption,
it also includes 8032808, which implements a platform specific
socket option SO_FLOW_SLA (in jdk.net)

There are changes to two repos:

http://cr.openjdk.java.net/~michaelm/8036979/jdk/01/

and

http://cr.openjdk.java.net/~michaelm/8036979/top/01/webrev/

Thanks
Michael

Re: RFR: 8036979: Support java.net.SocketOption<> in java.net socket types

[Michael McMahon]

Thanks Alan. All comments accepted, except as clarified below.

Michael

On 09/04/14 15:07, Alan Bateman wrote:

On 08/04/2014 18:49, Michael McMahon wrote:

Could I get the following reviewed please?

In addition to providing generic support for java.net.SocketOption,
it also includes 8032808, which implements a platform specific
socket option SO_FLOW_SLA (in jdk.net)

There are changes to two repos:

http://cr.openjdk.java.net/~michaelm/8036979/jdk/01/

I've looked through this webrev.

First I think it is great to see setOption/getOption added to the 
java.net socket types, this aligns it with we did in java.nio.channels 
in JDK 7 and makes it extensible as you've done with SO_FLOW_SLA.


A minor comment when reading through the java sources is that the 
formatting is a bit odd in places. The try-with-resources in 
OptionTest is one case. Another common one is the starting brace on 
its own line for cases where it would look much better on the previous 
line.


The javadoc looks good. A minor typo with "{{" in the package-info. 
Also in the javadoc for some of the enum member in SocketFlow.Status 
start in lowercase when I assume they should be capital.


You should double check that @since is added to the new classes. Looks 
to be missing from NetworkPermission and SocketFlow.Status at least.


I note that supportedOptions uses the class object for 
synchronization, it's probably okay initially but it creates the 
potential for VM-wide contention and may need to be looked at in the 
future.


Is the 2-arg NetworkPermission required? Maybe this is there just for 
consistency?




I notice that all permission types that extend BasicPermission have one. 
Maybe some types of Policy

object expect it, though strictly it shouldn't be necessary.

I assume that java.net.Sockets does not need to load libnet, it's 
already loaded by the class that defines the native methods.


A minor comment on the name setSecurityCheck, I wonder if 
checkSetOptionPermission might be a better name for this method.


Sockets/Test.java - L51, did you mean to check the result?

in this case no. It acts as a regression check for a bug that was 
triggered by invoking the method

prior to doing any other operation with sockets.

Re: RFR [9] 8039470:URLConnection.getContent incorrectly specifies the default location of content handler classes

[Michael McMahon]

Looks fine to me Chris.

- Michael

On 09/04/14 18:21, Chris Hegarty wrote:

Michael, Alan

There is/was an additional reference to the implementation specific 
package name in ContentHandler. It is now removed. I think this is the 
last one!


Latest specdiff:

http://cr.openjdk.java.net/~chegar/8039470/02/java/net/package-summary.html 



On 09/04/14 14:33, Alan Bateman wrote:

On 09/04/2014 14:13, Chris Hegarty wrote:

Flip-flop!

Thinking again about this, you're right.

http://cr.openjdk.java.net/~chegar/8039470/URLConnection-report.html
http://cr.openjdk.java.net/~chegar/8039470/URLConnection-report_01.html

I could go further with this change, but I just want to remove the
reference to the sun.* package, that should not be in the public API.


Would it make sense to just remove the bulk of step 2 from the javadoc
and just specify that it uses a built-in implementation default if is
exists for the content type?


Step 2 is important for user defined handlers, but it may not be 
correctly placed. I moved it to ContentHandler


-Chris.



-Alan

[8u-dev] RFR: 8036979: Support java.net.SocketOption<> in java.net socket types

[Michael McMahon]

Hi,

This is the webrev for the 8u20 version of the fix that was reviewed 
yesterday for 9.


JDK
===
http://cr.openjdk.java.net/~michaelm/8036979.8u20/jdk/01/webrev/

Top repo
=
http://cr.openjdk.java.net/~michaelm/8036979.8u20/top/01/webrev/

The good news is that the change is almost the same as the JDK 9 version
with the following differences:

1) The java.net public API changes are gone. The new public methods for 9
 in SocketImpl and DatagramSocketImpl are package private here.

2) A new package private class java.net.SocketsUtil acts as a bridge between
the public API in jdk.net.Sockets and the implementation in java.net

3) jdk.net.Sockets uses reflection to access the methods of 
java.net.SocketsUtil


4) The test of the public java.net API is gone and the other test augmented
with some additional tests for the standard socket options

Thanks,
Michael

Re: [8u-dev] RFR: 8036979: Support java.net.SocketOption<> in java.net socket types

[Michael McMahon]

On 11/04/14 10:27, Chris Hegarty wrote:

This looks mainly good to me.

Just a few small comments (mainly on the use of reflection):

1) jdk/net/Sockets.java
   You could use a SharedSecret to access the private methods in the
   public java.net package, but reflection is ok too.



I think the amount of reflection glue is not too much. So, I'd prefer
to stick with that in this instance.


2) If you stick with reflection then if a lookup of any of the methods
   fails, throws ReflectiveOperationException, then you should probably
   throw InternalError as there is something badly wrong.



okay, good idea.


3) Would it make sense for the invoker static methods of
   jdk.net.Sockets to throw UncheckedIOException(e) ??


I'd prefer to throw IOException when it is an IOException.
I'm open to suggestion for when it's not an IOException.
I know RuntimeException is vague, but then again UncheckedIOException
doesn't seem right either to me, since it's something other
than an io exception in this situation.

Thanks,
Michael


-Chris.

On 10/04/14 18:13, Michael McMahon wrote:

Hi,

This is the webrev for the 8u20 version of the fix that was reviewed
yesterday for 9.

JDK
===
http://cr.openjdk.java.net/~michaelm/8036979.8u20/jdk/01/webrev/

Top repo
=
http://cr.openjdk.java.net/~michaelm/8036979.8u20/top/01/webrev/

The good news is that the change is almost the same as the JDK 9 version
with the following differences:

1) The java.net public API changes are gone. The new public methods 
for 9

  in SocketImpl and DatagramSocketImpl are package private here.

2) A new package private class java.net.SocketsUtil acts as a bridge
between
 the public API in jdk.net.Sockets and the implementation in 
java.net


3) jdk.net.Sockets uses reflection to access the methods of
java.net.SocketsUtil

4) The test of the public java.net API is gone and the other test 
augmented

 with some additional tests for the standard socket options

Thanks,
Michael

Re: [8u-dev] RFR: 8036979: Support java.net.SocketOption<> in java.net socket types

[Michael McMahon]

Thanks Alan. Points taken. I'll add the lambda to the 9 version also.

Michael

On 11/04/14 16:04, Alan Bateman wrote:


Reflection or shared secrets is a coin toss here, I think what you 
have is okay. An alternative name for the package private class in 
java.net is SocketSecrets, just a suggestion as SocketsUtil sounds 
more general that it is.


I agree with Chris on throwing InternalError if any of the reflection 
code fails as that would indicate a bug or build issue somewhere.


jdk.net.Sockets mostly look good. I guess I would avoid _ in variable 
names but they are private so minor. Some of line lengths are a bit 
long in the method declaration. Not a big deal but they might have 
future side-by-side reviews harder.


Looks like "{" has ended up on the wrong line in a few places, 
NetworkPermission for example, a few other classes too.


ExtendedOptionsImpl. could use a lambda I assume.

ExtendedOptionsImpl.c L95 - alignment?

I don't have any other comments beyond the comments on the 9 version.

-Alan

Re: RFR [9] 8040837: Avoid provoking NFEs when initializing InetAddrCachePolicy

[Michael McMahon]

I think it would be an improvement to combine these doPrivileged() blocks
as suggested, though your patch needs work Bernd. For instance,
the multi-catch doesn't work. Also the PrivilegedAction<> type is wrong.

If someone wants to update it, then we can use that. Otherwise, we'll
go with the original suggested change.

Thanks
Michael

On 17/04/14 21:28, Bernd Eckenfels wrote:

Am Thu, 17 Apr 2014 21:50:23 +0200
schrieb Bernd Eckenfels :


Hello,

I would propose to use Integer.valueOf(tmp) instead, but looking at
the context I think it is even better to skip this and the following
null check with Integer.parseInt().

This is even shorter and it reduces the privileged actions invocations:

Integer tmp = java.security.AccessController.doPrivileged (
new PrivilegedAction() {
public String run() {
try {
String tmpString = Security.getProperty(cachePolicyProp);
   if (tmpString != null) return Integer.valueOf(tmpString);
} catch (NumberFormatException | IllegalArgumentException 
ignored) { }

try {
return Integer.getInteger(cachePolicyPropFallback);
} catch (NumberFormatException | IllegalArgumentException 
ignored) { }

return null;
}
});

 if (tmp != null) {
 cachePolicy = tmp.intValue();
 if (cachePolicy < 0) {
 cachePolicy = FOREVER;
 }
 propertySet = true;
 } else {
 /* No properties defined for positive caching. If there is no
  * security manager then use the default positive cache value. */
 if (System.getSecurityManager() == null) {
 cachePolicy = DEFAULT_POSITIVE;
 }
 }

NB: this will

RFR: 8041397: Lint regression in java.net.SocketOption

[Michael McMahon]

Could I get the following small change reviewed please?

It fixes a number of recently introduced lint warnings

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

Thanks
Michael.

RFR: 8041621: java/net/Inet4Address/textToNumericFormat.java fails on Solaris and Mac

[Michael McMahon]

This fixes a recent test failure caused by the change 
https://bugs.openjdk.java.net/browse/JDK-8040747
which added some new tests of the literal IP address parsing code. The 
new tests are valid with respect
to the parser, but were failing on some platforms because when the 
parser returns an error, the implementation
must defer to the OS resolver, which can have unpredictable results. The 
solution is to plug in a dummy
name service which throws an exception any time it is called (which is 
what the test expects)


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

Thanks,
Michael

RFR: 8034170: Digest authentication interop issue

[Michael McMahon]

Could I get the following change reviewed please for JDK 9
(same change will apply later to 8u-dev)?

In JDK 8 we fixed a problem with Digest authentication
where some parameters were being enclosed in double quotes
incorrectly. It seems some old versions of Tomcat expect this
behavior. So, the fix is to provide a flag that restores the old
behavior. The (qop and algorithm parameters are affected)

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

Thanks
Michael

RFR: 8043118: update test/java/net/URLPermission/nstest/lookup.sh to use explicit @build target

[Michael McMahon]

Could I get the following small test case change reviewed please?

This fixes a sporadic test case failure due to incorrect use of @build tag
in the script

--- a/test/java/net/URLPermission/nstest/lookup.sh
+++ b/test/java/net/URLPermission/nstest/lookup.sh
@@ -26,7 +26,7 @@
 # @library /lib/testlibrary
 # @compile -XDignore.symbol.file=true SimpleNameService.java
 #LookupTest.java SimpleNameServiceDescriptor.java
-# @build jdk.testlibrary.*
+# @build jdk.testlibrary.Utils
 # @run shell/timeout=50 lookup.sh
 #

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

Thanks
Michae

Re: RFR: 8043118: update test/java/net/URLPermission/nstest/lookup.sh to use explicit @build target

[Michael McMahon]

On 14/05/14 11:39, Alan Bateman wrote:

On 14/05/2014 11:35, Michael McMahon wrote:

Could I get the following small test case change reviewed please?

This fixes a sporadic test case failure due to incorrect use of 
@build tag

in the script

--- a/test/java/net/URLPermission/nstest/lookup.sh
+++ b/test/java/net/URLPermission/nstest/lookup.sh
@@ -26,7 +26,7 @@
 # @library /lib/testlibrary
 # @compile -XDignore.symbol.file=true SimpleNameService.java
 #LookupTest.java SimpleNameServiceDescriptor.java
-# @build jdk.testlibrary.*
+# @build jdk.testlibrary.Utils
 # @run shell/timeout=50 lookup.sh
 #

http://cr.openjdk.java.net/~michaelm/8043118/webrev.1/
I think Katja changed this test very recently to use the wildcard. Is 
your jtreg up to date? The wildcard support is recent, b09 I think. If 
you are b09 then maybe there is something else doing on here.


-Alan.


Okay. I just updated my jtreg and the test passes with the wildcard. I'm 
not sure why
it was changed to use the wildcard, but there doesn't seem to be any 
point in me changing it back.


Thanks,
Michael

Re: RFR: 8043118: update test/java/net/URLPermission/nstest/lookup.sh to use explicit @build target

[Michael McMahon]

On 14/05/14 14:20, Daniel Fuchs wrote:

Hi Michael,

On 5/14/14 1:11 PM, Michael McMahon wrote:

On 14/05/14 11:39, Alan Bateman wrote:

On 14/05/2014 11:35, Michael McMahon wrote:

Okay. I just updated my jtreg and the test passes with the wildcard. I'm
not sure why
it was changed to use the wildcard, but there doesn't seem to be any
point in me changing it back.


I believe the issue is that you need to specify any of the library
classes that might be referred to at run time on the @build command
line - otherwise you risk getting a ClassNotFoundException depending
on whether that class got compiled (or not) by the tests that ran
before yours...

So .* is more reliable.

(that's my understanding - but I may be wrong)



No, I think that is correct. Katja explained this offline. Jtreg
isn't able to work out dependencies the same way that javac does.
So, you need to either explicitly list all sources to be built
or provide the wildcard. My change would have worked, but
given the above it's probably safer to stick with the wildcard.

Thanks,
Michael


-- daniel



Thanks,
Michael

Re: RFR JDK-8024832: (so) ServerSocketChannel.socket().accept() throws IllegalBlockingModeException if not bound

[Michael McMahon]

On 27/05/14 10:40, Pavel Rappo wrote:

Hi everyone,

Could you please review my change for JDK-8024832?

http://cr.openjdk.java.net/~prappo/8024832/webrev.00/

Thanks
-Pavel


Just wondering, what does ServerSocket.accept() throw in the same situation?

Michael

Re: RFR JDK-8024832: (so) ServerSocketChannel.socket().accept() throws IllegalBlockingModeException if not bound

[Michael McMahon]

I think it should be throwing a SocketException rather than a
NotYetBoundException to be consistent with ServerSocket.accept()

Michael.

On 27/05/14 10:56, Chris Hegarty wrote:

Looks good to me Pavel. I can sponsor this change for you.

-Chris.

On 27 May 2014, at 10:40, Pavel Rappo  wrote:


Hi everyone,

Could you please review my change for JDK-8024832?

http://cr.openjdk.java.net/~prappo/8024832/webrev.00/

Thanks
-Pavel

Re: RFR JDK-8024832: (so) ServerSocketChannel.socket().accept() throws IllegalBlockingModeException if not bound

[Michael McMahon]

Okay, that's fine then. I didn't see the call to the translation method 
in the diff.


Thanks,
Michael.

On 27/05/14 11:05, Pavel Rappo wrote:

Michael,

java.net.ServerSocket.accept:

 if (!isBound())
 throw new SocketException("Socket is not bound yet");

sun.nio.ch.ServerSocketAdaptor.accept will translate 
java.nio.channels.NotYetBoundException into an instance of SocketException with 
the text you see above. Basically that's what the attached test verifies.

-Pavel

On 27 May 2014, at 10:49, Michael McMahon  wrote:


On 27/05/14 10:40, Pavel Rappo wrote:

Hi everyone,

Could you please review my change for JDK-8024832?

http://cr.openjdk.java.net/~prappo/8024832/webrev.00/

Thanks
-Pavel

Just wondering, what does ServerSocket.accept() throw in the same situation?

Michael

Re: RFR JDK-8024832: (so) ServerSocketChannel.socket().accept() throws IllegalBlockingModeException if not bound

[Michael McMahon]

On 27/05/14 15:56, mark.reinh...@oracle.com wrote:

2014/5/26 20:01 -0700, michael.x.mcma...@oracle.com:

I think it should be throwing a SocketException rather than a
NotYetBoundException to be consistent with ServerSocket.accept()

No, NotYetBoundException is the correct exception here.  The NIO
exception hierarchy was intentionally designed to be richer than
just "SocketException".  Consistency with the exceptions thrown
by the original java.net APIs is an anti-goal.

- Mark

Hi Mark,

The point was clarified on a separate conversation on net-dev.
The method in question refers to the channel's server socket adapter
(ie an instance of java.net.ServerSocket) rather than the channel itself.

What I overlooked was that the exception is being caught correctly
and converted to the right exception expected by ServerSocket.

Michael.

Re: [8u-dev] RFR: 8036979: Support java.net.SocketOption<> in java.net socket types

[Michael McMahon]

Hi Deven,

Thanks for pointing that out. I'll see if we can fix it in time for 8u20.
In fact, it should really have the same value in JDK 9 as well.

Michael

On 03/06/14 08:48, Deven You wrote:

Hi Michael,

I have a small question here. I have roughly gone through the patch 
and find several javadoc indicate @since 1.9.


Since it is an 8u20 patch, I wonder if we should change @since 1.9 to 
@since 1.8 for both 8u20 and java8?


I could provide a patch to change @since 1.9 to @since 1.8 if this is 
what we want.


Thanks a lot!

On 04/11/2014 01:13 AM, Michael McMahon wrote:

Hi,

This is the webrev for the 8u20 version of the fix that was reviewed 
yesterday for 9.


JDK
===
http://cr.openjdk.java.net/~michaelm/8036979.8u20/jdk/01/webrev/

Top repo
=
http://cr.openjdk.java.net/~michaelm/8036979.8u20/top/01/webrev/

The good news is that the change is almost the same as the JDK 9 version
with the following differences:

1) The java.net public API changes are gone. The new public methods 
for 9

 in SocketImpl and DatagramSocketImpl are package private here.

2) A new package private class java.net.SocketsUtil acts as a bridge 
between

the public API in jdk.net.Sockets and the implementation in java.net

3) jdk.net.Sockets uses reflection to access the methods of 
java.net.SocketsUtil


4) The test of the public java.net API is gone and the other test 
augmented

with some additional tests for the standard socket options

Thanks,
Michael

Re: [8u-dev] RFR: 8036979: Support java.net.SocketOption<> in java.net socket types

[Michael McMahon]

Does it matter that they are not in 1.8 FCS? Maybe it depends on the 
exact meaning
of @since. Does it refer to the JDK version, or the Java SE version? If 
the latter,

then we couldn't put them in these classes at all, which doesn't seem right.

Michael

On 04/06/14 10:12, Chris Hegarty wrote:
Since these APIs are not in 1.8 FCS, is there another way to indicate 
what release they were added in?


-Chris.

On 04/06/14 09:51, Michael McMahon wrote:

Hi Deven,

Thanks for pointing that out. I'll see if we can fix it in time for 
8u20.

In fact, it should really have the same value in JDK 9 as well.

Michael

On 03/06/14 08:48, Deven You wrote:

Hi Michael,

I have a small question here. I have roughly gone through the patch
and find several javadoc indicate @since 1.9.

Since it is an 8u20 patch, I wonder if we should change @since 1.9 to
@since 1.8 for both 8u20 and java8?

I could provide a patch to change @since 1.9 to @since 1.8 if this is
what we want.

Thanks a lot!

On 04/11/2014 01:13 AM, Michael McMahon wrote:

Hi,

This is the webrev for the 8u20 version of the fix that was reviewed
yesterday for 9.

JDK
===
http://cr.openjdk.java.net/~michaelm/8036979.8u20/jdk/01/webrev/

Top repo
=
http://cr.openjdk.java.net/~michaelm/8036979.8u20/top/01/webrev/

The good news is that the change is almost the same as the JDK 9 
version

with the following differences:

1) The java.net public API changes are gone. The new public methods
for 9
 in SocketImpl and DatagramSocketImpl are package private here.

2) A new package private class java.net.SocketsUtil acts as a bridge
between
the public API in jdk.net.Sockets and the implementation in 
java.net


3) jdk.net.Sockets uses reflection to access the methods of
java.net.SocketsUtil

4) The test of the public java.net API is gone and the other test
augmented
with some additional tests for the standard socket options

Thanks,
Michael

Re: RFR [9] 8044590: Broken links in jre.api.net.socketoptions

[Michael McMahon]

Looks good.

- Michael

On 04/06/14 11:29, Chris Hegarty wrote:

Looks like a typo in the original @link tag:

diff --git a/src/share/classes/jdk/net/Sockets.java 
b/src/share/classes/jdk/net/Sockets.java

--- a/src/share/classes/jdk/net/Sockets.java
+++ b/src/share/classes/jdk/net/Sockets.java
@@ -51,7 +51,7 @@
  * When a security manager is installed, some non-standard socket 
options

  * may require a security permission before being set or get.
  * The details are specified in {@link ExtendedSocketOptions}. No 
permission

- * is required for {@link java.net.StandardSocketOption}s.
+ * is required for {@link java.net.StandardSocketOptions}.
  *
  * @see java.nio.channels.NetworkChannel
  */

-Chris.

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

[Michael McMahon]

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

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

Thanks,
Michael.

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

[Michael McMahon]

That's fine Sean. We'll just remove them for 8u20.

Thanks
Michael

On 05/06/14 16:04, Seán Coffey wrote:
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 
 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 JDK-8027308: HttpURLConnection should better handle URLs with literal IPv6 addresses

[Michael McMahon]

Looks good. What matters most I think is that the common case
where there is no IPv6 address included, is efficient.

I think a test case should be possible for this also.

- Michael.

On 09/06/14 06:40, Chris Hegarty wrote:

This looks good to me Pavel.

-Chris.

On 6 Jun 2014, at 18:42, Pavel Rappo  wrote:


Hi everyone,

Could you please review my change for JDK-8027308?

http://cr.openjdk.java.net/~prappo/8027308/webrev.00/

The issue in question actually consists of 2 issues. The first one is about supporting 
zone id syntax in URIs. Which is described in [1]. The second one is about handling 
link-local addresses in HTTP headers. The current status of [1] is "Proposed 
Standard". I wasn't able to use the described syntax even with the system utils 
locally (OS X 10.9.3). So I think the work on the first issue can be postponed until [1] 
is widely implemented.
The use case in the second issue can be easily addressed. The only thing is 
that given the complexity of interaction between HttpURLConnection and 
different authentication schemes the change won't fix the scenario of proxied 
http connection. This is something that should be done after the refactoring of 
sun.net.www.protocol.http.

-
[1] http://tools.ietf.org/html/rfc6874

Thanks
-Pavel

RFR 8046588: test for SO_FLOW_SLA availability does not check for EACCESS

[Michael McMahon]

Could I get this change reviewed please?

We need to check if the current process has permission to use the 
SO_FLOW_SLA

socket option as well as test if the feature is installed.

The problem would cause the new test to fail on Solaris machines with S11.2
unless running with the required privilege.

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

This change also needs to be backported to 8u20.

Thanks,
Michael

Re: RFR 8046588: test for SO_FLOW_SLA availability does not check for EACCESS

[Michael McMahon]

On 12/06/14 18:47, Alan Bateman wrote:

On 12/06/2014 18:04, Michael McMahon wrote:

Could I get this change reviewed please?

We need to check if the current process has permission to use the 
SO_FLOW_SLA

socket option as well as test if the feature is installed.

The problem would cause the new test to fail on Solaris machines with 
S11.2

unless running with the required privilege.

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

This change also needs to be backported to 8u20.
I can see that this helps the tests but I just wonder if this might be 
confusing to someone that attempts to use this socket option but 
forgets to run with privileges. In that case then then methods such as 
Socket#setOption will UnsupportedOperationException whereas throwing 
IOException with a somewhat useful message as it does not might be 
better.


Would it be better to just change the test to handle the access denied 
instead?


-Alan.


It would be possible to change the error back, but what about 
supportedOptions() - what should
that return? It doesn't seem right that it would include an option that 
cannot be used and how do

we test it, if we can't tell whether the option is usable or not?

Maybe it depends on how these privileges get configured, which is 
something I'm looking into,
but if it's a case that specific user attributes need to be configured 
on a system, then I'm not sure

we can count on that for our test systems.

Michael

Re: RFR 8046588: test for SO_FLOW_SLA availability does not check for EACCESS

[Michael McMahon]

On 12/06/14 20:35, Alan Bateman wrote:

On 12/06/2014 20:15, Michael McMahon wrote:


It would be possible to change the error back, but what about 
supportedOptions() - what should
that return? It doesn't seem right that it would include an option 
that cannot be used and how do

we test it, if we can't tell whether the option is usable or not?

Maybe it depends on how these privileges get configured, which is 
something I'm looking into,
but if it's a case that specific user attributes need to be 
configured on a system, then I'm not sure

we can count on that for our test systems.
On platforms where the support option is supported then I would think 
that supportedOptions should include it, otherwise I could imagine 
users getting confused by the UOE. For the tests then having them pass 
then the option is supported but can't be set due to lack of 
privileges seems reasonable to me. I guess it's a subjective and maybe 
others might have opinions on this.


-Alan.


But we don't have a distinction between the exception thrown from EACCES 
and E not supported (or whatever it is).


I don't think we want to start parsing error strings ?

Michael

Re: RFR 8046588: test for SO_FLOW_SLA availability does not check for EACCESS

[Michael McMahon]

On 13/06/14 10:08, Chris Hegarty wrote:



On 12/06/14 21:04, Michael McMahon wrote:

On 12/06/14 20:35, Alan Bateman wrote:

On 12/06/2014 20:15, Michael McMahon wrote:


It would be possible to change the error back, but what about
supportedOptions() - what should
that return? It doesn't seem right that it would include an option
that cannot be used and how do
we test it, if we can't tell whether the option is usable or not?

Maybe it depends on how these privileges get configured, which is
something I'm looking into,
but if it's a case that specific user attributes need to be
configured on a system, then I'm not sure
we can count on that for our test systems.

On platforms where the support option is supported then I would think
that supportedOptions should include it, otherwise I could imagine
users getting confused by the UOE.


I think I agree with Alan here. If the platforms support the option, 
then supportedOptions() should include it.


>> For the tests then having them pass

then the option is supported but can't be set due to lack of
privileges seems reasonable to me. I guess it's a subjective and maybe
others might have opinions on this.

-Alan.


But we don't have a distinction between the exception thrown from EACCES
and E not supported (or whatever it is).


This is tricky, and adds a new dimension to socket option access, that 
I didn't realize when this was added.



I don't think we want to start parsing error strings ?


Short of a new unchecked Exception type, then I think we can only 
depend on the exception message ( not pretty! ).


-Chris.



Okay. I can see the reasoning why supportedOptions should refer to the 
platform rather than the process/instance
running. We could consider adding a sub-class of IOException for 
permission related failures, but I'm not proposing
to do that here. For now, I'll just ensure that the error message 
conveys the permission problem.


New webrev: http://cr.openjdk.java.net/~michaelm/8046588/webrev.2/

We also need to check for EPERM. Apparently, there are some codepaths 
that use that instead

of EACCES.

Michael



Michael

Re: RFR 8046588: test for SO_FLOW_SLA availability does not check for EACCESS

[Michael McMahon]

On 13/06/14 12:10, Alan Bateman wrote:

On 13/06/2014 11:49, Michael McMahon wrote:


Okay. I can see the reasoning why supportedOptions should refer to 
the platform rather than the process/instance
running. We could consider adding a sub-class of IOException for 
permission related failures, but I'm not proposing
to do that here. For now, I'll just ensure that the error message 
conveys the permission problem.


New webrev: http://cr.openjdk.java.net/~michaelm/8046588/webrev.2/

We also need to check for EPERM. Apparently, there are some codepaths 
that use that instead

of EACCES.
For the test change then does it mean that a genuine failure will 
cause the test to pass? I don't like change exception messages but I 
just wonder if this test might have to resort to that to avoid passing 
then there is another problem.


-Alan


To be honest, the test doesn't/(can't easily) check if a flow has been 
created. So, in practice
a success return code doesn't prove that everything is working. 
Exercising the code at least
is a basic smoke test. If we add a new exception then maybe we can 
revisit, but I wanted
to get this change in ahead of the refactoring for modularity, becuase 
this change
is needed for 8u20 and I don't want to hold that up and the refactoring 
work will take some

time to review (it's a bit more complicated than expected).

Michael

RFR: 8047187: Test jdk/net/Sockets/Test.java fails to compile after fix JDK-8046588

[Michael McMahon]

This is a simple test case fix for 8u-dev only. Unfortunately, the test case
for 9 was backported in its entirety and makes calls to APIs not 
available in 8.


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

Thanks,
Michael.

Re: Code Review request: 8047186: jdk.net.Sockets throws InvocationTargetException instead of original runtime exceptions

[Michael McMahon]

On 18/06/14 13:53, Artem Smotrakov wrote:

Hello,

Please review this fix for 8u:

https://bugs.openjdk.java.net/browse/JDK-8047186
http://cr.openjdk.java.net/~asmotrak/so_flow_sla/sockets_exceptions/webrev.01/ 



getOption() and setOption() methods of jdk.net.Sockets class throw 
InvocationTargetException instead of actual runtime exceptions like 
NullPointerException and AccessControlException. I think it is better 
to throw an original runtime exception to reduce and simplify stack 
traces.


jdk/net/Sockets/Test.java should have caught the issue, but currently 
it fails due to https://bugs.openjdk.java.net/browse/JDK-8047187. 
That's why I didn't added separate test for this bug.


Artem


I think what you're saying is that we are currently wrapping all 
RuntimeExceptions in a new non-specific

RuntimeException, and it's just better to throw the original exception.

Seems reasonable to me.

Michael.

RFR: 8029607 Type of Service (TOS) cannot be set in IPv6 header

[Michael McMahon]

Could I get the following change reviewed please?

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

It adds support for the IP_TOS socket option to ServerSocketChannel
(and ServerSocket). This means
that the type of service/traffic class can be set on a server socket
and therefore the option set on the responding SYN packet
for incoming connections.

It also fixes a problem where both IPv4 (TOS) and IPv6 (traffic class)
couldn't be used in the same VM instance. After the fix, it still only works
properly in Linux. Windows has an entirely different (non API based)
mechanism for this. Other OS'es may provide the underlying support in
future.

Thanks,
Michael.

Re: RFR: 8029607 Type of Service (TOS) cannot be set in IPv6 header

[Michael McMahon]

On 25/06/14 13:09, Alan Bateman wrote:

On 25/06/2014 12:21, Michael McMahon wrote:

Could I get the following change reviewed please?

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


This mostly looks good to me. I assume you have run the :jdk_nio tests 
and also run them with -vmoption:-Djava.net.preferIPv4Stack=true to 
make sure that there aren't any existing tests that need to be updated.




Yes, it passes ok with IPv6 switched off.

An inconsistency is that net_util_md.h is defining IPV6_TCLASS if not 
already defined whereas genSocketOptionRegistry.c is not. As 
IPV6_TCLASS has been defined for many years then the the #define in 
net_util_md.h might not be needed.


A minor nit in net_util_md.c where L1320 could be s/IPV4/IPv4 and at 
L1337 then the second line of parameters is mis-aligned.


Another minor nit on Net.c L438, probably best to split this over 3 
lines to avoid the inconsistent line length.




Okay. Will update per comments above.

Thanks!
Michael

RFR: 8048212 Two tests failed with "java.net.SocketException: Bad protocol option" on Windows after 8029607

[Michael McMahon]

Could I get the following change reviewed please?

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

It fixes a problem caused by the fix for 8029607. The changes for
that fix should not have been applied to Windows. So, this fix ensures
that the IPv4 option is used always on Windows.

Michael

Re: RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)

[Michael McMahon]

Hi Peter,

Thanks for contributing this. I've started to review it
and will get back to you with comments later in the week.

Regards,
Michael.

On 01/07/14 19:35, Peter Levart wrote:

Hi,

I propose a patch for this issue:

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

The motivation to re-design caching of InetAddress-es was not this 
issue though, but a desire to attack synchronization bottlenecks in 
methods like URL.equals and URL.hashCode which use host name to IP 
address mapping. I plan to tackle the synchronization in URL in a 
follow-up proposal, but I wanted to 1st iron-out the "leaves" of the 
call-tree. Here's the proposed patch:


http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.01/

sun.net.InetAddressCachePolicy:

- two static methods (get() and getNegative()) were synchronized. 
Removed synchronization and made underlying fields volatile.
- also added a normalization of negative policy in 
setNegativeIfNotSet(). The logic in InetAddress doesn't cope with 
negative values distinct from InetAddressCachePolicy.FOREVER (-1), so 
this was a straight bug. The setIfNotSet() doesn't need this 
normalization, because checkValue() throws exception if passed-in 
value < InetAddressCachePolicy.FOREVER.


java.net.InetAddress:

- complete redesign of caching. Instead of distinct Positive/Negative 
caches, there's only one cache - a ConcurrentHashMap. The value in the 
map knows if it contains positive or negative answer.
- the design of this cache is similar but much simpler than 
java.lang.reflect.WeakCache, since it doesn't have to deal with 
WeakReferences and keys are simpler (just strings - hostnames). 
Similarity is in how concurrent requests for the same key (hostname) 
are synchronized when the entry is not cached yet, but still avoid 
synchronization when entry is cached. This preserves the behaviour of 
original InetAddress caching code but simplifies it greatly (100+ 
lines removed).
- I tried to preserve the interaction between 
InetAddress.getLocalHost() and InetAddress.getByName(). The 
getLocalHost() caches the local host address for 5 seconds privately. 
When it expires it performs new name service look-up and "refreshes" 
the entry in the InetAddress.getByName() cache although it has not 
expired yet. I think this is meant to prevent surprises when 
getLocalHost() returns newer address than getByName() which is called 
after that.
- I also fixed the JDK-7186258 as a by-product (but don't know yet how 
to write a test for this issue - any ideas?)


I created a JMH benchmark that tests the following methods:

- InetAddress.getLocalHost()
- InetAddress.getByName() (with positive and negative answer)

Here're the results of running on my 4-core (8-threads) i7/Linux:

http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/InetAddress.Cache_bench_results.01.pdf 



The getByNameNegative() test does not show much improvement in patched 
vs. original code. That's because by default the policy is to NOT 
cache negative answers. Requests for same hostname to the 
NameService(s) are synchronized. If 
"networkaddress.cache.negative.ttl" system property is set to some 
positive value, results are similar to those of getByNamePositive() 
test (the default policy for positive caching is 30 seconds).


I ran the jtreg tests in test/java/net and have the same score as with 
original unpatched code. I have 3 failing tests from original and 
patched runs:


JT Harness : Tests that failed
java/net/MulticastSocket/Promiscuous.java: Test for interference when 
two sockets are bound to the same port but joined to different 
multicast groups
java/net/MulticastSocket/SetLoopbackMode.java: Test 
MulticastSocket.setLoopbackMode
java/net/MulticastSocket/Test.java: IPv4 and IPv6 multicasting broken 
on Linux


And 1 test that had error trying to be run:

JT Harness : Tests that had errors
java/net/URLPermission/nstest/lookup.sh:

Because of:

test result: Error. Can't find source file: jdk/testlibrary/*.java in 
directory-list: 
/home/peter/work/hg/jdk9-dev/jdk/test/java/net/URLPermission/nstest 
/home/peter/work/hg/jdk9-dev/jdk/test/lib/testlibrary


All other 258 java/net tests pass.



So what do you think?


Regards, Peter

Re: RFR: 8048212 Two tests failed with "java.net.SocketException: Bad protocol option" on Windows after 8029607

[Michael McMahon]

On 30/06/14 19:43, Alan Bateman wrote:

On 30/06/2014 17:53, Michael McMahon wrote:

Could I get the following change reviewed please?

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

It fixes a problem caused by the fix for 8029607. The changes for
that fix should not have been applied to Windows. So, this fix ensures
that the IPv4 option is used always on Windows.

I don't know if Windows supports setting IP_TOS on an IPv6 socket but 
in any case, it might be simplest to just go back to the original 
behavior which was for this case to be a no-op. The spec allows for that.


-Alan


Sorry for the delay in getting back to this.

The following is a simpler change that restores the original NIO 
behavior on Windows


http://cr.openjdk.java.net/~michaelm/8048212/webrev.2/

Michael

Re: RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)

[Michael McMahon]

Hi Peter,

Is it necessary to remove the cache entry in the local host case (L1226) ?
It seems redundant to cache it here, and also explicitly in the 
CachedLocalHost object


Michael

On 02/07/14 12:56, Peter Levart wrote:

Hi,

I updated the webrev with first two suggestions from Bernd (expireTime 
instead of createTime and cacheNanos + only use putIfAbsent instead of 
get followed by putIfAbsent):


http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.02/

Thanks, Bernd.

The id field in CachedAddresses is necessary for compareTo to never 
return 0 for two different instances (used as element in 
ConcurrentSkipListSet).


For last two suggestions I'm not sure whether they are desired, so I'm 
currently leaving them as is.



Regards, Peter

On 07/01/2014 10:06 PM, Bernd Eckenfels wrote:

Looks good, like it, Peter.

some nits: instead of adding createTime and cacheNanos, only have a
expireAfter?

L782: is it better to use putIfAbsent unconditionally, instead of
get/putIfAbsent in NameServicdeAddr?

L732: I am unsure about the id field, isnt it enough to have the
identity equality check for the replacement check and otherwise depend
on equals()?

L1223: What about moving the cache exiry inside the if (useCache)

BTW1: might be the wrong RFR, but considering your good performance
numbers for an active cache, would having 100ms or similiar default
negative cache time make sense without impacting (visible) the semantic.



Gruss
Bernd


Am Tue, 01 Jul 2014 20:35:57 +0200
schrieb Peter Levart :


Hi,

I propose a patch for this issue:

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

The motivation to re-design caching of InetAddress-es was not this
issue though, but a desire to attack synchronization bottlenecks in
methods like URL.equals and URL.hashCode which use host name to IP
address mapping. I plan to tackle the synchronization in URL in a
follow-up proposal, but I wanted to 1st iron-out the "leaves" of the
call-tree. Here's the proposed patch:

http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.01/ 



sun.net.InetAddressCachePolicy:

- two static methods (get() and getNegative()) were synchronized.
Removed synchronization and made underlying fields volatile.
- also added a normalization of negative policy in
setNegativeIfNotSet(). The logic in InetAddress doesn't cope with
negative values distinct from InetAddressCachePolicy.FOREVER (-1), so
this was a straight bug. The setIfNotSet() doesn't need this
normalization, because checkValue() throws exception if passed-in
value < InetAddressCachePolicy.FOREVER.

java.net.InetAddress:

- complete redesign of caching. Instead of distinct Positive/Negative
caches, there's only one cache - a ConcurrentHashMap. The value in
the map knows if it contains positive or negative answer.
- the design of this cache is similar but much simpler than
java.lang.reflect.WeakCache, since it doesn't have to deal with
WeakReferences and keys are simpler (just strings - hostnames).
Similarity is in how concurrent requests for the same key (hostname)
are synchronized when the entry is not cached yet, but still avoid
synchronization when entry is cached. This preserves the behaviour of
original InetAddress caching code but simplifies it greatly (100+
lines removed).
- I tried to preserve the interaction between
InetAddress.getLocalHost() and InetAddress.getByName(). The
getLocalHost() caches the local host address for 5 seconds privately.
When it expires it performs new name service look-up and "refreshes"
the entry in the InetAddress.getByName() cache although it has not
expired yet. I think this is meant to prevent surprises when
getLocalHost() returns newer address than getByName() which is called
after that.
- I also fixed the JDK-7186258 as a by-product (but don't know yet
how to write a test for this issue - any ideas?)

I created a JMH benchmark that tests the following methods:

- InetAddress.getLocalHost()
- InetAddress.getByName() (with positive and negative answer)

Here're the results of running on my 4-core (8-threads) i7/Linux:

http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/InetAddress.Cache_bench_results.01.pdf 




The getByNameNegative() test does not show much improvement in
patched vs. original code. That's because by default the policy is to
NOT cache negative answers. Requests for same hostname to the
NameService(s) are synchronized. If
"networkaddress.cache.negative.ttl" system property is set to some
positive value, results are similar to those of getByNamePositive()
test (the default policy for positive caching is 30 seconds).

I ran the jtreg tests in test/java/net and have the same score as
with original unpatched code. I have 3 failing tests from original
and patched runs:

JT Harness : Tests that failed
java/net/MulticastSocket/Promiscuous.java: Test for interference when
two sockets are bound to the same port but joined to different
multicast groups
java/net/MulticastSocket/SetLoopbackMode.java: 

Re: RFR JDK-7186258: InetAddress$Cache should replace currentTimeMillis with nanoTime (+more)

[Michael McMahon]

Peter,

Thanks for the explanation. No. I think your change is good. I've run 
tests here locally

and I'm happy with it overall.

Michael

On 07/07/14 14:10, Peter Levart wrote:

On 07/07/2014 12:59 PM, Michael McMahon wrote:

Hi Peter,

Is it necessary to remove the cache entry in the local host case 
(L1226) ?
It seems redundant to cache it here, and also explicitly in the 
CachedLocalHost object


Michael


Hi Michael,

Thanks for looking into this.

getLocalHost() seems to have a special hard-coded policy of positive 
caching for 5 seconds that is independent of general getByName() 
caching policy (30 seconds by default). The behaviour of original code 
that I'm trying to replicate is such that when getLocalHost() notices 
a change of local host name -> address mapping, the mapping in global 
cache for this change is also updated. I think this is to avoid 
situations like:


Let's say the local host name is "cube":

InetAddress addr1 = InetAddress.getLocalHost();
InetAddress addr2 = InetAddress.getByName("cube");
// addr1.equals(addr2) here

// 5 seconds later, cube -> IP mapping is updated in DNS or /etc/hosts 
...


addr1 = InetAddress.getLocalHost();
addr2 = InetAddress.getByName("cube");
// if getLocalHost() did not update global cache,
// addr1 (new IP address) would be different from addr2 (old IP address)


Another way to accomplish similar guarantee would be to special-case 
the caching policy in global cache which would check whether the entry 
is for local host name and set 'expiryTime' accordingly. This would be 
a little different behaviourally, because InetAddress.getByName() 
would get a 5 second expiry for local host name too, regardless of 
whether InetAddress.getLocalHost() has been called at all. But we 
could get rid of special CachedLocalHost class then. Is such 
behavioural change warranted?


Regards, Peter



On 02/07/14 12:56, Peter Levart wrote:

Hi,

I updated the webrev with first two suggestions from Bernd 
(expireTime instead of createTime and cacheNanos + only use 
putIfAbsent instead of get followed by putIfAbsent):


http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.02/ 



Thanks, Bernd.

The id field in CachedAddresses is necessary for compareTo to never 
return 0 for two different instances (used as element in 
ConcurrentSkipListSet).


For last two suggestions I'm not sure whether they are desired, so 
I'm currently leaving them as is.



Regards, Peter

On 07/01/2014 10:06 PM, Bernd Eckenfels wrote:

Looks good, like it, Peter.

some nits: instead of adding createTime and cacheNanos, only have a
expireAfter?

L782: is it better to use putIfAbsent unconditionally, instead of
get/putIfAbsent in NameServicdeAddr?

L732: I am unsure about the id field, isnt it enough to have the
identity equality check for the replacement check and otherwise depend
on equals()?

L1223: What about moving the cache exiry inside the if (useCache)

BTW1: might be the wrong RFR, but considering your good performance
numbers for an active cache, would having 100ms or similiar default
negative cache time make sense without impacting (visible) the 
semantic.




Gruss
Bernd


Am Tue, 01 Jul 2014 20:35:57 +0200
schrieb Peter Levart :


Hi,

I propose a patch for this issue:

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

The motivation to re-design caching of InetAddress-es was not this
issue though, but a desire to attack synchronization bottlenecks in
methods like URL.equals and URL.hashCode which use host name to IP
address mapping. I plan to tackle the synchronization in URL in a
follow-up proposal, but I wanted to 1st iron-out the "leaves" of the
call-tree. Here's the proposed patch:

http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.01/ 



sun.net.InetAddressCachePolicy:

- two static methods (get() and getNegative()) were synchronized.
Removed synchronization and made underlying fields volatile.
- also added a normalization of negative policy in
setNegativeIfNotSet(). The logic in InetAddress doesn't cope with
negative values distinct from InetAddressCachePolicy.FOREVER (-1), so
this was a straight bug. The setIfNotSet() doesn't need this
normalization, because checkValue() throws exception if passed-in
value < InetAddressCachePolicy.FOREVER.

java.net.InetAddress:

- complete redesign of caching. Instead of distinct Positive/Negative
caches, there's only one cache - a ConcurrentHashMap. The value in
the map knows if it contains positive or negative answer.
- the design of this cache is similar but much simpler than
java.lang.reflect.WeakCache, since it doesn't have to deal with
WeakReferences and keys are simpler (just strings - hostnames).
Similarity is in how concurrent requests for the same key (hostname)
are synchronized when the entry is not cached yet, but still avoid
synchronization when entry is cached. This preserv

Re: RFR 7150092: NTLM authentication fail if user specified a different realm

[Michael McMahon]

Max,

These changes look fine. Just a couple of minor comments:

L130 in Client.java appears to be superfluous now.

The comment at L186 in Server.java might probably should
be removed or else expanded upon.

Thanks
Michael


On 23/06/14 09:09, Wang Weijun wrote:

Ping again.

On Jun 12, 2014, at 14:07, Wang Weijun  wrote:


Hi All

Please review the code change at

  http://cr.openjdk.java.net/~weijun/7150092/webrev.00/

The problem is that in NTLM, the server might prompt for a domain name (in Type 
2 message), and the client can also provide one. Before this fix, if the two 
are different, the client chooses the one from the server. After this fix, the 
client will always uses its own even if it's empty. This is confirmed by 
looking at the behavior of IE/Firefox/Chrome. The server sent domain name was 
designed to be used to create the NTLMv2 response but it's now obsoleted by 
alist. Chrome/Firefox simply ignore it, so will Java. (IE does use it if there 
is no alist)

There are some other small changes:

Client.java
---

96-108: No one sends hostname and domain in the Type 1 message, so they are 
removed. Everyone adds a 0x4 flag which means Request Target.

Removed old 137-139: That's the major change.

159: I used to detect whether there is an alist by looking at the length. This 
is not accurate if the domain is very long. The correct way is to look at the 
flag (0x80 means alist is there)

Server.java
---

98: Adds a flag 0x1 which means the target name (line 99) written into the 
message is a domain

135: Always uses the client provided domain to search for password. This is 
also a part of the major change.

NTLMClient.java
---

If user has not responded to NameCallback and/or RealmCallback, it means they 
accept the default value.

NTLMServer.java
---

ntdomain could be empty or null, the 2-arg constructor of RealmCallback could 
fail in this case. Use 1-arg constructor.

NTLMAuthentication.java
---

According to my observation of IE/Firefox/Chrome, when user does not type in a 
domain name in the password prompting dialog, the domain sent to server is an 
empty string, and the host name is always full name. Update Java to be the same.

NTLMTest.java
-

Update the test to reflect code changes.

Thanks
Max

Re: java.net.URI and RFC 3986 compliance

[Michael McMahon]

On 10/07/14 09:11, Peter Levart wrote:

On 07/10/2014 02:50 AM, Peter Firmstone wrote:
Are there parties on this list interested in updating java.net.URI to 
RFC3986?


Is there anyone here who has previously attempted this?  If so what 
issues did you find with regard to backward compatibility?


Regards,

Peter.



Hi Peter,

I think It's good that you started this discussion. Since you already 
have some experience with RFC3986 and possibly also know the 
differences to RFC 2396 (I have just started reading both RFCs, so I 
am in no way a relevant source of information in this area), please 
feel free to ignore my noob questions...


Would it be possible to add a method to URI, say URI.normalize3986() 
and leave existing logic of URI unchanged? What could and what 
couldn't be achieved by that?


Regards, Peter



I think the starting point should be a list of the differences, and then 
we can see whether additional methods
would suffice. Apart from anything else, the behavior of each method has 
to be fully specified and documented in the class.

I'll try to locate any relevant history.

But, support for RFC3986 is definitely desirable. I agree also, that it 
shouldn't be predicated on a global switch.
Additional constructors/factory methods would be one approach, but it is 
dependent on what the differences are.


Michael

Re: sun.net.www.protocol.https.HttpsURLConnectionImpl doesn't equal to self

[Michael McMahon]

I'll file a bug for this Stanimir. Thanks for reporting it.
Should be able to fix it in JDK 9 fairly promptly
and we'll see about back porting it then.

- Michael.

On 18/08/14 15:04, Stanimir Simeonoff wrote:

Hi,

As the title says there is a major bug with HttpsURLConnection as it breaks
the contract of equals. So the instance  cannot be contained (mostly
removed) in any collection reliably. (Concurrent)HashMap has a provision to
test equality by reference prior calling equals, though.
Finding the bug in production is quite a ride as the http variant doesn't
exhibit the problem.

Here is a simple test case.

public static void main(String[] args) throws Throwable{
  URLConnection c = new URL("https://oracle.com";).openConnection();
  System.out.println(c.getClass().getName()+" equals self: "
+c.equals(c));
  c.getInputStream().close();
  System.out.println(c.getClass().getName()+" equals self: "
+c.equals(c));
}


The culprit is in HttpsURLConnectionImpl.equals that blindly calls
delagate.equals:

 public boolean equals(Object obj) {
 return delegate.equals(obj);
 }

It should be changed to:

 public boolean equals(Object obj) {
 return this==obj || (obj instanceof HttpsURLConnectionImpl) &&
delegate.equals( ((HttpsURLConnectionImpl)obj).delegate );
 }


The class has some other issue that involves declaring "finalize" method to
simply call delegate's dispose. The finalize method is unneeded and just
creates unnecessary link in the finalization queue + Finalizer object.

Thanks
Stanimir

Re: RFR JDK-8049228: Improve multithreaded scalability of InetAddress cache

[Michael McMahon]

This still looks fine to me Peter.

Thanks
Michael

On 19/08/14 11:51, Peter Levart wrote:

Hi Michael,

I have re-based the patch to the new jdk9 source layout. Nothing 
changes from the webrev.03 except paths:


http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.04/


Regards, Peter

On 07/09/2014 01:52 PM, Peter Levart wrote:

Hi Michael,

Thanks for testing. I have prepared another webrev:

http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.03/ 



It only cleans up two comments suggested by Bernd (removed 
superfluous phrase "with 0" from statements about comparing time 
instants). So do you think this needs more testing / another review 
or can I consider this reviewed for jdk9-dev ?


Regards, Peter


On 07/07/2014 04:13 PM, Michael McMahon wrote:

Peter,

Thanks for the explanation. No. I think your change is good. I've 
run tests here locally

and I'm happy with it overall.

Michael

On 07/07/14 14:10, Peter Levart wrote:

On 07/07/2014 12:59 PM, Michael McMahon wrote:

Hi Peter,

Is it necessary to remove the cache entry in the local host case 
(L1226) ?
It seems redundant to cache it here, and also explicitly in the 
CachedLocalHost object


Michael


Hi Michael,

Thanks for looking into this.

getLocalHost() seems to have a special hard-coded policy of 
positive caching for 5 seconds that is independent of general 
getByName() caching policy (30 seconds by default). The behaviour 
of original code that I'm trying to replicate is such that when 
getLocalHost() notices a change of local host name -> address 
mapping, the mapping in global cache for this change is also 
updated. I think this is to avoid situations like:


Let's say the local host name is "cube":

InetAddress addr1 = InetAddress.getLocalHost();
InetAddress addr2 = InetAddress.getByName("cube");
// addr1.equals(addr2) here

// 5 seconds later, cube -> IP mapping is updated in DNS or 
/etc/hosts ...


addr1 = InetAddress.getLocalHost();
addr2 = InetAddress.getByName("cube");
// if getLocalHost() did not update global cache,
// addr1 (new IP address) would be different from addr2 (old IP 
address)



Another way to accomplish similar guarantee would be to 
special-case the caching policy in global cache which would check 
whether the entry is for local host name and set 'expiryTime' 
accordingly. This would be a little different behaviourally, 
because InetAddress.getByName() would get a 5 second expiry for 
local host name too, regardless of whether 
InetAddress.getLocalHost() has been called at all. But we could get 
rid of special CachedLocalHost class then. Is such behavioural 
change warranted?


Regards, Peter



On 02/07/14 12:56, Peter Levart wrote:

Hi,

I updated the webrev with first two suggestions from Bernd 
(expireTime instead of createTime and cacheNanos + only use 
putIfAbsent instead of get followed by putIfAbsent):


http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.02/ 



Thanks, Bernd.

The id field in CachedAddresses is necessary for compareTo to 
never return 0 for two different instances (used as element in 
ConcurrentSkipListSet).


For last two suggestions I'm not sure whether they are desired, 
so I'm currently leaving them as is.



Regards, Peter

On 07/01/2014 10:06 PM, Bernd Eckenfels wrote:

Looks good, like it, Peter.

some nits: instead of adding createTime and cacheNanos, only have a
expireAfter?

L782: is it better to use putIfAbsent unconditionally, instead of
get/putIfAbsent in NameServicdeAddr?

L732: I am unsure about the id field, isnt it enough to have the
identity equality check for the replacement check and otherwise 
depend

on equals()?

L1223: What about moving the cache exiry inside the if (useCache)

BTW1: might be the wrong RFR, but considering your good performance
numbers for an active cache, would having 100ms or similiar default
negative cache time make sense without impacting (visible) the 
semantic.




Gruss
Bernd


Am Tue, 01 Jul 2014 20:35:57 +0200
schrieb Peter Levart :


Hi,

I propose a patch for this issue:

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

The motivation to re-design caching of InetAddress-es was not this
issue though, but a desire to attack synchronization 
bottlenecks in

methods like URL.equals and URL.hashCode which use host name to IP
address mapping. I plan to tackle the synchronization in URL in a
follow-up proposal, but I wanted to 1st iron-out the "leaves" 
of the

call-tree. Here's the proposed patch:

http://cr.openjdk.java.net/~plevart/jdk9-dev/InetAddress.Cache/webrev.01/ 



sun.net.InetAddressCachePolicy:

- two static methods (get() and getNegative()) were synchronized.
Removed synchronization and made underlying fields volatile.
- also added a normalization of negative policy in
setNegativeIfNotSet(). The logic in InetAddress doesn't cope with
negative values distinct f

RFR: 8055299 HttpsURLConnection.equals() broken

[Michael McMahon]

This is the recently reported fix to HttpsURLConnection.equals

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

The fix includes a test. Not shown in the webrev is java key store
file called testkeys, which is copied from another location
in the test tree and is to be installed in the same directory
as the new test.

Thanks
Michael