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

[Mark Sheppard]

Hi Chris,
  yes, I concur, also

regards
Mark

On 23/10/2013 12:09, Michael McMahon wrote:

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

hg: jdk8/tl/corba: 8028215: ORB.init fails with SecurityException if properties select the JDK default ORB

[mark . sheppard]

Changeset: fe781b3badd6
Author:msheppar
Date:  2013-11-21 11:30 +
URL:   http://hg.openjdk.java.net/jdk8/tl/corba/rev/fe781b3badd6

8028215: ORB.init fails with SecurityException if properties select the JDK 
default ORB
Summary: check for default ORBImpl and ORBSingleton set via properties or 
System properties
Reviewed-by: alanb, coffeys, mchung

! src/share/classes/org/omg/CORBA/ORB.java

hg: jdk8/tl/jdk: 8028215: ORB.init fails with SecurityException if properties select the JDK default ORB

[mark . sheppard]

Changeset: d5d4b9a63174
Author:msheppar
Date:  2013-11-21 11:36 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d5d4b9a63174

8028215: ORB.init fails with SecurityException if properties select the JDK 
default ORB
Summary: check for default ORBImpl and ORBSingleton set via properties or 
System properties
Reviewed-by: alanb, coffeys, mchung

+ test/com/sun/corba/se/impl/orb/SetDefaultORBTest.java

RFR: JDK-8025211 - Intermittent test failure: java/net/DatagramSocket/PortUnreachable.java

[Mark Sheppard]

should have been this list by rights!!

regards
Mark

 Original Message 
Subject: 	RFR: JDK-8025211 - Intermittent test failure: 
java/net/DatagramSocket/PortUnreachable.java

Date:   Fri, 29 Nov 2013 14:21:33 +
From:   Mark Sheppard 
Organization:   Oracle Corporation
To: core-libs-dev 



Hi
please oblige and review the following changes

http://cr.openjdk.java.net/~msheppar/8025211/webrev/
which address the issue raised in the bug
https://bugs.openjdk.java.net/browse/JDK-8025211

an intermittent failure occurs on some windows test machines.

this replaces a Thread.sleep(5000) with explicit synchronization between
sender
and receiver via a CountDownLatch

regards
Mark

Re: RFR: JDK-8025211 - Intermittent test failure: java/net/DatagramSocket/PortUnreachable.java

[Mark Sheppard]

Alan, Daniel,
   thanks for the replies.

yes, 'tis a possibility, and it appears to work ok as a test, as it
avoids the sender's Thread.sleep creating a race condition, such that
the send doesn't happen prior to the timeout of the client receive.

The context of the test appears to be creating conditions
such that an ICMP  PortUnreachable event is generated,
which caused a close a particular version of windows.
The test is for socket closed scenario, afaik

tested on the errant test machine with consistent success.

regards
Mark

On 29/11/2013 16:41, Daniel Fuchs wrote:

On 11/29/13 5:19 PM, Alan Bateman wrote:

On 29/11/2013 14:21, Mark Sheppard wrote:

Hi
please oblige and review the following changes

http://cr.openjdk.java.net/~msheppar/8025211/webrev/
which address the issue raised in the bug
https://bugs.openjdk.java.net/browse/JDK-8025211

an intermittent failure occurs on some windows test machines.

this replaces a Thread.sleep(5000) with explicit synchronization
between sender
and receiver via a CountDownLatch

(cc'ing net-dev)

Would an alternative here be to just get rid of the server thread (do
both the send + receive on the main thread)?


Hmm... The message sent by the server is short enough so that it
should be stored in the client's socket buffer and received later
by the client, even though the client is not yet blocked waiting
in clientSock.receive() when the message is sent.

That might work.

Unless multi-threading was relevant to the configuration being
tested?


-- daniel



-Alan

Re: RFR: JDK-8025211 - Intermittent test failure: java/net/DatagramSocket/PortUnreachable.java

[Mark Sheppard]

Hi
   based on feedback the changes for issue:
https://bugs.openjdk.java.net/browse/JDK-8025211

have been amended to the following:
http://cr.openjdk.java.net/~msheppar/8025211/webrev.02/

please oblige and review

regards
Mark

On 29/11/2013 17:17, Mark Sheppard wrote:


Alan, Daniel,
   thanks for the replies.

yes, 'tis a possibility, and it appears to work ok as a test, as it
avoids the sender's Thread.sleep creating a race condition, such that
the send doesn't happen prior to the timeout of the client receive.

The context of the test appears to be creating conditions
such that an ICMP  PortUnreachable event is generated,
which caused a close a particular version of windows.
The test is for socket closed scenario, afaik

tested on the errant test machine with consistent success.

regards
Mark

On 29/11/2013 16:41, Daniel Fuchs wrote:

On 11/29/13 5:19 PM, Alan Bateman wrote:

On 29/11/2013 14:21, Mark Sheppard wrote:

Hi
please oblige and review the following changes

http://cr.openjdk.java.net/~msheppar/8025211/webrev/
which address the issue raised in the bug
https://bugs.openjdk.java.net/browse/JDK-8025211

an intermittent failure occurs on some windows test machines.

this replaces a Thread.sleep(5000) with explicit synchronization
between sender
and receiver via a CountDownLatch

(cc'ing net-dev)

Would an alternative here be to just get rid of the server thread (do
both the send + receive on the main thread)?


Hmm... The message sent by the server is short enough so that it
should be stored in the client's socket buffer and received later
by the client, even though the client is not yet blocked waiting
in clientSock.receive() when the message is sent.

That might work.

Unless multi-threading was relevant to the configuration being
tested?


-- daniel



-Alan

hg: jdk8/tl/jdk: 8025211: Intermittent test failure: java/net/DatagramSocket/PortUnreachable.java

[mark . sheppard]

Changeset: 39b3b0e77af5
Author:msheppar
Date:  2013-12-02 14:01 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/39b3b0e77af5

8025211: Intermittent test failure: java/net/DatagramSocket/PortUnreachable.java
Summary: modified test to execute in a single thread to eliminate potential 
race condition
Reviewed-by: alanb, chegar, dfuchs

! test/java/net/DatagramSocket/PortUnreachable.java

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

[Mark Sheppard]

Hi,
   please oblige and review the following changes

http://cr.openjdk.java.net/~msheppar/7102702/webrev/

to address the issue

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

This test fails on windows, and it was found to be failing with
an error WSAEFAULT (10014),  when invoking a recvfrom() in the
purgeOutstandingICMP function  (in 
src/windows/native/java/net/DualStackPlainDatagramSocketImpl.c).
This is indicative of a problem with the from address structure passed 
to the recvfrom().

Changing the
struct sockaddr_in to SOCKETADDRESS rectifies the issue i.e the
purgeOutstandingICMP will "swallow" the ICMP message event notifications,
manifested as an WSAECONNRESET error for a recvfrom() invocation.

 The test will pass as expected.

regards
Mark

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

[Mark Sheppard]

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.


regards
Mark

On 18/12/2013 16:05, Chris Hegarty wrote:

On 18 Dec 2013, at 15:13, Alan Bateman  wrote:


On 18/12/2013 15:09, Mark Sheppard wrote:

Hi,
   please oblige and review the following changes

http://cr.openjdk.java.net/~msheppar/7102702/webrev/

Good sleuthing!! The change looks good to me.

Yes, this is a good find, and the changes look good.

Is there the same issue ( and fix ) in TwoStacksPlainDatagramSocketImpl.c also ?

-Chris


-Alan.

RFR: java/net/MulticastSocket/SetGetNetworkInterfaceTest.java throws java.net.SocketException: Cannot assign requested address

[Mark Sheppard]

Hi,
   Please oblige and review the following changes:

http://cr.openjdk.java.net/~msheppar/8027903/webrev/

which address the issue:

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

The test failure was found to be due to an interface configured with an 
IPv6 address and
no IPv4 address. The native code was calling 
mcast_set_if_by_addr_v4(...), which
raised an exception. The native code in 
src/solaris/native/java/net/PlainDatagramSocketImpl.c was amended to
check for a pending exception, and clear it prior to executing the ipv6 
function, in the ipv6 conditional block.


regards
Mark

Re: RFR: java/net/MulticastSocket/SetGetNetworkInterfaceTest.java throws java.net.SocketException: Cannot assign requested address

[Mark Sheppard]

Hi Alan,
  added an ExceptionDescribe to native code and an -Xcheck:jni to the 
cmd line options to produce the

following:

WARNING in native method: JNI call made with exception pending
at java.net.PlainDatagramSocketImpl.socketSetOption(Native Method)
at 
java.net.AbstractPlainDatagramSocketImpl.setOption(AbstractPlainDatagramSocketImpl.java:309)
at 
java.net.MulticastSocket.setNetworkInterface(MulticastSocket.java:554)

- locked <0x00076c8c78e8> (a java.lang.Object)
at 
SetGetNetworkInterfaceTest.main(SetGetNetworkInterfaceTest.java:54)

at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

at java.lang.reflect.Method.invoke(Method.java:483)
at 
com.sun.javatest.regtest.MainWrapper$MainThread.run(MainWrapper.java:94)

at java.lang.Thread.run(Thread.java:744)

and ExceptionDescribe outputs

Exception in thread "MainThread" java.net.SocketException: Cannot assign 
requested address

at java.net.PlainDatagramSocketImpl.socketSetOption(Native Method)
at 
java.net.AbstractPlainDatagramSocketImpl.setOption(AbstractPlainDatagramSocketImpl.java:309)
at 
java.net.MulticastSocket.setNetworkInterface(MulticastSocket.java:554)
at 
SetGetNetworkInterfaceTest.main(SetGetNetworkInterfaceTest.java:54)

at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

at java.lang.reflect.Method.invoke(Method.java:483)
at 
com.sun.javatest.regtest.MainWrapper$MainThread.run(MainWrapper.java:94)

at java.lang.Thread.run(Thread.java:744)


regards
Mark

On 29/12/2013 17:05, Alan Bateman wrote:

On 23/12/2013 18:21, Mark Sheppard wrote:

Hi,
   Please oblige and review the following changes:

http://cr.openjdk.java.net/~msheppar/8027903/webrev/

which address the issue:

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

The test failure was found to be due to an interface configured with 
an IPv6 address and
no IPv4 address. The native code was calling 
mcast_set_if_by_addr_v4(...), which

raised an exception.
Do you know what the exception is? Just wondering what exception is 
being cleared/ignored.


-Alan

RFR: JDK-8015692 - java.net.BindException is thrown on Windows XP when HTTP server is started and stopped in the loop.

[Mark Sheppard]

Hi

Please oblige and review the changes in the webrev

http://cr.openjdk.java.net/~msheppar/8015692/jdk9/webrev/

to address the issue raised in the bug

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

Summary:
a series Junit tests which start stop instances of an 
com.sun.net.httpserver.HttpServer failed due to

java.net.BindException: Address already in use: bind

This was raised against Windows XP, but the sample test to reproduce the 
issue

was run on Windows 7, and the problem was seen to occur on this OS also.
The sample was run against jdk7, jdk8 and jdk9: reproducible on each.

On investigation it  appears that some additional co-ordination is 
required between the
HttpServer's  (actually SereverImpl) dispatcher thread and the thread 
invoking the stop
method. This change has amended the stop method to wait for the 
Dispatcher thread to complete, then
invokes the selector's selectNow, to handled cancelled events, and 
closes the selector.

The selector.close() has been removed from the Dispatcher's run method.

regards
Mark

Re: RFR: JDK-8015692 - java.net.BindException is thrown on Windows XP when HTTP server is started and stopped in the loop.

[Mark Sheppard]

Hi Chris,
   thanks for the response.

Yes, that's true.
It was just the way it evolved as I analyzed the issue.
Originally, the join was after the close and selectNow.
The close was moved from Dispatcher to stop, as there was some 
"interplay" between the Dispatcher thread

and the stop thread, when the Dispatcher was invoking the close.
Then added the join() in the stop method, to ensure that the Dispatcher 
wasn't still executing after the server had been

stopped.

As the Selector is opened in the ServerImpl constructor and not in the 
Dispatcher, it seemed
from a symmetry view point more logical  to invoke the close in the 
ServerImpl stop


The selectNow is just insurance for cleanup purposes.

It is possible that the join should be higher up in the stop() flow i.e. 
immediately after the

setting the finish flag?
As such, the Dispatcher should be finished with the various 
HttpConnection collections, before

the stop processes them.

regards
Mark

On 21/02/2014 07:22, Chris Hegarty wrote:

Mark,

I agree with you, there is certainly some additional co-ordination needed 
between the thread invoking the stop method and the dispatcher thread.

I wonder why you needed to add the selectNow() and the close() after you have 
joined the dispatcher thread? Since you are guaranteed that the dispatcher 
thread will have exited before join() returns?

-Chris.

On 17 Feb 2014, at 01:20, Mark Sheppard  wrote:


Hi

Please oblige and review the changes in the webrev

http://cr.openjdk.java.net/~msheppar/8015692/jdk9/webrev/

to address the issue raised in the bug

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

Summary:
a series Junit tests which start stop instances of an 
com.sun.net.httpserver.HttpServer failed due to
java.net.BindException: Address already in use: bind

This was raised against Windows XP, but the sample test to reproduce the issue
was run on Windows 7, and the problem was seen to occur on this OS also.
The sample was run against jdk7, jdk8 and jdk9: reproducible on each.

On investigation it  appears that some additional co-ordination is required 
between the
HttpServer's  (actually SereverImpl) dispatcher thread and the thread invoking 
the stop
method. This change has amended the stop method to wait for the Dispatcher 
thread to complete, then
invokes the selector's selectNow, to handled cancelled events, and closes the 
selector.
The selector.close() has been removed from the Dispatcher's run method.

regards
Mark

RFR: 8025293 - JNI exception pending checks in java.net

[Mark Sheppard]

Hi
  please oblige and review the following changes

http://cr.openjdk.java.net/~msheppar/8025293/webrev/

to address the issue in

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

this applies additional checks after JNI native calls in
src/solaris/native/java/net/NetworkInterface.c
src/windows/native/java/net/NetworkInterface.c

The main changes are NULL checks and pending exception checks.

regards
Mark

Re: RFR: 8025293 - JNI exception pending checks in java.net

[Mark Sheppard]

Chris,
  thanks for the response.
I can use the common CHECK_NULL_RETURN convention if preferred.
The reason I went with if ( name_utf != NULL) { ...} else { ...} is 
because the != NULL is the most

likely code path in getByName.

Nonetheless, I'll make the change for consistency.

regards
Mark

On 04/03/2014 11:27, Chris Hegarty wrote:

This looks good to me Mark.

Trivially, and it is more of a stylistic preference, towards the end of the 
Solaris NetworkInterface.c I would remove the if (ob) … else, and use 
CHECK_NULL_RETURN(obj, NULL). But, what you have is right, either is fine.

-Chris.

On 4 Mar 2014, at 11:06, Mark Sheppard  wrote:


Hi
  please oblige and review the following changes

http://cr.openjdk.java.net/~msheppar/8025293/webrev/

to address the issue in

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

this applies additional checks after JNI native calls in
src/solaris/native/java/net/NetworkInterface.c
src/windows/native/java/net/NetworkInterface.c

The main changes are NULL checks and pending exception checks.

regards
Mark

Re: RFR: 8025293 - JNI exception pending checks in java.net

[Mark Sheppard]

Hi Alan,
  thanks for the response.  I originally had a CHECK_NULL_RETURN after 
getBraodcast and tests

failed on Macos, but not on other platforms!
The issue is that getBroadcast returns NULL when broadcast is not 
available for an interface, or when the ioctl calls
return an error. The former is the case on macos with the loopback lo 
interface.

Hence we went with the ExceptionCheck after these calls.
WRT returning ifs, I just followed the convention created in the 
CHECKED_MALLOC3 macro, as

ifs is an "in" parameter.

regards
Mark

On 04/03/2014 11:28, Alan Bateman wrote:

On 04/03/2014 11:06, Mark Sheppard wrote:

Hi
  please oblige and review the following changes

http://cr.openjdk.java.net/~msheppar/8025293/webrev/

to address the issue in

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

this applies additional checks after JNI native calls in
src/solaris/native/java/net/NetworkInterface.c
src/windows/native/java/net/NetworkInterface.c

The main changes are NULL checks and pending exception checks.
I went through the webrev and it looks reasonable to me. The 
awkwardness of not knowing if GetStringUTFChars fails with a pending 
exception or not makes me wonder if the recent suggestion on 
core-libs-dev to check for a pending exception might be worth looking 
at again, even by introducing variations of JNU_ThrowOutOfMemoryError. 
Not for this patch of course but this patch does highlight the issue 
again.


One thing that isn't 100% clear to me is in the Solaris/Linux version 
of addif where it returns ifs if either getBroadcast or getSubnet fail 
with a pending exception. Is there additional cleanup/free that needs 
to be done for this case or is this the reason for returning the 
partially initialized ifs. I also wonder if it's necessary to check 
for a pending exception, could getBroadcast returning NULL is used too?


-Alan.

RFR: JDK-8036607 - JNI exception pending in jdk/src/windows/native/java/net/DualStackPlainDatagramSocketImpl.c

[Mark Sheppard]

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

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

Summary:
Changes to src/windows/native/java/net/DualStackPlainDatagramSocketImpl.c
two additions to take into account pending JNI exceptions have been
added

regards
Mark

-oOo-
diff -r 9099a251d211 
src/windows/native/java/net/DualStackPlainDatagramSocketImpl.c
--- a/src/windows/native/java/net/DualStackPlainDatagramSocketImpl.c Wed 
Mar 05 11:53:35 2014 -0800
+++ b/src/windows/native/java/net/DualStackPlainDatagramSocketImpl.c Fri 
Mar 07 11:44:51 2014 +

@@ -384,15 +384,19 @@ JNIEXPORT jint JNICALL Java_java_net_Dua
 if (packetAddress == NULL) {
 packetAddress = NET_SockaddrToInetAddress(env, (struct 
sockaddr *)&sa,

   &port);
-/* stuff the new Inetaddress into the packet */
-(*env)->SetObjectField(env, dpObj, dp_addressID, 
packetAddress);

+if (packetAddress != NULL) {
+/* stuff the new Inetaddress into the packet */
+(*env)->SetObjectField(env, dpObj, dp_addressID, 
packetAddress);

+}
 }

-/* populate the packet */
-(*env)->SetByteArrayRegion(env, packetBuffer, 
packetBufferOffset, rv,

+if (!(*env)->ExceptionCheck(env)) {
+/* populate the packet */
+(*env)->SetByteArrayRegion(env, packetBuffer, 
packetBufferOffset, rv,

(jbyte *)fullPacket);
-(*env)->SetIntField(env, dpObj, dp_portID, port);
-(*env)->SetIntField(env, dpObj, dp_lengthID, rv);
+(*env)->SetIntField(env, dpObj, dp_portID, port);
+(*env)->SetIntField(env, dpObj, dp_lengthID, rv);
+}
 }

 if (packetBufferLen > MAX_BUFFER_LEN) {

RFR: JDK8036601 - NI exception pending in jdk/src/windows/native/sun/net/dns/ResolverConfigurationImpl.c

[Mark Sheppard]

Hi
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8036601/webrev/

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

summary:
NULL return value checks and JNU_ThrowOutOfMemoryError added

regards
Mark

Re: RFR: JDK8036601 - NI exception pending in jdk/src/windows/native/sun/net/dns/ResolverConfigurationImpl.c

[Mark Sheppard]

Hi Alan,
   thanks for the reply  yes, I'll add the STS_ERROR

regards
Mark
On 12/03/2014 16:51, Alan Bateman wrote:

On 12/03/2014 16:43, Mark Sheppard wrote:

Hi
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8036601/webrev/

This mostly looks good to me.

One observation is that is the return from loadConfig isn't clear. 
What would you think about adding STS_ERROR (-1) so that it's clear 
from loadConfig and the caller that this is the error case. In the 
original code there ere several status values and the IS_XXX macros 
were used to check them.


-Alan.

RFR: JDK-8036600 - JNI exception pending in src/jdk/src/windows/native/sun/net/www/protocol/http/ntlm/NTLMAuthSequence.c

[Mark Sheppard]

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

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

summary:
main changes are to check JNI return values, malloc returns and raise 
JNU_ThrowOutOfMemoryError

as needed

regards
Mark

RFR: JDK-8035571 - Check jdk/src/windows/native/java/net/TwoStacksPlain* for JNI pending exceptions

[Mark Sheppard]

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

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

summary:
JNI return value and pending exception checks added

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

[Mark Sheppard]

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: RFR: JDK-8035631 - JNI exception pending in jdk/src/windows/native/java/net/NetworkInterface_winXP.c

[Mark Sheppard]

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

RFR: 8044029 - JNI exception pending checks in java.net

[Mark Sheppard]

Hi,
   please oblige and review the following fix
http://cr.openjdk.java.net/~msheppar/8044029/webrev/

for the issue
https://bugs.openjdk.java.net/browse/JDK-8044029

which is a backport of
https://bugs.openjdk.java.net/browse/JDK-8025293

the changeset didn't appear to apply cleanly, necessitating its 
application by hand


regards
Mark

RFR: 8041609 - Check jdk/src/windows/native/java/io/WinNTFileSystem_md.c for JNI pending exceptions

[Mark Sheppard]

Hi
please oblige review and approve the backport to jdk8u-dev, after 
the review of the following change


http://cr.openjdk.java.net/~msheppar/8041609/webrev/

for the issue
https://bugs.openjdk.java.net/browse/JDK-8041609

which is a backport of
https://bugs.openjdk.java.net/browse/JDK-8035870

the hg import  of changeset
http://hg.openjdk.java.net/jdk9/dev/jdk/rev/56366827ebab
didn't apply cleanly so was manually

the regression test run was fine

regards
Mark

RFR: JDK-8040810 - Uninitialised memory in jdk/src/windows/native/java/net: net_util_md.c, TwoStacksPlainSocketImpl.c, TwoStacksPlainDatagramSocketImpl.c, DualStackPlainSocketImpl.c, DualStackPlainDa

[Mark Sheppard]

Hi
please oblige and review the following changes

http://cr.openjdk.java.net/~msheppar/8040810/webrev/

which address the issue raised in

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

resulting from static code analysis.

these changes explicitly initialize local function variables, which are 
in the main
out parameters to other function calls and hence are set within the 
called function.
It can be reasonably argued that the initialization is unnecessary, but 
current coding

guidance is to perform the initialization

regards
Mark

Re: RFR: JDK-8040810 - Uninitialised memory in jdk/src/windows/native/java/net: net_util_md.c, TwoStacksPlainSocketImpl.c, TwoStacksPlainDatagramSocketImpl.c, DualStackPlainSocketImpl.c, DualStackPla

[Mark Sheppard]

HI Alan,
  thanks for the response ... yes,  rv = 0 is isn't strictly necessary 
(in for a penny, in for a pound approach)


regards
Mark

On 16/07/2014 18:28, Alan Bateman wrote:

On 16/07/2014 18:00, Mark Sheppard wrote:

Hi
please oblige and review the following changes

http://cr.openjdk.java.net/~msheppar/8040810/webrev/

which address the issue raised in

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

resulting from static code analysis.

these changes explicitly initialize local function variables, which 
are in the main
out parameters to other function calls and hence are set within the 
called function.
It can be reasonably argued that the initialization is unnecessary, 
but current coding

guidance is to perform the initialization

I assume in NET_Bind that it isn't necessary to initialize rv, same 
thing in NET_SocketClose. Otherwise looks okay.


-Alan

RFR: JDK-8050922 - add additional diagnostic to java/net/MulticastSocket/TestInterfaces.java

[Mark Sheppard]

Hi,
   please oblige and review the following diagnostic output addition to
the test java/net/MulticastSocket/TestInterfaces.java

http://cr.openjdk.java.net/~msheppar/8050922/webrev/

for the task

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

which will assist in diagnosing an intermittent failing test
https://bugs.openjdk.java.net/browse/JDK-8041677

the addition displays the interface details, of the failing scenario,  
to std err


regards
Mark

RFR: JDK-8054118 - java/net/ipv6tests/UdpTest.java failed intermittently

[Mark Sheppard]

Hi,
  please oblige and review the following fix
http://cr.openjdk.java.net/~msheppar/8054118/webrev/

to address the issue raised in
https://bugs.openjdk.java.net/browse/JDK-8054118

The Windows test environment has a Teredo interface.
This oscillates between being configured with IPv6 and not been configured.
The configured state is transient. When configured its IPv6 is retrieved as
first local ipv6 address. As such, it can happen that during the test the
interface is disabled, and the test will fail with the report Socket 
receive timeout.


This fix removes the Teredo interface from the testing scenario, so that 
a more stable interface

will be used.

regards
Mark

Re: RFR: JDK-8054118 - java/net/ipv6tests/UdpTest.java failed intermittently

[Mark Sheppard]

thanks Chris

On 07/08/2014 19:20, Chris Hegarty wrote:

Thanks Mark, Reviewed.

-Chris.

P.S. At some point in the future, we should try to create a general test 
utility library to retrieve suitable network interfaces for tests to use.

On 7 Aug 2014, at 19:15, Mark Sheppard  wrote:


Hi,
  please oblige and review the following fix
http://cr.openjdk.java.net/~msheppar/8054118/webrev/

to address the issue raised in
https://bugs.openjdk.java.net/browse/JDK-8054118

The Windows test environment has a Teredo interface.
This oscillates between being configured with IPv6 and not been configured.
The configured state is transient. When configured its IPv6 is retrieved as
first local ipv6 address. As such, it can happen that during the test the
interface is disabled, and the test will fail with the report Socket receive 
timeout.

This fix removes the Teredo interface from the testing scenario, so that a more 
stable interface
will be used.

regards
Mark

Re: RFR: JDK-8054118 - java/net/ipv6tests/UdpTest.java failed intermittently

[Mark Sheppard]

grand ... thanks Dmitry

Mark

On 07/08/2014 19:25, Dmitry Samersoff wrote:

Mark,

We can put nic.getDisplayName() under isWindows and save a bit of
computer power.

-Dmitry


On 2014-08-07 22:15, Mark Sheppard wrote:

Hi,
   please oblige and review the following fix
http://cr.openjdk.java.net/~msheppar/8054118/webrev/

to address the issue raised in
https://bugs.openjdk.java.net/browse/JDK-8054118

The Windows test environment has a Teredo interface.
This oscillates between being configured with IPv6 and not been configured.
The configured state is transient. When configured its IPv6 is retrieved as
first local ipv6 address. As such, it can happen that during the test the
interface is disabled, and the test will fail with the report Socket
receive timeout.

This fix removes the Teredo interface from the testing scenario, so that
a more stable interface
will be used.

regards
Mark

RFR: JDK-8035571 - Check jdk/src/windows/native/java/net/TwoStacksPlain* for JNI pending exceptions

[Mark Sheppard]

Hi,
   please oblige and review the updated webrev
http://cr.openjdk.java.net/~msheppar/8035571/webrev.02/

for the issue
https://bugs.openjdk.java.net/browse/JDK-8035571

this is applying the previous un-reviewed patch to the updated
jdk9 modular src structure.

regards
Mark

 Original Message 
Subject: 	RFR: JDK-8035571 - Check 
jdk/src/windows/native/java/net/TwoStacksPlain* for JNI pending exceptions

Date:   Thu, 13 Mar 2014 20:00:30 +
From:   Mark Sheppard 
Organization:   Oracle Corporation
To: OpenJDK Network Dev list 



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

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

summary:
JNI return value and pending exception checks added

RFR: JDK-8044306 - java.net.Inet4AddressImpl.getLocalHostName() uses IPv6 rather than IPv4

[Mark Sheppard]

Hi
  please oblige and review the small change
http://cr.openjdk.java.net/~msheppar/8044306/webrev/

to address the issue raised in
https://bugs.openjdk.java.net/browse/JDK-8044306

hints.ai_family is assigned AF_INET rather than AF_UNSPEC
as the native function
Java_java_net_Inet4AddressImpl_getLocalHostName
is part of an IPv4 call flow, such that AF_UNSPEC allows
an IPv6 data to be retrieved as highlighted in bug report

regards
Mark

RFR: JDK-8058932 - java/net/InetAddress/IPv4Formats.java failed because hello.foo.bar does exist

[Mark Sheppard]

Hi

Please oblige and review the following small change to test 
test/java/net/InetAddress/IPv4Formats.java


--- a/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30 
13:25:04 2014 +0100
+++ b/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30 
15:11:05 2014 +0100

@@ -36,7 +36,7 @@
 {"126.1", "126.0.0.1"},
 {"128.50.65534", "128.50.255.254"},
 {"192.168.1.2", "192.168.1.2"},
-{"hello.foo.bar", null},
+{"somehost.some-domain", null},
 {"1024.1.2.3", null},
 {"128.14.66000", null }

which addresses the issue

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

ping hello.foo.bar

Pinging hello.foo.bar [127.0.53.53] with 32 bytes of data:
Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
Reply from 127.0.53.53: bytes=32 time<1ms TTL=128

this highlights a DNS configuration issue as indicated in
https://www.icann.org/resources/pages/name-collision-2013-12-06-en

so we remove foo.bar from the test and replace with somehost.some-domain

regards
Mark

Re: RFR: JDK-8058932 - java/net/InetAddress/IPv4Formats.java failed because hello.foo.bar does exist

[Mark Sheppard]

thanks Daniel Alan

it was actually the domain foo.bar that proved to be a TLD for some 
reason and  resolved

to the special address 127.0.53.53, indicating a DNS problem.

so we could use your strategy on the domain part of the pseudo FQDN, and 
a bit like

New York New York, we pseudo random name it twice ?

 {"x-" + UUID.randomUUID().toString() + "-x.x-" + 
UUID.randomUUID().toString() + "-x", null},


regards
Mark

On 30/09/2014 16:47, Daniel Fuchs wrote:

On 30/09/14 17:31, Alan Bateman wrote:

On 30/09/2014 08:21, Mark Sheppard wrote:

Hi

Please oblige and review the following small change to test
test/java/net/InetAddress/IPv4Formats.java

--- a/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
13:25:04 2014 +0100
+++ b/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
15:11:05 2014 +0100
@@ -36,7 +36,7 @@
 {"126.1", "126.0.0.1"},
 {"128.50.65534", "128.50.255.254"},
 {"192.168.1.2", "192.168.1.2"},
-{"hello.foo.bar", null},
+{"somehost.some-domain", null},
 {"1024.1.2.3", null},
 {"128.14.66000", null }

This looks okay to me, at least until somehost.some-domain starts to be
resolved to some address :-)


I wonder: would something like

  "x-" + UUID.randomUUID().toString() + "-x.some-domain"

result in a syntactically valid address? If so it might
reduce the chances of collision...

best regards,

-- daniel



-Alan

Re: RFR: JDK-8058932 - java/net/InetAddress/IPv4Formats.java failed because hello.foo.bar does exist

[Mark Sheppard]

thanks Chris ... so shall we go with the simplest thing that works :-)
i.e. somehost.some-domain  ?

M.

On 30/09/2014 17:35, Chris Hegarty wrote:


On 30 Sep 2014, at 08:47, Daniel Fuchs <mailto:daniel.fu...@oracle.com>> wrote:



On 30/09/14 17:31, Alan Bateman wrote:

On 30/09/2014 08:21, Mark Sheppard wrote:

Hi

Please oblige and review the following small change to test
test/java/net/InetAddress/IPv4Formats.java

--- a/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
13:25:04 2014 +0100
+++ b/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
15:11:05 2014 +0100
@@ -36,7 +36,7 @@
{"126.1", "126.0.0.1"},
{"128.50.65534", "128.50.255.254"},
{"192.168.1.2", "192.168.1.2"},
-{"hello.foo.bar", null},
+{"somehost.some-domain", null},
{"1024.1.2.3", null},
{"128.14.66000", null }

This looks okay to me, at least until somehost.some-domain starts to be
resolved to some address :-)


+1


I wonder: would something like

 "x-" + UUID.randomUUID().toString() + "-x.some-domain"

result in a syntactically valid address? If so it might
reduce the chances of collision…


The collision here is as a result of the top-level domain, so I’m not 
sure it is necessary to “randomize” the fully qualified domain name.


-Chris.



best regards,

-- daniel



-Alan

Re: RFR: JDK-8058932 - java/net/InetAddress/IPv4Formats.java failed because hello.foo.bar does exist

[Mark Sheppard]

Thanks Tom and Dmitry

last up best dressed ...

.invalid as the test domain is  a good recommendation

change is now

--- a/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30 
13:25:04 2014 +0100
+++ b/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30 
23:23:46 2014 +0100

@@ -36,7 +36,7 @@
 {"126.1", "126.0.0.1"},
 {"128.50.65534", "128.50.255.254"},
 {"192.168.1.2", "192.168.1.2"},
-{"hello.foo.bar", null},
+{"invalidhost.invalid", null},
 {"1024.1.2.3", null},
 {"128.14.66000", null }


regards
Mark


On 30/09/2014 18:53, Dmitry Samersoff wrote:

Mark,

It probably should be some-name.invalid

IANA reserve .invalid TLD for tests like this one

see:

http://www.iana.org/assignments/special-use-domain-names/special-use-domain-names.xhtml

-Dmitry

On 2014-09-30 19:21, Mark Sheppard wrote:

Hi

Please oblige and review the following small change to test
test/java/net/InetAddress/IPv4Formats.java

--- a/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
13:25:04 2014 +0100
+++ b/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
15:11:05 2014 +0100
@@ -36,7 +36,7 @@
  {"126.1", "126.0.0.1"},
  {"128.50.65534", "128.50.255.254"},
  {"192.168.1.2", "192.168.1.2"},
-{"hello.foo.bar", null},
+{"somehost.some-domain", null},
  {"1024.1.2.3", null},
  {"128.14.66000", null }

which addresses the issue

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

ping hello.foo.bar

Pinging hello.foo.bar [127.0.53.53] with 32 bytes of data:
Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
Reply from 127.0.53.53: bytes=32 time<1ms TTL=128

this highlights a DNS configuration issue as indicated in
https://www.icann.org/resources/pages/name-collision-2013-12-06-en

so we remove foo.bar from the test and replace with somehost.some-domain

regards
Mark

Re: RFR: JDK-8058932 - java/net/InetAddress/IPv4Formats.java failed because hello.foo.bar does exist

[Mark Sheppard]

thanks Chris

On 01/10/2014 15:58, Chris Hegarty wrote:

Reviewed.

-Chris.

On 1 Oct 2014, at 00:01, Dmitry Samersoff  wrote:


Looks good for me!

-Dmitry

On 2014-10-01 02:26, Mark Sheppard wrote:

Thanks Tom and Dmitry

last up best dressed ...

.invalid as the test domain is  a good recommendation

change is now

--- a/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
13:25:04 2014 +0100
+++ b/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
23:23:46 2014 +0100
@@ -36,7 +36,7 @@
 {"126.1", "126.0.0.1"},
 {"128.50.65534", "128.50.255.254"},
 {"192.168.1.2", "192.168.1.2"},
-{"hello.foo.bar", null},
+{"invalidhost.invalid", null},
 {"1024.1.2.3", null},
 {"128.14.66000", null }


regards
Mark


On 30/09/2014 18:53, Dmitry Samersoff wrote:

Mark,

It probably should be some-name.invalid

IANA reserve .invalid TLD for tests like this one

see:

http://www.iana.org/assignments/special-use-domain-names/special-use-domain-names.xhtml


-Dmitry

On 2014-09-30 19:21, Mark Sheppard wrote:

Hi

Please oblige and review the following small change to test
test/java/net/InetAddress/IPv4Formats.java

--- a/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
13:25:04 2014 +0100
+++ b/test/java/net/InetAddress/IPv4Formats.javaTue Sep 30
15:11:05 2014 +0100
@@ -36,7 +36,7 @@
  {"126.1", "126.0.0.1"},
  {"128.50.65534", "128.50.255.254"},
  {"192.168.1.2", "192.168.1.2"},
-{"hello.foo.bar", null},
+{"somehost.some-domain", null},
  {"1024.1.2.3", null},
  {"128.14.66000", null }

which addresses the issue

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

ping hello.foo.bar

Pinging hello.foo.bar [127.0.53.53] with 32 bytes of data:
Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
Reply from 127.0.53.53: bytes=32 time<1ms TTL=128
Reply from 127.0.53.53: bytes=32 time<1ms TTL=128

this highlights a DNS configuration issue as indicated in
https://www.icann.org/resources/pages/name-collision-2013-12-06-en

so we remove foo.bar from the test and replace with somehost.some-domain

regards
Mark


--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR: JDK-8015692 - java.net.BindException is thrown on Windows XP when HTTP server is started and stopped in the loop.

[Mark Sheppard]

Hi
Please oblige and review the updated patch
http://cr.openjdk.java.net/~msheppar/8015692/webrev.02a/

to address the issue
https://bugs.openjdk.java.net/browse/JDK-8015692

as suggested the ServerImpl.stop() method performs a join, only, for the 
dispatcher thread

so that all housekeeping is done prior to exiting stop.

regards
Mark

On 24/02/2014 17:08, Chris Hegarty wrote:

Hi Mark,

I think join should be sufficient here.

I understand your argument to move selector close into stop, but that 
just seems to require extra co-ordination between stop and the 
dispatcher loop, namely you now need to check if the selector is 
closed in a few places. I think it is simpler to leave the original 
code as is, dispatcher closes the selector, and only selectNow is 
invoked from stop. Or maybe I'm missing something.


-Chris.

On 21/02/14 12:21, Mark Sheppard wrote:

Hi Chris,
thanks for the response.

Yes, that's true.
It was just the way it evolved as I analyzed the issue.
Originally, the join was after the close and selectNow.
The close was moved from Dispatcher to stop, as there was some
"interplay" between the Dispatcher thread
and the stop thread, when the Dispatcher was invoking the close.
Then added the join() in the stop method, to ensure that the Dispatcher
wasn't still executing after the server had been
stopped.

As the Selector is opened in the ServerImpl constructor and not in the
Dispatcher, it seemed
from a symmetry view point more logical  to invoke the close in the
ServerImpl stop

The selectNow is just insurance for cleanup purposes.

It is possible that the join should be higher up in the stop() flow i.e.
immediately after the
setting the finish flag?
As such, the Dispatcher should be finished with the various
HttpConnection collections, before
the stop processes them.

regards
Mark

On 21/02/2014 07:22, Chris Hegarty wrote:

Mark,

I agree with you, there is certainly some additional co-ordination
needed between the thread invoking the stop method and the dispatcher
thread.

I wonder why you needed to add the selectNow() and the close() after
you have joined the dispatcher thread? Since you are guaranteed that
the dispatcher thread will have exited before join() returns?

-Chris.

On 17 Feb 2014, at 01:20, Mark Sheppard  
wrote:



Hi

Please oblige and review the changes in the webrev

http://cr.openjdk.java.net/~msheppar/8015692/jdk9/webrev/

to address the issue raised in the bug

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

Summary:
a series Junit tests which start stop instances of an
com.sun.net.httpserver.HttpServer failed due to
java.net.BindException: Address already in use: bind

This was raised against Windows XP, but the sample test to reproduce
the issue
was run on Windows 7, and the problem was seen to occur on this OS 
also.

The sample was run against jdk7, jdk8 and jdk9: reproducible on each.

On investigation it  appears that some additional co-ordination is
required between the
HttpServer's  (actually SereverImpl) dispatcher thread and the thread
invoking the stop
method. This change has amended the stop method to wait for the
Dispatcher thread to complete, then
invokes the selector's selectNow, to handled cancelled events, and
closes the selector.
The selector.close() has been removed from the Dispatcher's run 
method.


regards
Mark

Re: RFR: JDK-8015692 - java.net.BindException is thrown on Windows XP when HTTP server is started and stopped in the loop.

[Mark Sheppard]

Thanks Chris  I'll make the changes to ServerImpl and will remove 
the System.out from the test


regards
Mark
On 13/11/2014 10:49, Chris Hegarty wrote:

On 12 Nov 2014, at 22:12, Mark Sheppard  wrote:


Hi
Please oblige and review the updated patch
http://cr.openjdk.java.net/~msheppar/8015692/webrev.02a/

The source change looks fine to me.  Just a comment/question; since you are 
swallowing the IE I think you should restore the interrupt status on the thread.

 try {
 dispatcherThread.join();
 } catch (InterruptedException e) {
 Thread.currentThread().interrupt(); <<<< HERE
 logger.log(Level.FINER, "ServerImpl.stop: ", e);
 }

The test could be simplified somewhat by creating one HttpServer, getting its 
port (getServerAddress()) , and then re-using that port.  Maybe a little less 
#’s in the output ? ;-)

-Chris.


to address the issue
https://bugs.openjdk.java.net/browse/JDK-8015692

as suggested the ServerImpl.stop() method performs a join, only, for the 
dispatcher thread
so that all housekeeping is done prior to exiting stop.

regards
Mark

On 24/02/2014 17:08, Chris Hegarty wrote:

Hi Mark,

I think join should be sufficient here.

I understand your argument to move selector close into stop, but that just 
seems to require extra co-ordination between stop and the dispatcher loop, 
namely you now need to check if the selector is closed in a few places. I think 
it is simpler to leave the original code as is, dispatcher closes the selector, 
and only selectNow is invoked from stop. Or maybe I'm missing something.

-Chris.

On 21/02/14 12:21, Mark Sheppard wrote:

Hi Chris,
thanks for the response.

Yes, that's true.
It was just the way it evolved as I analyzed the issue.
Originally, the join was after the close and selectNow.
The close was moved from Dispatcher to stop, as there was some
"interplay" between the Dispatcher thread
and the stop thread, when the Dispatcher was invoking the close.
Then added the join() in the stop method, to ensure that the Dispatcher
wasn't still executing after the server had been
stopped.

As the Selector is opened in the ServerImpl constructor and not in the
Dispatcher, it seemed
from a symmetry view point more logical  to invoke the close in the
ServerImpl stop

The selectNow is just insurance for cleanup purposes.

It is possible that the join should be higher up in the stop() flow i.e.
immediately after the
setting the finish flag?
As such, the Dispatcher should be finished with the various
HttpConnection collections, before
the stop processes them.

regards
Mark

On 21/02/2014 07:22, Chris Hegarty wrote:

Mark,

I agree with you, there is certainly some additional co-ordination
needed between the thread invoking the stop method and the dispatcher
thread.

I wonder why you needed to add the selectNow() and the close() after
you have joined the dispatcher thread? Since you are guaranteed that
the dispatcher thread will have exited before join() returns?

-Chris.

On 17 Feb 2014, at 01:20, Mark Sheppard  wrote:


Hi

Please oblige and review the changes in the webrev

http://cr.openjdk.java.net/~msheppar/8015692/jdk9/webrev/

to address the issue raised in the bug

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

Summary:
a series Junit tests which start stop instances of an
com.sun.net.httpserver.HttpServer failed due to
java.net.BindException: Address already in use: bind

This was raised against Windows XP, but the sample test to reproduce
the issue
was run on Windows 7, and the problem was seen to occur on this OS also.
The sample was run against jdk7, jdk8 and jdk9: reproducible on each.

On investigation it  appears that some additional co-ordination is
required between the
HttpServer's  (actually SereverImpl) dispatcher thread and the thread
invoking the stop
method. This change has amended the stop method to wait for the
Dispatcher thread to complete, then
invokes the selector's selectNow, to handled cancelled events, and
closes the selector.
The selector.close() has been removed from the Dispatcher's run method.

regards
Mark

Re: RFR: JDK-8015692 - java.net.BindException is thrown on Windows XP when HTTP server is started and stopped in the loop.

[Mark Sheppard]

Hi
   please oblige and review the updated patch as per Chris' suggestions

http://cr.openjdk.java.net/~msheppar/8015692/webrev.03/

regards
Mark
On 13/11/2014 10:49, Chris Hegarty wrote:

On 12 Nov 2014, at 22:12, Mark Sheppard  wrote:


Hi
Please oblige and review the updated patch
http://cr.openjdk.java.net/~msheppar/8015692/webrev.02a/

The source change looks fine to me.  Just a comment/question; since you are 
swallowing the IE I think you should restore the interrupt status on the thread.

 try {
 dispatcherThread.join();
 } catch (InterruptedException e) {
 Thread.currentThread().interrupt(); <<<< HERE
 logger.log(Level.FINER, "ServerImpl.stop: ", e);
 }

The test could be simplified somewhat by creating one HttpServer, getting its 
port (getServerAddress()) , and then re-using that port.  Maybe a little less 
#’s in the output ? ;-)

-Chris.


to address the issue
https://bugs.openjdk.java.net/browse/JDK-8015692

as suggested the ServerImpl.stop() method performs a join, only, for the 
dispatcher thread
so that all housekeeping is done prior to exiting stop.

regards
Mark

On 24/02/2014 17:08, Chris Hegarty wrote:

Hi Mark,

I think join should be sufficient here.

I understand your argument to move selector close into stop, but that just 
seems to require extra co-ordination between stop and the dispatcher loop, 
namely you now need to check if the selector is closed in a few places. I think 
it is simpler to leave the original code as is, dispatcher closes the selector, 
and only selectNow is invoked from stop. Or maybe I'm missing something.

-Chris.

On 21/02/14 12:21, Mark Sheppard wrote:

Hi Chris,
thanks for the response.

Yes, that's true.
It was just the way it evolved as I analyzed the issue.
Originally, the join was after the close and selectNow.
The close was moved from Dispatcher to stop, as there was some
"interplay" between the Dispatcher thread
and the stop thread, when the Dispatcher was invoking the close.
Then added the join() in the stop method, to ensure that the Dispatcher
wasn't still executing after the server had been
stopped.

As the Selector is opened in the ServerImpl constructor and not in the
Dispatcher, it seemed
from a symmetry view point more logical  to invoke the close in the
ServerImpl stop

The selectNow is just insurance for cleanup purposes.

It is possible that the join should be higher up in the stop() flow i.e.
immediately after the
setting the finish flag?
As such, the Dispatcher should be finished with the various
HttpConnection collections, before
the stop processes them.

regards
Mark

On 21/02/2014 07:22, Chris Hegarty wrote:

Mark,

I agree with you, there is certainly some additional co-ordination
needed between the thread invoking the stop method and the dispatcher
thread.

I wonder why you needed to add the selectNow() and the close() after
you have joined the dispatcher thread? Since you are guaranteed that
the dispatcher thread will have exited before join() returns?

-Chris.

On 17 Feb 2014, at 01:20, Mark Sheppard  wrote:


Hi

Please oblige and review the changes in the webrev

http://cr.openjdk.java.net/~msheppar/8015692/jdk9/webrev/

to address the issue raised in the bug

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

Summary:
a series Junit tests which start stop instances of an
com.sun.net.httpserver.HttpServer failed due to
java.net.BindException: Address already in use: bind

This was raised against Windows XP, but the sample test to reproduce
the issue
was run on Windows 7, and the problem was seen to occur on this OS also.
The sample was run against jdk7, jdk8 and jdk9: reproducible on each.

On investigation it  appears that some additional co-ordination is
required between the
HttpServer's  (actually SereverImpl) dispatcher thread and the thread
invoking the stop
method. This change has amended the stop method to wait for the
Dispatcher thread to complete, then
invokes the selector's selectNow, to handled cancelled events, and
closes the selector.
The selector.close() has been removed from the Dispatcher's run method.

regards
Mark

RFR: JDK-8065222 - sun/net/www/protocol/http/B6369510.java doesn't execute as expected

[Mark Sheppard]

Hi,
   please oblige and review the following change
http://cr.openjdk.java.net/~msheppar/8065222/webrev/

to address the issue
https://bugs.openjdk.java.net/browse/JDK-8065222

The URL construction in the test now uses 
InetAddress.getLocalHost().getHostName(), rather than the
address.getHostName(), which was returning wildcard IP address, causing 
a MalformedURLException.

Test, amended to propagate exception failures, also.

regards
Mark

Re: RFR: JDK-8065222 - sun/net/www/protocol/http/B6369510.java doesn't execute as expected

[Mark Sheppard]

Hi
   minor update to fix, the boolean handlerRunAsExpected flag has been 
removed,

so the fix rectifies the URL construction

http://cr.openjdk.java.net/~msheppar/8065222/webrev.01/

regards
Mark
On 19/11/2014 19:50, Mark Sheppard wrote:

Hi,
   please oblige and review the following change
http://cr.openjdk.java.net/~msheppar/8065222/webrev/

to address the issue
https://bugs.openjdk.java.net/browse/JDK-8065222

The URL construction in the test now uses 
InetAddress.getLocalHost().getHostName(), rather than the
address.getHostName(), which was returning wildcard IP address, 
causing a MalformedURLException.

Test, amended to propagate exception failures, also.

regards
Mark

Re: RFR: JDK-8065222 - sun/net/www/protocol/http/B6369510.java doesn't execute as expected

[Mark Sheppard]

thanks Chris

M.
On 21/11/2014 22:38, Chris Hegarty wrote:

Looks good, thanks Mark.

-Chris


On 21 Nov 2014, at 22:08, Mark Sheppard  wrote:

Hi
   minor update to fix, the boolean handlerRunAsExpected flag has been removed,
so the fix rectifies the URL construction

http://cr.openjdk.java.net/~msheppar/8065222/webrev.01/

regards
Mark

On 19/11/2014 19:50, Mark Sheppard wrote:
Hi,
   please oblige and review the following change
http://cr.openjdk.java.net/~msheppar/8065222/webrev/

to address the issue
https://bugs.openjdk.java.net/browse/JDK-8065222

The URL construction in the test now uses 
InetAddress.getLocalHost().getHostName(), rather than the
address.getHostName(), which was returning wildcard IP address, causing a 
MalformedURLException.
Test, amended to propagate exception failures, also.

regards
Mark

Re: URLStreamHandler.getHostAddress() performance

[Mark Sheppard]

I think this raises  a more fundamental question, as to why   the URL 
hashCode()  and equals() methods  delegates to URLStreamHandler
in the first place? rather than performing the processing within the URL 
class itself, and synchronizing appropriately within.


If you call equals() and hasCode() concurrently on the same URL 
instance, there exists the possibility (slight) that
the hostAddress could be set differently, when it is being set for the 
first time, without the synchronization of the getHostAddress() in the 
URLStreamHandler.


Although it may rarely happen the mechanics of InetAddress.getByName() 
potentially,  lend itself to a different return address
on multiple calls, as it returns the first address from a array of 
addresses retrieved - assumption is that the ordering will always be the 
same.


regards
Mark

On 25/11/2014 12:58, Wang Weijun wrote:

On Nov 25, 2014, at 20:25, Pavel Rappo  wrote:

Hi Max,

I don't see any particular reason for this. Maybe it's just a "precaution". It 
seems to me it's the only field
of the URL class set directly (without setter) from an outside.

Does it hurt performance a lot? In what cases?

There is a 7x difference in my experiment on SecureClassLoader.defineClass().

Or, it can be shown using this benchmark:

package org.openjdk.bench;

import org.openjdk.jmh.annotations.*;

import java.io.IOException;
import java.net.URL;
import java.util.*;
import java.util.concurrent.ConcurrentHashMap;

@State(Scope.Benchmark)
public class Weird {
 final int num = 100;
 final Object[] urls = new Object[num];

 public static class CS {
 private final Object url;
 public CS(Object url) { this.url = url; }
 public int hashCode() { return url.hashCode(); }
 public boolean equals(Object o) { return o instanceof CS && 
url.equals(((CS)o).url); }
 }

 private final Map pdcacheC = new ConcurrentHashMap<>();

 @Setup
 public void init() throws Exception {
 for (int i=0; i< num; i++) {
 urls[i] = new URL("file:/tmp/"+i);
 }
 }

 @State(Scope.Thread)
 public static class ThreadState {
 final Random rand = new Random();
 }


 @Benchmark
 public void setC(ThreadState state) throws IOException {
 Object cs = new CS(urls[next(state)]);
 pdcacheC.computeIfAbsent(cs, x -> "");
 }

 private int next(ThreadState state) {
 return state.rand.nextInt(num);
 }
}

--Max



-Pavel


On 25 Nov 2014, at 12:02, Wang Weijun  wrote:

I am benchmarking security manager and notice this

protected synchronized InetAddress getHostAddress(URL u) {
   if (u.hostAddress != null)
   return u.hostAddress;

   String host = u.getHost();
   if (host == null || host.equals("")) {
   return null;
   } else {
   try {
   u.hostAddress = InetAddress.getByName(host);
   } catch (UnknownHostException ex) {
   return null;
   } catch (SecurityException se) {
   return null;
   }
   }
   return u.hostAddress;
}

Is there any reason why the method is synchronized? Why not simply "synchronized 
(u)"?

Thanks
Max

RFR: JDK-8066130 - com.sun.net.httpserver stop() throws NullPointerException if it is not started

[Mark Sheppard]

Hi,
   Please oblige and review the minor change
http://cr.openjdk.java.net/~msheppar/8066130/webrev/

to address the issue

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

a previous amendment to com.sun.net.httpserver.ServerImpl ( 
https://bugs.openjdk.java.net/browse/JDK-8015692)
failed to consider that a stop method may be invoked prior to a start 
method, hence assumed dispatcherThread would be set,
null check on disptacherThread added to stop 
com.sun.net.httpserver.ServerImpl stop method


regards
Mark

RFR: JDK-8039595 - closed/java/net/DatagramPacket/CheckInetAddress.java fails on macosx

[Mark Sheppard]

Hi
please oblige and review the following change

http://cr.openjdk.java.net/~msheppar/8039595/webrev/

which addresses the failures raised in

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

it should also address the CheckInetAddress test failures

https://bugs.openjdk.java.net/browse/JDK-6924602 - TEST_BUG: 
CheckInetAddress.java sometimes fails because it uses wrong address
https://bugs.openjdk.java.net/browse/JDK-8028680 - 
closed/java/net/DatagramPacket/CheckInetAddress.java fails in azure


investigation of the issue saw the problem occur on multi-homed network 
configurations.
In some circumstances there was no connectivity between the address 
chosen for the client datagram socket, and

the address used for the server datagram socket.
In macos case the test was selecting a temporary address that was 
deprecated, therefore not usable

as an initiating socket address.

The purpose of the test is to determine that a server DatagramPacket can 
be re-used in a receive call and
obtain the relevant sender address from the datagram. The server socket 
is bound to a wild card address.
As such, the test now uses InetAddress.getLocalHost() in both the send 
datagram packet address and for the
client's DatagramSocket to ensure connectivity with the server 
DatagramSocket.


regards
Mark

RFR: JDK-8068597 - Add error code to to exception condition message resulting from GetAdaptersAddresses function calls

[Mark Sheppard]

Hi
please oblige and review the following additional code to
src/java.base/windows/native/libnet/NetworkInterface_winXP.c

webrev: http://cr.openjdk.java.net/~msheppar/8068597/webrev/

JBS item: https://bugs.openjdk.java.net/browse/JDK-8068597

which is a temporary addition to assist in the investigation and diagnosis
of a problem on Windows in the NetworkInterface.getNetworInterfaces() 
call flow,

as highlighted in https://bugs.openjdk.java.net/browse/JDK-8065078

There are intermittent failures of the  GetAdaptersAddresses Windows API 
function call.

The extra diagnostic adds the error code to the Exception message thrown.

regards
Mark

Re: RFR: JDK-8068597 - Add error code to to exception condition message resulting from GetAdaptersAddresses function calls

[Mark Sheppard]

thanks Chris

regards
Mark
On 08/01/2015 14:29, Chris Hegarty wrote:

Reviewed.

As a temporary measure to get more diagnostic information on a very difficult 
intermittent failure, 8065078, I agree with this change. Once we get the 
additional diagnostic information, then we can revisit this code.

-Chris.

On 8 Jan 2015, at 14:16, Mark Sheppard  wrote:


Hi
please oblige and review the following additional code to
src/java.base/windows/native/libnet/NetworkInterface_winXP.c

webrev: http://cr.openjdk.java.net/~msheppar/8068597/webrev/

JBS item: https://bugs.openjdk.java.net/browse/JDK-8068597

which is a temporary addition to assist in the investigation and diagnosis
of a problem on Windows in the NetworkInterface.getNetworInterfaces() call flow,
as highlighted in https://bugs.openjdk.java.net/browse/JDK-8065078

There are intermittent failures of the  GetAdaptersAddresses Windows API 
function call.
The extra diagnostic adds the error code to the Exception message thrown.

regards
Mark

RFR: JDK-8046893 - JNI exception pending in jdk/src/solaris/native/java/net: ExtendedOptionsImpl.c, PlainDatagramSocketImpl.c

[Mark Sheppard]

Hi
   please review the following small change
http://cr.openjdk.java.net/~msheppar/8046893/webrev/

which addresses the parfait issue in
https://bugs.openjdk.java.net/browse/JDK-8046893

regards
Mark

RFR: JDK-8046901 - Check jdk/src/solaris/native/sun/nio for Parfait flagged uninitialized memory

[Mark Sheppard]

Hi
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8046901/webrev/

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

pertaining to uninitialized memory in
src/java.base/unix/native/libnio/ch/DatagramChannelImpl.c
src/java.base/unix/native/libnio/ch/ServerSocketChannelImpl.c
src/jdk.sctp/unix/native/libsctp/SctpNet.c


regards
Mark

Re: RFR: JDK-8046901 - Check jdk/src/solaris/native/sun/nio for Parfait flagged uninitialized memory

[Mark Sheppard]

Hi Alan,
   that's correct it's not an issue (false positive) as port is an out 
parameter, but the

prescription was to initialize any flagged variables.

regards
Mark

On 18/02/2015 15:37, Alan Bateman wrote:

On 18/02/2015 15:30, Mark Sheppard wrote:

Hi
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8046901/webrev/

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

pertaining to uninitialized memory in
src/java.base/unix/native/libnio/ch/DatagramChannelImpl.c
src/java.base/unix/native/libnio/ch/ServerSocketChannelImpl.c
src/jdk.sctp/unix/native/libsctp/SctpNet.c
Is there really a bug here? NET_SockaddrToInetAddress is called with 
&port which sets the port.


-Alan.

RFR: JDK-8065078 - NetworkInterface.getNetworkInterfaces() triggers intermittent test failures

[Mark Sheppard]

Hi
   please oblige and review the following small change
http://cr.openjdk.java.net/~msheppar/8065078/webrev/

to address the issue raised in
https://bugs.openjdk.java.net/browse/JDK-8065078

the getAdapters and getAdpater functions were amended in
to add additional diagnostic of the error code, in the event of a
failure, following the second invocation of GetAdpatersAddresses in the 
call flows.

This has shown that the  ERROR_BUFFER_OVERFLOW (i.e. 111)
can occur even though the buffer, to hold the addresses, has been realloced
to the size as per that returned in the variable len
from the initial GetAdapatersAddresses. Thus, it is possible to have a 
"race condition" where

configuration changes are taking place simultaneously to
the NetworkInterface.getNetworkInterfaces() call

The proposed change increases the initial size of the addresses buffer 
to 4096,
and in the event of a failure increases the returned buffer size by this 
amount again.


Additionally, the update of the static variable bufsize, after the 
realloc, was seen as potentially problematic
and has thread safe implications, so these updates have been removed. 
The shared variable is not protected

by a mutex during read and write of variable.

regards
Mark

Re: RFR: JDK-8065078 - NetworkInterface.getNetworkInterfaces() triggers intermittent test failures

[Mark Sheppard]

thanks for the review Chris

regards
Mark
On 05/03/2015 14:09, Chris Hegarty wrote:

On 4 Mar 2015, at 21:53, Mark Sheppard  wrote:


Hi
   please oblige and review the following small change
http://cr.openjdk.java.net/~msheppar/8065078/webrev/

I agree with the increased buffer size, and the strategy you have employed. I 
think you just need to remove a few comments, otherwise looks fine to me.

-Chris.


to address the issue raised in
https://bugs.openjdk.java.net/browse/JDK-8065078

the getAdapters and getAdpater functions were amended in
to add additional diagnostic of the error code, in the event of a
failure, following the second invocation of GetAdpatersAddresses in the call 
flows.
This has shown that the  ERROR_BUFFER_OVERFLOW (i.e. 111)
can occur even though the buffer, to hold the addresses, has been realloced
to the size as per that returned in the variable len
from the initial GetAdapatersAddresses. Thus, it is possible to have a "race 
condition" where
configuration changes are taking place simultaneously to
the NetworkInterface.getNetworkInterfaces() call

The proposed change increases the initial size of the addresses buffer to 4096,
and in the event of a failure increases the returned buffer size by this amount 
again.

Additionally, the update of the static variable bufsize, after the realloc, was 
seen as potentially problematic
and has thread safe implications, so these updates have been removed. The 
shared variable is not protected
by a mutex during read and write of variable.

regards
Mark

RFR: JDK-8041677 - java/net/MulticastSocket/TestInterfaces failed on Oracle VM Virtual Ethernet Adapter

[Mark Sheppard]

Hi
  please oblige and review the following change
http://cr.openjdk.java.net/~msheppar/8041677/webrev/

which addresses the issue
https://bugs.openjdk.java.net/browse/JDK-8041677

analysis has shown that the Teredo interfaces causes issue, due to its 
dynamic configuration, when included in
the test. A conditional statement excludes Teredo from the latter part 
of the test, so

the conditional is moved to the start of the while loop

regards
Mark

RFR: JDK-8077377 - java/net/MulticastSocket/SetOutgoingIf.java fails intermittently with NullPointerException

[Mark Sheppard]

Hi
   please oblige and review the following change
http://cr.openjdk.java.net/~msheppar/8077377/webrev/

to address the issue
https://bugs.openjdk.java.net/browse/JDK-8077377

it is possible that "stray" packets can be received in this 
MulticastSocket test, and
this results in a NPE from the NetworkInterface lookup based on the 
received address.

Change adds a test that the "from" NetworkInterface is not null.

regards
Mark

Re: RFR: JDK-8041677 - java/net/MulticastSocket/TestInterfaces failed on Oracle VM Virtual Ethernet Adapter

[Mark Sheppard]

Thanks Alan

On 26/05/2015 20:34, Alan Bateman wrote:

On 26/05/2015 20:21, Mark Sheppard wrote:

Hi
  please oblige and review the following change
http://cr.openjdk.java.net/~msheppar/8041677/webrev/

which addresses the issue
https://bugs.openjdk.java.net/browse/JDK-8041677

analysis has shown that the Teredo interfaces causes issue, due to 
its dynamic configuration, when included in
the test. A conditional statement excludes Teredo from the latter 
part of the test, so

the conditional is moved to the start of the while loop

This looks okay to me.

-Alan

Re: RFR: JDK-8077377 - java/net/MulticastSocket/SetOutgoingIf.java fails intermittently with NullPointerException

[Mark Sheppard]

Hi Alan,
yes, that's about it. The test is just logging that expected 
interface is not equal to from.

So ignoring the stray packet seems reasonable and overcomes the NPE.
We could  log the fact that a stray packet has been received

if (from != null) {
if (!from.equals(shouldbe)) {
System.out.println("Packets on group "
+ group + " should come from "
+ shouldbe.getName() + ", but came from 
"
+ from.getName());
//throw new RuntimeException("Test failed.");
}
} else {
System.out.println("Unexpected packet received from  " + 
packet.getAddress());
}

I can remove the commented throw

regards
Mark


On 26/05/2015 20:43, Alan Bateman wrote:

On 26/05/2015 20:34, Mark Sheppard wrote:

Hi
   please oblige and review the following change
http://cr.openjdk.java.net/~msheppar/8077377/webrev/

to address the issue
https://bugs.openjdk.java.net/browse/JDK-8077377

it is possible that "stray" packets can be received in this 
MulticastSocket test, and
this results in a NPE from the NetworkInterface lookup based on the 
received address.

Change adds a test that the "from" NetworkInterface is not null.
If I read this correctly then it just ignores the packet when it comes 
from a non-local interface (or interference from something else on the 
network).


Should the commented out throwing on RuntimeException be removed while 
you are there?


-Alan.

Re: RFR: JDK-8041677 - java/net/MulticastSocket/TestInterfaces failed on Oracle VM Virtual Ethernet Adapter

[Mark Sheppard]

thanks Chris

On 26/05/2015 22:12, Chris Hegarty wrote:

Looks fine.

Not for now, but sometime we should consider adding a utility to the test 
library to retrieve an appropriate set of network interfaces suitable for use 
in tests ( rather than patching all these tests individually ).

-Chris.


On 26 May 2015, at 20:21, Mark Sheppard  wrote:

Hi
  please oblige and review the following change
http://cr.openjdk.java.net/~msheppar/8041677/webrev/

which addresses the issue
https://bugs.openjdk.java.net/browse/JDK-8041677

analysis has shown that the Teredo interfaces causes issue, due to its dynamic 
configuration, when included in
the test. A conditional statement excludes Teredo from the latter part of the 
test, so
the conditional is moved to the start of the while loop

regards
Mark

RFR: JDK-8129507 - sun/net/www/protocol/http/B6369510.java fails intermittently

[Mark Sheppard]

Hi
   Please oblige and review the change below
which addresses the issue
https://bugs.openjdk.java.net/browse/JDK-8129507

This amends the url.openConnection() to take the Proxy.NO_PROXY argument,
so that a direct connection is made to test http server, thus bypassing 
any configured and enabled

http proxies on mac OS.

regards
Mark

-- changeset ---

diff -r 9a66ca9b7e36 test/sun/net/www/protocol/http/B6369510.java
--- a/test/sun/net/www/protocol/http/B6369510.java  Tue Jun 23 
14:20:59 2015 -0700
+++ b/test/sun/net/www/protocol/http/B6369510.java  Wed Jun 24 
13:00:49 2015 +0100

@@ -63,7 +63,7 @@

 // GET Request
 URL url = new URL("http://"; + 
InetAddress.getLocalHost().getHostName() + ":" + address.getPort() + 
"/test/");

-HttpURLConnection uc = (HttpURLConnection)url.openConnection();
+HttpURLConnection uc = 
(HttpURLConnection)url.openConnection(Proxy.NO_PROXY);

 int resp = uc.getResponseCode();
 if (resp != 200)
 throw new RuntimeException("Failed: Response code from 
GET is not 200 RSP == " + resp);

@@ -71,7 +71,7 @@
 System.out.println("Response code from GET = 200 OK");

 //POST Request
-uc = (HttpURLConnection)url.openConnection();
+uc = (HttpURLConnection)url.openConnection(Proxy.NO_PROXY);
 uc.setDoOutput(true);
 uc.setRequestMethod("POST");
 OutputStream os = uc.getOutputStream();

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

[Mark Sheppard]

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

in this s is being duped onto fd, and part of the dup2 operation is the 
closing of fd, but
what's is the expected state of file descriptor fd in the event of a 
dup2 failure?


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

and closed ? is it still considered a valid fd?

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

at this stage can it still be considered valid?
should valid() return false?

regards
Mark

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

Thanks!

It looks good to me now.

Sincerely yours,
Ivan

On 07.09.2015 16:08, Vyom Tewari wrote:

Hi All,

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

Thanks,
Vyom


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

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

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

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

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

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


-Alan.

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

[Mark Sheppard]

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

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


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

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





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

On 7 Sep 2015, at 17:41, Mark Sheppard  wrote:


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

in this s is being duped onto fd, and part of the dup2 operation is the closing 
of fd, but
what's is the expected state of file descriptor fd in the event of a dup2 
failure?

I’m not sure that the documentation for dup2 gives us enough detail here. The 
only situation I can see where fd would not be closed is when EBADF is 
returned. Should not be an issue here.


s is closed in any case, but what about fd, should it be attended to if dup2 < 0
and closed ? is it still considered a valid fd?

what can be said about the state of the encapsulating FileDescriptor associated 
with fd?
at this stage can it still be considered valid?
should valid() return false?

Probably, but may not be worth bothering with unless there are operations that 
can access it after this method throws.

-Chris.


regards
Mark

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

Thanks!

It looks good to me now.

Sincerely yours,
Ivan

On 07.09.2015 16:08, Vyom Tewari wrote:

Hi All,

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

Thanks,
Vyom


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

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

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

Bug:
 JDK-8080402 : File Leak in 
jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Webrev:
http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/

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

close isn't restartable so I think we need to look at that while we are here.

-Alan.

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

[Mark Sheppard]

Hi Vyom,
yes, I believe the consensus is to proceed with the changes.

regards
Mark

On 09/09/2015 16:23, Vyom Tewari wrote:

Hi Mark,

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


Thanks,
Vyom


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

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

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


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

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





On 08/09/2015 09:28, Chris Hegarty wrote:
On 7 Sep 2015, at 17:41, Mark Sheppard  
wrote:



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

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


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

and closed ? is it still considered a valid fd?

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

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


-Chris.


regards
Mark

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

Thanks!

It looks good to me now.

Sincerely yours,
Ivan

On 07.09.2015 16:08, Vyom Tewari wrote:

Hi All,

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

Thanks,
Vyom


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

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

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

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

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

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


-Alan.

Re: RFR 4906983: java.net.URL constructors throw MalformedURLException in undocumented way

[Mark Sheppard]

Hi
  I don't think the URL string  http://server:-1/path can be considered 
a valid URL as the port value -1 is not legal port value.


However, the URL class doesn't object and throw a MalformedURLException, 
which is something of an
anomaly, caused by the spec allowing a port value of -1 for the 4 arg 
constructor, which maps to
the default value for the associated protocol.  Using an input port 
value of -1 is unnecessary, as the 3 arg constructor
is sufficient to specify a URL with a default port value. It would seem 
more appropriate to encapsulate the

-1 value as an implementation detail.

A further anomaly arises when a subsequent getPort() method is invoked, 
and this returns a value of -1, rather than
the protocol's associated default value. The -1 doesn't seem to activate 
until a connection is made using the URL's details.


a more generalized description for  MalformedURLException  could be 
used, e.g.


if the parsed URL fails to comply with the scheme specific syntax of the 
associated protocol.


regards
Mark



On 10/09/2015 20:32, Sebastian Sickelmann wrote:

Hi,

first thanks to Chris and David for their helpful input . I looked 
through the existing

Testcases and found one that is already testing for negative-port numbers.
So i extended the @bug line with "4906983" which I hope is the right 
solution to do it.


I am with Chris, when he says normally you only have numbers between 1 and
65535 (because many protocols are using tcp). So i changed to 
documentation as

Chris suggested it.

But ports above this "natural" barrier are valid too. It depends on 
the protocol what
to do with the port information. So I also extended the testcase to 
check that their are

valid port numbers also above 65535 and the special -1.

But i asked myself should
new URL("http://server:-1/path";);
be realy a valid URL? What do you think?

Special thanks to David who hosted the new webrev:
http://cr.openjdk.java.net/~dbuck/4906983.1/

-- Sebastian


Am 10.09.2015 um 12:38 schrieb Chris Hegarty:

Another minor comment...

While what you have suggested is not incorrect, I’m afraid it is giving the 
wrong impression about the typical acceptable port ranges. A port of 
Integer.MAX_VALUE is not all that useful, since it typically maps to a TCP port 
number ( but not always ). Maybe just remove the valid values from @param port, 
and add something like the following to MalformedURLException: “.., or the port 
is a negative number other than -1” ?

-Chris.

On 10 Sep 2015, at 11:13, Chris Hegarty  wrote:


On 8 Sep 2015, at 21:01, Sebastian Sickelmann  
wrote:


Hi,

Please find my small patch[1] to javadoc in java.net.URL that adresses
JDK-4906983(javadoc-fix)[2].

I signed the SCA/OCA some time ago. Feel free to check at the OCA
Signatures-List[3]

thanks to david buck for hosting this patch on cr.openjdk.java.net.

-- Sebastian Sickelmann

[1]http://cr.openjdk.java.net/~dbuck/4906983.0/

Just to confirm this is a spec only change, that documents long standing 
existing behaviour, right?

I think we should add a minimal testcase to cover this.

Thanks,
-Chris.


[2]https://bugs.openjdk.java.net/browse/JDK-4906983

[3]http://www.oracle.com/technetwork/community/oca-486395.html

Re: RFR 4906983: java.net.URL constructors throw MalformedURLException in undocumented way

[Mark Sheppard]

I was thinking as a change for all constructors, as there are URL, which 
may overload authority part's structural elements such that port might be

a "remote object id",  or some other form of token.

OK, that's fine

On 13/09/2015 15:25, Chris Hegarty wrote:
Is this suggested wording for the “spec” accepting constructors?  I 
think what we have for the constructors accepting protocol, host, 
port, etc, is more accurate.

Re: RFR: 8135305: InetAddress.isReachable reports true when loopback interface is specified

[Mark Sheppard]

Hi Rob,
   looks fine ...  the "him" variable in the

Java_java_net_Inet4AddressImpl_isReachable0

would appear not to be used now, so could probably be removed with its  
call on memset ?


regards
Mark

On 24/09/2015 15:45, Rob McKenna wrote:
Please ignore the formatting errors. Thats either a problem with hg 
diff or webrev, but its fixed in the repo.


-Rob

On 24/09/15 15:43, Rob McKenna wrote:

Hi folks,

The recent change to isReachable (8133015:
InetAddress.isReachable(tmout) returning wrong value on Windows for IPv6
- http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/59ff6cd9535d) neglected
to take the specified interface into account for ipv4.

This change ensures that isReachable returns false for external hosts
when the loopback interface is specified.

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

 -Rob

Re: SO_REUSEPORT feature support in JDK 9 for socket communication

[Mark Sheppard]

the following JBS item exists:
https://bugs.openjdk.java.net/browse/JDK-6432031

search of windows documentation suggests that SO_REUSEPORT is still not 
an option


man setsockopt on Solaris shows it as an option, but without precise 
description of semantics


regards
Mark

On 22/10/2015 14:33, Michael McMahon wrote:

On 22/10/15 14:24, Alan Bateman wrote:



On 22/10/2015 14:04, Roger Riggs wrote:

Hi Sandhya,

The folks on net-dev@openjdk.java.net will be interested too.

Yes, net-dev is the best list for this.

One other thing to mention is the SocketOption interface and the 
setOption/getOption methods. This allows for platform or JDK-specific 
specific socket options, it also allows it to be implemented by the 
NIO SocketChannel and friends.


-Alan


and there is the jdk.net API which extends this to the java.net socket 
types.


I think a Java SE API would be fine if it is widely and consistently 
implemented
across all the reference platforms and if it does not cause 
compatibility issues.


If it only works on some platforms then maybe jdk.net could be the 
place for it.


- Michael

RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

Hi,
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8134577/webrev/

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

the operative word has been "eliminate".
As such, the interface and service descriptor 
sun.net.spi.nameservice.NameService

sun.net.spi.nameservice.NameServiceDescriptor*
*together with its implementation (sun.net.nameservice.dns.DNSNameService)
has been remove from the JDK libraries.

The immediate impact is seen in the JDK testing framework.

To facilitate testing activity, and provide a replacement for the 
customized NameService implementations in the

JDK tests,  the default NameService has been extended to support
the retrieval of host to IP address mappings from a file.
The file path is specified with a system property " jdk.internal.hosts".

Previously a nameservice provider was specified by setting the system 
property

"sun.net.spi.nameservice.provider.", as per the documentation
http://docs.oracle.com/javase/8/docs/technotes/guides/net/properties.html

InetAddress now tests to determine if this property is set and will 
throw a ServiceConfigurationError
indicating that this functionality is no longer supported. The choice of 
ServideConfigurationError may cause
some debate, or disagreement. The rationale was that InternalError, is 
documented to relate to a JVM error,

and javax.naming.NamingException has a context of DirContext.
A possible alternative candidate could be 
javax.naming.ServiceUnavailableException.
As such, the setting of the property "sun.net.spi.nameservice.provider." 
was used, previously, as a configuration
parameter for the loading of a NamerService service provider, and as 
this is now (considered) an error, ServiceConfigurationError,

seemed a best fit!

These changes impacted a number of jdk security tests, also. The 
affected tetsts have been amended to adopt the
changes, with the exception of 
test/sun/security/x509/URICertStore/ExtensionsWithLDAP.java

which will require some rewrite.

regards
Mark

Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

Hi Artem,
thanks for the feedback, I'll make the changes for 2 & 3.
I'll look into rework of try-with-resources.

regards
Mark

On 26/10/2015 00:24, Artem Smotrakov wrote:

Hi Mark,

I am not a reviewer, just have a couple of comments about InetAddress.java

1. It may be better to create an instance of Scanner in 
try-with-resource block to be sure that Scanner.close() method is called.


2. Lines 909-923:

There are two similar "if" blocks in the loop. Looks like the first 
one is not necessary (I also see similar code in lookupAllHostAddr() 
method, maybe this code could be moved to a separate method).


3. extractHostAddr() and extractHost() methods:

The methods assume that "hostEntry" contains at least one whitespace, 
and access first and second elements of "mapping " array. It may be 
better to check that length of "mapping" is more than one to avoid a 
possible ArrayIndexOutOfBoundsException.


Looks like those methods may also be static.

Artem

On 10/26/2015 02:32 AM, Mark Sheppard wrote:

Hi,
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8134577/webrev/

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

the operative word has been "eliminate".
As such, the interface and service descriptor 
sun.net.spi.nameservice.NameService

sun.net.spi.nameservice.NameServiceDescriptor*
*together with its implementation 
(sun.net.nameservice.dns.DNSNameService)

has been remove from the JDK libraries.

The immediate impact is seen in the JDK testing framework.

To facilitate testing activity, and provide a replacement for the 
customized NameService implementations in the

JDK tests,  the default NameService has been extended to support
the retrieval of host to IP address mappings from a file.
The file path is specified with a system property " jdk.internal.hosts".

Previously a nameservice provider was specified by setting the system 
property

"sun.net.spi.nameservice.provider.", as per the documentation
http://docs.oracle.com/javase/8/docs/technotes/guides/net/properties.html

InetAddress now tests to determine if this property is set and will 
throw a ServiceConfigurationError
indicating that this functionality is no longer supported. The choice 
of ServideConfigurationError may cause
some debate, or disagreement. The rationale was that InternalError,  
is documented to relate to a JVM error,

and javax.naming.NamingException has a context of DirContext.
A possible alternative candidate could be 
javax.naming.ServiceUnavailableException.
As such, the setting of the property 
"sun.net.spi.nameservice.provider." was used, previously, as a 
configuration
parameter for the loading of a NamerService service provider, and as 
this is now (considered) an error, ServiceConfigurationError,

seemed a best fit!

These changes impacted a number of jdk security tests, also. The 
affected tetsts have been amended to adopt the
changes, with the exception of 
test/sun/security/x509/URICertStore/ExtensionsWithLDAP.java

which will require some rewrite.

regards
Mark

Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

thanks Alan,

I'll double check the illegal state token recognition.
There was a security test that used this.

property name of "jdk.net.hosts.file"   is fine.

we can remove the code for reading the service provider property 
sun.net.spi.nameservice.provider.
and ignore its setting if we think that is most appropriate. I 
considered the ServiceConfigurationError, not quite "user friendly", but
a reasonable form of feedback, to convey that the functionality was not 
available and so that an application

could be amended to align with NameService retirement.

On 27/10/2015 15:05, Alan Bateman wrote:

On 25/10/2015 23:32, Mark Sheppard wrote:

Hi,
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8134577/webrev/

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

the operative word has been "eliminate".
This has been a very troublesome mechanism so good to see this reduced 
down to a hosts file that we can we use for testing.


Shouldn't the reading of sun.net.spi.nameservice.provider. be 
removed so that the throwing of ServiceConfigurationError can be 
removed too?


Also just on the property name, I would assume it should be something 
like "jdk.net.hosts.file" rather than "jdk.internal.hosts".


Do we have tests that are using the illegal_state_exception token? I 
didn't spot any in the webrev. Just wondering if this is the right 
exception for testing.


-Alan.

Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

Hi Max,
   thanks for the reply.

I'll change the hosts file names used  to correspond with an associated test

dynamic update of host / ip mappings updates are accommodated as seen 
per the tests

http://cr.openjdk.java.net/~msheppar/8134577/webrev/test/sun/net/InetAddress/nameservice/simple/DefaultCaching.java.sdiff.html
http://cr.openjdk.java.net/~msheppar/8134577/webrev/test/sun/net/InetAddress/nameservice/simple/CacheTest.java.sdiff.html


yes, that's correct the structure of hosts file does not align with 
/etc/hosts (ip address list of aliases), it aligns with the

various customized NameService implementations in the test framework.

I'll look into the UnknownHostException suggestion

regards
Mark

On 26/10/2015 01:57, Wang Weijun wrote:

I see a lot of krb5 tests modified.

Basically, the NameServiceDescriptor inside KDC.java maps everything to 
localhost except for one (I guess Artem invented the 2nd feature). Can we 
expand the grammar a little bit to support this? For example:

   not.existing.host UnknownHostException
   * 127.0.0.1

I see you've added all names the tests have touched in 
test/sun/security/krb5/auto/hosts, but I am not sure if we might add randomly 
generated hostnames in any test later.

BTW, the standard /etc/hosts has lines like "127.0.0.1 localhost" which has a 
different order than yours. Shall we rename hosts to something else so that people does 
not get confused?

Thanks
Max


http://cr.openjdk.java.net/~msheppar/8134577/webrev/

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

Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

Hi,
based on feedback from first review the updates have been amended
please oblige and review the current set of changes as per

http://cr.openjdk.java.net/~msheppar/8134577/webrev.02/

regards
Mark

On 25/10/2015 23:32, Mark Sheppard wrote:

Hi,
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8134577/webrev/

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

the operative word has been "eliminate".
As such, the interface and service descriptor 
sun.net.spi.nameservice.NameService

sun.net.spi.nameservice.NameServiceDescriptor*
*together with its implementation (sun.net.nameservice.dns.DNSNameService)
has been remove from the JDK libraries.

The immediate impact is seen in the JDK testing framework.

To facilitate testing activity, and provide a replacement for the 
customized NameService implementations in the

JDK tests,  the default NameService has been extended to support
the retrieval of host to IP address mappings from a file.
The file path is specified with a system property " jdk.internal.hosts".

Previously a nameservice provider was specified by setting the system 
property

"sun.net.spi.nameservice.provider.", as per the documentation
http://docs.oracle.com/javase/8/docs/technotes/guides/net/properties.html

InetAddress now tests to determine if this property is set and will 
throw a ServiceConfigurationError
indicating that this functionality is no longer supported. The choice 
of ServideConfigurationError may cause
some debate, or disagreement. The rationale was that InternalError,  
is documented to relate to a JVM error,

and javax.naming.NamingException has a context of DirContext.
A possible alternative candidate could be 
javax.naming.ServiceUnavailableException.
As such, the setting of the property 
"sun.net.spi.nameservice.provider." was used, previously, as a 
configuration
parameter for the loading of a NamerService service provider, and as 
this is now (considered) an error, ServiceConfigurationError,

seemed a best fit!

These changes impacted a number of jdk security tests, also. The 
affected tetsts have been amended to adopt the
changes, with the exception of 
test/sun/security/x509/URICertStore/ExtensionsWithLDAP.java

which will require some rewrite.

regards
Mark

Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

Hi,
   to summarize the amendments below:

ServiceConfigurationError removed - the code no longer reads the 
nameservice provider property, it silently ignored


try-with-resources statement added in getHostByAddr and 
lookupAllHostAddr methods of NameService, and duplicate statements removed


hosts file name in tests changed to TestHosts to avoid any potential 
confusion with /etc/hosts


regards
Mark



On 12/11/2015 16:46, Mark Sheppard wrote:

Hi,
based on feedback from first review the updates have been amended
please oblige and review the current set of changes as per

http://cr.openjdk.java.net/~msheppar/8134577/webrev.02/

regards
Mark

On 25/10/2015 23:32, Mark Sheppard wrote:

Hi,
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8134577/webrev/

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

the operative word has been "eliminate".
As such, the interface and service descriptor 
sun.net.spi.nameservice.NameService

sun.net.spi.nameservice.NameServiceDescriptor*
*together with its implementation 
(sun.net.nameservice.dns.DNSNameService)

has been remove from the JDK libraries.

The immediate impact is seen in the JDK testing framework.

To facilitate testing activity, and provide a replacement for the 
customized NameService implementations in the

JDK tests,  the default NameService has been extended to support
the retrieval of host to IP address mappings from a file.
The file path is specified with a system property " jdk.internal.hosts".

Previously a nameservice provider was specified by setting the system 
property

"sun.net.spi.nameservice.provider.", as per the documentation
http://docs.oracle.com/javase/8/docs/technotes/guides/net/properties.html

InetAddress now tests to determine if this property is set and will 
throw a ServiceConfigurationError
indicating that this functionality is no longer supported. The choice 
of ServideConfigurationError may cause
some debate, or disagreement. The rationale was that InternalError,  
is documented to relate to a JVM error,

and javax.naming.NamingException has a context of DirContext.
A possible alternative candidate could be 
javax.naming.ServiceUnavailableException.
As such, the setting of the property 
"sun.net.spi.nameservice.provider." was used, previously, as a 
configuration
parameter for the loading of a NamerService service provider, and as 
this is now (considered) an error, ServiceConfigurationError,

seemed a best fit!

These changes impacted a number of jdk security tests, also. The 
affected tetsts have been amended to adopt the
changes, with the exception of 
test/sun/security/x509/URICertStore/ExtensionsWithLDAP.java

which will require some rewrite.

regards
Mark

Re: Patch for adding SO_REUSEPORT socket option

[Mark Sheppard]

there would appear to be a link to a webrev in the JBS bug 
https://bugs.openjdk.java.net/browse/JDK-6432031 



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



I presume that a review is requested for this patch?

regards
Mark

On 19/11/2015 14:11, Michael McMahon wrote:

Hi Kishor

Did you mean to include the patch with this message?
If you send it, I can convert it to a webrev and put
it on the cr.openjdk server for review

Thanks
Michael

On 16/11/15 18:49, Kharbas, Kishor wrote:


Hello all,

I request the community to review a patch for adding SO_REUSEPORT 
support. There is already an existing JBS opened at 
https://bugs.openjdk.java.net/browse/JDK-6432031 



Details :

SO_REUSEPORT removes 1:1 assignment between listen socket and IP:PORT 
pair and enable multiple sockets listening to the same address and 
port. This improves the scalability and parallelism of network 
traffic handling. It is enabled for both TCP and UDP sockets (at 
least for Linux). For more details, please refer to 
https://lwn.net/Articles/542629/. Many applications, especially Linux 
or BSD based webservers such as Apache httpd and Nginx are already 
supporting it now. Ruby and Python have it supported as well. Other 
Java applications such as Netty webserver have it supported via JNI 
function since JDK has not supported it yet.


By enabling the SO_REUSEPORT feature itself, up to 4X throughput and 
latency improvement have been observed from various applications. 
Specific to Java application with this patch, we modified Apache 
Hadoop Distributed File System (HDFS) source code to take advantage 
of this feature. We observed up to 1.93x performance improvements.


The feature is supported since Linux Kernel 3.9. It is also supported 
in BSDs, Solaris and Mac OS. Windows does not have it. In the current 
patch, we only enable the feature on Linux platform since we do not 
have BSD, Solaris and Mac OS for testing. Whether the feature is 
supported or not on the running kernel is determined at the run time.


P.S. Based on Alan Baleman's comment on JBS, we are in meanwhile 
working on adding this option to 'java.net.ExtendedSocketOption'.


Regards,

Kishor Kharbas

Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

Alan thanks for the feedback ...

the implementation is based on what is currently being used in the tests 
via the existing

set of NameService implementations.

yes, the processing of the hosts file returns the first mapping it 
encounters.
The functionality is to provide similar to that which currently exists 
in the various

NameService implementations, such as SimpleNameService.
This would, afaik also, be in line with name service library functions
processing /etc/hosts file.

WRT returning a list of ip addresses, if that is needed then we can 
amend the implementation, but
current implementation of tests' NameServices implementations didn't 
provide or use such

functionality

Again, none of the tests had a NameService implementation 
(SimpleNameService etc,) supporting IPv6, so this determined
what went into the current implementation. It is something that could be 
added in future when required.


wrt comments, they could be accommodated, but note that the structure of 
a jdk.net.hosts.file is not the same as
/etc/hosts, which maps an IP address to a list of host aliases. If you 
that we change the mapping order and the format

we can do that.

wrt illegal_state_exception token, I left this in, thinking that it was 
worthwhile as it tests a concurrency issue within InetAddress and the 
InetAddress cache.

We can remove this also.

i'll add a transient declaration to re-enforce their absence from 
serialized form.


So, do we wish to change the mapping structure, as per /etc/hosts ?
do we wish that a list of ip addresses are returned, even if tests don't 
currently need it ?


regards
Mark

On 22/11/2015 09:56, Alan Bateman wrote:


On 12/11/2015 16:46, Mark Sheppard wrote:

Hi,
based on feedback from first review the updates have been amended
please oblige and review the current set of changes as per

http://cr.openjdk.java.net/~msheppar/8134577/webrev.02/


I'm looked through the changes in this webrev.

At a high-level then it's great to see this troublesome internal 
provider interface going away. Using the property jdk.net.hosts.file 
to specify a hosts file that tests can use seems reasonable.


In a hosts file then a host with several IP addresses can be listed on 
several lines. If I read lookupAllHostAddr correctly then it's 
ignoring this case and will only find the first line. For testing 
purposes then should we allow for hosts file where a host is defined 
to have multiple addresses?


Another question is whether IPv6 addresses should be supported, it 
looks like lookupAllHostAddr only expects IPv4 addresses.


The other thing is comments (#) ? I'm sure it could be useful to be 
able to copy a hosts file and use for testing.


The token illegal_state_exception still looks strange to me as it 
means InetAddress.getByName will throw an exception that it is not 
specified to throw. I see the test Hang is using it but that test is 
for the provider interface that is removed so I assume the test should 
be removed too.


A minor point but useLocalHostFileLookpu and hostFilename are static 
so I assume the transient modifier isn't needed.


-Alan.

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

[Mark Sheppard]

yes, the fix looks fine, and I verified the individual isReachable test 
on the test host where it failed.


it is possible to refactor the function

Java_java_net_Inet4AddressImpl_isReachable0

a little,  by extracting the newly re-introduced "else" block into its 
own function

e.g. wxp_ping4

such that

if (isVistaSP1OrGreater()) {
jint src_addr = 0;
jint dest_addr = 0;
jbyte caddr[4];
int sz;

/**

 * Convert IP address from byte array to integer
 */
sz = (*env)->GetArrayLength(env, addrArray);
if (sz != 4) {
  return JNI_FALSE;
}
memset((char *) caddr, 0, sizeof(caddr));
(*env)->GetByteArrayRegion(env, addrArray, 0, 4, caddr);
dest_addr = ((caddr[0]<<24) & 0xff00);
dest_addr |= ((caddr[1] <<16) & 0xff);
dest_addr |= ((caddr[2] <<8) & 0xff00);
dest_addr |= (caddr[3] & 0xff);
dest_addr = htonl(dest_addr);

/**

 * If a network interface was specified, let's convert its address
 * as well.
 */
if (!(IS_NULL(ifArray))) {
memset((char *) caddr, 0, sizeof(caddr));
(*env)->GetByteArrayRegion(env, ifArray, 0, 4, caddr);
src_addr = ((caddr[0]<<24) & 0xff00);
src_addr |= ((caddr[1] <<16) & 0xff);
src_addr |= ((caddr[2] <<8) & 0xff00);
src_addr |= (caddr[3] & 0xff);
src_addr = htonl(src_addr);
}

return ping4(env, src_addr, dest_addr, timeout);

} else {
wxp_ping4(env, this, addrArray, timeout, ifArray, ttl)
}



regards
Mark

On 25/11/2015 15:32, Seán Coffey wrote:
Looks ok to me Rob and provides a re-introduction of the old 
Java_java_net_Inet4AddressImpl_isReachable0 function for XP systems 
where necessary. Reviewed.


Approved for jdk8u-dev also.

Regards,
Sean.

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

forgot to cc net-dev

-Rob

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

Hi folks,

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


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

 -Rob

Re: 8143397: It looks like InetAddress.isReachable(timeout) works incorrectly

[Mark Sheppard]

Hi Rob,
change looks fine and handles the MS idiosyncrasies neatly
change works fine ... consistent responses and failing test returns 
expected results



regards
Mark

On 09/12/2015 01:44, Rob McKenna wrote:
The intention of the 2nd revision of the fix is to make the 
undocumented 1000ms problem a non issue.


If a user calls this function with a timeout of 200ms that timeout is 
automatically substituted for 1000ms in the IcmpSendEcho call. Once 
the response is received its RTT is checked to make sure it is lower 
than 200ms. If not the method returns false.


That being the case though the call could potentially take up to 
1000ms to complete where a host is not reachable even though a smaller 
timeout is specified. The return value of isReachable() will correctly 
say whether the host was reachable or not within the actual timeout 
specified by the user however.


In order to avoid the older (and less reliable) way of testing 
reachability I feel that this small corner case is a worthwhile tradeoff.


-Rob

On 09/12/15 01:31, Xuelei Fan wrote:

Is it nice to say in the spec that it is not reliable if the timeout is
too small?  At least 1000ms timeout by default may be not acceptable in
some circumstances.

Xuelei

On 12/9/2015 12:31 AM, Rob McKenna wrote:

Testing has shown that when a timeout < 1000ms is specified the
IcmpSendEcho calls fail (apparently) randomly. Once the timeout is
1000ms or greater it works as expected. Therefore I've updated the fix
to use 1000ms as a minimum. The existing logic ensures that the ttl is
less than the specified timeout in any case:

http://cr.openjdk.java.net/~robm/8143397/webrev.02/

 -Rob

On 01/12/15 14:59, Rob McKenna wrote:

It appears that there is an undocumented minimum timeout in the
IcmpSendEcho* functions. If the timeout parameter is set to a number
below this minimum timeout it is effectively ignored. Thus if you 
wanted
to ensure that you could ping a particular host within a certain 
timeout

less than the undocumented minimum, you could potentially receive a
false positive. (i.e. if you set the timeout to 20ms but the ping 
takes

30ms, IcmpSendEcho will still succeed)

The following fix checks the icmp reply packet and compares the round
trip time to the requested timeout parameter before deciding 
whether the

call was successful or not:

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

  -Rob

Re: RFR 4906983: java.net.URL constructors throw MalformedURLException in undocumented way

[Mark Sheppard]

yes that's fine with me also

regards
Mark

On 15/12/2015 15:04, Chris Hegarty wrote:

On 13 Nov 2015, at 04:59, Sebastian Sickelmann  
wrote:


Hi,

my recordings related to this thread show me that this thread is open. Sorry 
for loosing track for some time.

If i am right i got an thumbs up review result from Chris Hegarty for an older 
patch http://cr.openjdk.java.net/~dbuck/4906983.1/ (pending the outcome of the 
mail-exchange with Mark-Sheppard).
@Chris: Am i right in my assumption? Is this still valid?

The result on the pending mail-exchange is in the
http://cr.openjdk.java.net/~dbuck/4906983.3/

I’m ok with this latest version.

-Chris.



@Mark: Are you ok, with the provided patch?

@David: Thanks for hosting the webrevs for me and suggesting that i try to 
become an author of OpenJDK.

As I am now an author, i would like to create a jdk-changeset (depending on a 
positive review result) and host it on my cr.openjdk.java.net account.
I would list Chris and Mark as reviewers.

@David: Are you still ok with supporting me to push this changeset depending on 
a positive review result? Are you ok to be mentioned as reviewer too? I haven't 
found any review statement from your site. Thanks again for your great support 
getting me up and running with the openjdk infrastructure.

Thanks
Sebastian


On 09/16/2015 04:27 PM, Sebastian Sickelmann wrote:

Hi,

here is the updated webrev:


http://cr.openjdk.java.net/~dbuck/4906983.3/


Hope the comments are fine now.

What would be the normal procedure in net-dev for accepting a change (a
2 group-member thumbs-up like in core-libs)?

-- Sebastian

.cleared long thread... please see mailing-list-archive for 
details

Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

Hi
  please oblige and review the current version of the fix  for
https://bugs.openjdk.java.net/browse/JDK-8134577

at
http://cr.openjdk.java.net/~msheppar/8134577/webrev.05/

which is  based on feedback from the second review.

In summary the following changes have been made

* hosts file mapping structure has been changed to align with 
/etc/hosts, that is,rather than  
  


* elementary comment handling added, skip lines starting with #

* lookupAllHostAddr returns an array of InetAddresses, rather than a 
single element array containing the first host address mapping


* the illegal_state_exception  token handling has been removed, and a 
corresponding test deadlock/Hang.java removed


* transient key word added to member variables useLocalHostsFileLookup 
and hostsFileName


* added an explicit test to exercise the internal NameService

regards
Mark

On 13/11/2015 12:44, Mark Sheppard wrote:

Hi,
   to summarize the amendments below:

ServiceConfigurationError removed - the code no longer reads the 
nameservice provider property, it silently ignored


try-with-resources statement added in getHostByAddr and 
lookupAllHostAddr methods of NameService, and duplicate statements removed


hosts file name in tests changed to TestHosts to avoid any potential 
confusion with /etc/hosts


regards
Mark



On 12/11/2015 16:46, Mark Sheppard wrote:

Hi,
based on feedback from first review the updates have been amended
please oblige and review the current set of changes as per

http://cr.openjdk.java.net/~msheppar/8134577/webrev.02/

regards
Mark

On 25/10/2015 23:32, Mark Sheppard wrote:

Hi,
   please oblige and review the following changes
http://cr.openjdk.java.net/~msheppar/8134577/webrev/

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

the operative word has been "eliminate".
As such, the interface and service descriptor 
sun.net.spi.nameservice.NameService

sun.net.spi.nameservice.NameServiceDescriptor*
*together with its implementation 
(sun.net.nameservice.dns.DNSNameService)

has been remove from the JDK libraries.

The immediate impact is seen in the JDK testing framework.

To facilitate testing activity, and provide a replacement for the 
customized NameService implementations in the

JDK tests,  the default NameService has been extended to support
the retrieval of host to IP address mappings from a file.
The file path is specified with a system property " jdk.internal.hosts".

Previously a nameservice provider was specified by setting the 
system property

"sun.net.spi.nameservice.provider.", as per the documentation
http://docs.oracle.com/javase/8/docs/technotes/guides/net/properties.html

InetAddress now tests to determine if this property is set and will 
throw a ServiceConfigurationError
indicating that this functionality is no longer supported. The 
choice of ServideConfigurationError may cause
some debate, or disagreement. The rationale was that InternalError,  
is documented to relate to a JVM error,

and javax.naming.NamingException has a context of DirContext.
A possible alternative candidate could be 
javax.naming.ServiceUnavailableException.
As such, the setting of the property 
"sun.net.spi.nameservice.provider." was used, previously, as a 
configuration
parameter for the loading of a NamerService service provider, and as 
this is now (considered) an error, ServiceConfigurationError,

seemed a best fit!

These changes impacted a number of jdk security tests, also. The 
affected tetsts have been amended to adopt the
changes, with the exception of 
test/sun/security/x509/URICertStore/ExtensionsWithLDAP.java

which will require some rewrite.

regards
Mark

Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

Hi,
   as per feedback below webrev has been updated

http://cr.openjdk.java.net/~msheppar/8134577/webrev.06/

change summary:

* List nameServices replaced with Nameservice nameService

* references to nameServices removed

* private interface NameService added, with two implementation 
PlatformNameService and HostsFileNameService


* Scanner created with UTF-8  charset

* removed StringTokenizer, replaced with String.split()

* comment handling extended, handling comments on line with mapping entry

* try with resources added to addMappingToHostsFile method in tests

regards
Mark

On 02/01/2016 07:40, Alan Bateman wrote:

On 31/12/2015 14:30, Mark Sheppard wrote:

Hi
  please oblige and review the current version of the fix  for
https://bugs.openjdk.java.net/browse/JDK-8134577

at
http://cr.openjdk.java.net/~msheppar/8134577/webrev.05/

which is  based on feedback from the second review.
I looked through the latest webrev and I think this is starting to 
look good. Having the hosts file be the same format (albeit a subset) 
as /etc/hosts is a big improvement on previous iterations.


InetAddress.nameServices is still a List but the List 
will always have one element, should this be changed to 
InetAddress.nameService (singular)?


I think it would be cleaner if NameService were changed to an 
interface with two implementations, say PlatformNameService and 
HostsFileNameService. That way you could eliminate the 
useLocalHostsFileLookup and hostsFileName fields from InetAddress and 
HostsFileNameService could encapsulate the parsing of the hosts file 
rather than NameService trying to support both ways.


The property name jdk.net.hosts.file is good but if set to a file that 
doesn't exist then the current patch falls back to use the platform 
name service. I assume this is not the original intention.


The Scanner is created without specifying a charset, is this 
deliberate because the platform /etc/hosts is in platform encoding? 
For tests then it might be better to use UTF-8 because these hosts 
file will be used on several different platforms.


Is there any reason to use legacy StringTokenizer in 
createAddressByteArray? In other areas of the patch then it using 
Scanner or String.split so it seems inconsistent to see legacy 
StringTokenizer in use too.


You mentioned in the mail about # supported as a comment when the 
first character. It doesn't seem to be much effort to just drop 
everything after the # so that it is more consistent with the platform 
hosts file.


UHE is thrown in a few places without any exception message. For the 
hosts file then it would be useful to include some message to make 
configuration issues easier to diagnose.


The comment in NameService has a historical reference to the JNDI DNS 
provider, I assume that is not needed.


I also looked through the test changes. Several tests set test.src and 
not clear that this is needed. I assume that what you really want is:

String testSrc = System.getProperty("test.src", ".");

addMappingToHostsFile is added to a number of tests. It would be good 
if this could use try-with-resources to avoid leaving a file open then 
the write fails (say a test machine with a full file system).


sun/security/x509/URICertStore/ExtensionsWithLDAP.java has been added 
to the exclude list with JDK-8134577 as the issue number. Is there is 
a different issue number for this?


Are all of the tests in test/sun/net/InetAddress/nameservice still 
needed? Some of these tests, as least in the 'dns' directory, are 
tests for the JNDI DNS provider and maybe they should be deleted.


The update to KDC means that any test using this library will need to 
run in othervm mode. That may be true already but we should check. 
Related to this is that setting the system property in the main method 
might be fragile in that it's possible for code to execute before the 
main method that triggers the initialization of InetAddress. Something 
to keep in mind as we might have to re-visit these tests again.


-Alan

Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

thanks for the feedback, Alan

based on suggestions, all issues have been addressed and patch has been 
updated


http://cr.openjdk.java.net/~msheppar/8134577/webrev.07

the following should be noted:

ExtensionsWithLDAP.java is added to the exclude list with JDK-8134577 - 
This is on the problem list as it will need to be re-written
The NameService associated with this test threw UnknownHostExceptions 
and added  the host name to a List, which was then examined

later in the test as a verification.

The instantiation of a NameService is as intended, i.e.
if jdk.net.hosts.file set and file exist then
instantiate HostsFileNameService
else
instantiate PlatformNameService

this is in keeping with previous initialization semantics - if the no 
service provider NameService

created then the default NameService was instantiated.

regards
Mark


On 05/01/2016 13:10, Alan Bateman wrote:

On 05/01/2016 00:54, Mark Sheppard wrote:

Hi,
   as per feedback below webrev has been updated

http://cr.openjdk.java.net/~msheppar/8134577/webrev.06/

change summary:

* List nameServices replaced with Nameservice nameService

* references to nameServices removed

* private interface NameService added, with two implementation 
PlatformNameService and HostsFileNameService


* Scanner created with UTF-8  charset

* removed StringTokenizer, replaced with String.split()

* comment handling extended, handling comments on line with mapping entry

* try with resources added to addMappingToHostsFile method in tests


I think this is starting to look good.

A few small comments:

- instantiateNameService (would createNameService be nicer?) still 
falls back to PlatformNameService when the hosts file doesn't exist. 
Is that expected?


- HostsFileNameService.hostsFile can be final.

- in removeComments then I assume you can just use indexOf and check 
for -1 rather than contains + indexOf.


- it would be nice if NameService had some javadoc. Also the methods 
don't need to be declared public.


- it would be nice if PlatformnameService and HostsFileNameService 
could use /** */ style javadoc rather than //, just to keep the code 
consistent.


- ExtensionsWithLDAP.java is added to the exclude list with 
JDK-8134577, is there is separate issue for this?


- formatting. There are a few places where the indention is messed up 
(5 spaces instead of 4 in a few places). Also some of the throws XXX 
are intended in many different ways. Blank lines can help readability 
but seem to be inconsistently used here. There's clearly been a lot of 
revisions on this issue and maybe the simplest is to just reformat 
this section of the file in the IDE.


-Alan.

Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

again thanks for the review and feedback, Alan

patch amended as suggested
http://cr.openjdk.java.net/~msheppar/8134577/webrev.08/

removed File.exists test in createNameService - lookup methods in 
HostsFileNameService already throw UHE for FileNotFoundException


InetAddress.impl marked final

removed references to JNDI

added test for non-existent hosts file

regards
Mark

On 06/01/2016 20:37, Alan Bateman wrote:

On 06/01/2016 15:37, Mark Sheppard wrote:

thanks for the feedback, Alan

based on suggestions, all issues have been addressed and patch has 
been updated


http://cr.openjdk.java.net/~msheppar/8134577/webrev.07

the following should be noted:

ExtensionsWithLDAP.java is added to the exclude list with JDK-8134577 
- This is on the problem list as it will need to be re-written
The NameService associated with this test threw UnknownHostExceptions 
and added  the host name to a List, which was then examined

later in the test as a verification.

The instantiation of a NameService is as intended, i.e.
if jdk.net.hosts.file set and file exist then
instantiate HostsFileNameService
else
instantiate PlatformNameService

this is in keeping with previous initialization semantics - if the no 
service provider NameService

created then the default NameService was instantiated.

I think this looks quite good.

If jdk.net.hosts.file is set to a file that doesn't exist then I think 
it should throw an exception or else just work as if the hosts file is 
empty. However, it's probably not worth spending time on as this 
mechanism is mostly for tests.


There is still a reference to the JNDI-DNS provider in 
HostsFileNameService and I assume that should be removed as it's 
essentially a historical reference now.


I assume InetAddress.impl can be changed to be final.

For ExtensionsWithLDAP.java then you need to create an issue to track 
it and use that issue number when adding it to the exclude list.


I think that is all I have on this topic.

-Alan

RFR: JDK-8147862 - Null check too late in sun.net.httpserver.ServerImpl

[Mark Sheppard]

Hi
   please oblige and review the small change
http://cr.openjdk.java.net/~msheppar/8147862/webrev/

to address
https://bugs.openjdk.java.net/browse/JDK-8147862

it was observed that  a null check on a SocketChannel object in
the Disptacher.run() method within ServerImpl, should be
earlier in the call flow.

regards
Mark

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

[Mark Sheppard]

FWIW

the src is restructured in JDK9, but equivalent changes exist

src/java.base/unix/native/libnet/net_util_md.c
src/java.base/unix/native/libnet/net_util_md.h

regards
Mark


On 12/02/2016 11:28, Chris Hegarty wrote:

On 12/02/16 11:09, Ramanand Patil wrote:

Hi Sean,

Since I saw the convention of filenames being a bug id, I had created 
a new test. :)

Anyways, I have updated the existing test with my bug id.
Here is the updated Webrev: 
http://cr.openjdk.java.net/~rpatil/8135259/webrev.02/


The changes look fine. ( This is JDK 8 only ).

The convention of using the bug Id in the test class name is old,
and we don't really follow it any more. I am perfectly happy with
what you have, but you can if you like rename the test, 'hg mv',
to something more appropriate, e.g. ErrorMessage, or something
else.

-Chris.



Regards,
Ramanand.

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


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


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


Regards,
Sean.

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

Hi Sean,

Thank you. I have updated copyright year to the earlier patch and 
added a simple test case to cover this bug.

Please review the updated Webrev:
http://cr.openjdk.java.net/~rpatil/8135259/webrev.01/

Regards,
Ramanand.

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

(dropping core-libs)

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


Regards,
Sean.

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

Hi Ramanand!

Your fix looks good to me.

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

Sincerely yours,
Ivan


On 10.02.2016 10:08, Ramanand Patil wrote:

Hi all,

Please help me in getting this review done.

Regards,
Ramanand.

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

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

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

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

Fix: Using a function call directly "gai_strerror(gai_error)"
instead of using the function pointer which was declared but not 
initialized.

Apart from this I have removed few other unused variables and
function pointers. Brief description of how this bug was introduced
is added to the bug comment.

Regards,

Ramanand.

Re: RFR: JDK-8134577 - Eliminate or standardize a replacement for sun.net.spi.nameservice.NameServiceDescriptor

[Mark Sheppard]

Hi Chris,
  thanks for the feedback, I'll make the appropriate amendments

regards
Mark

On 18/03/2016 11:17, Chris Hegarty wrote:

Hi Mark,

This mainly looks good. Some specific comments on InetAddress.

  - Why did you add transient to this field?
 private static TRANSIENT NameService nameService = null;

  - There will be support for IPv6, right?  There is a comment
in the code that says otherwise.

  - jdk.net.hosts.file pointing to a file that does not exist
results in an UHE always, right? There is a comment in the
code that says otherwise.

I incorporated my comments, along with some stylistic and
proposed re-wordings, into a webrev for your convenience:


http://cr.openjdk.java.net/~chegar/8134577_comments/src/java.base/share/classes/java/net/InetAddress.java.sdiff.html 



-Chris.

On 07/01/16 15:38, Alan Bateman wrote:



On 07/01/2016 10:44, Mark Sheppard wrote:

again thanks for the review and feedback, Alan

patch amended as suggested
http://cr.openjdk.java.net/~msheppar/8134577/webrev.08/

removed File.exists test in createNameService - lookup methods in
HostsFileNameService already throw UHE for FileNotFoundException

InetAddress.impl marked final

removed references to JNDI

added test for non-existent hosts file


I think InetAddress looks okay now. At some point we should do some
clean-up on this code but I think it's okay for now.

Before you push this then can you create a bug to track the issue with
ExtensionsWithLDAP.java and put that issue number in the exclude list?

-Alan

Re: RFR(S): 8156521: Minor Fixes and cleanups in NetworkInterface.c

[Mark Sheppard]

the declaration in of plen (also scope and dad_status) as an int in 
enumIPv6Interfaces would appear to be done to align with

its use in fscanf, even though it is a two character conversion.
So would it not be more appropriate to cast it as a short when passed to 
addif ?


the name plen would also convey a variable containing a length, but that 
doesn't appear to be the purpose of plen.

a more appropriate name would be more informative in the code

regards
Mark


On 11/05/2016 09:50, Chris Hegarty wrote:

Hi Christoph,

On 11 May 2016, at 08:43, Dmitry Samersoff  wrote:


...
bugreport: https://bugs.openjdk.java.net/browse/JDK-8156521

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

I think this is mainly fine, and good to have such cleanup in this area.

I agree with the replacement of strcpy with strncpy, but I think we should
explicitly null terminate in case the src is greater or equal to the dst buffer
size. This is done elsewhere in this file too, e.g.

strncpy(buf, input, sizeof(buf) - 1);
buf[sizeof(buf) - 1] = '\0';



Apart from quite a few whitespace changes to clean up the coding, I went
through and replaced all occurences of strcpy with strncpy as this was a
finding of a code scanner that we used. Also in function
“enumIPv6Interfaces” for Linux the local variable plen was changed from
int to short.

Why was this done for plen specifically, and not scope, or others ?

-Chris.

Re: RFR 8157811 [9] Additional minor fixes and cleanups in NetworkInterface.c

[Mark Sheppard]

Hi Chris,
   the fix for 8068028 was marked 9-na for some reason, as such afaik 
the changes were not propagated to JDK9,
which changed its src tree structure at that time (i think, jdk8 were on 
solaris files), parfait may not have highlighted them in that context


afaik the other changes in that set were not applied 
(Inet6AddressImpl.c, net_util_md.c) .


regards
Mark

On 25/05/2016 10:50, Chris Hegarty wrote:

As a follow up to JDK-8156521, and when comparing against the
version of this file in jdk8u-dev a few minor issues were noticed.

There is a free of a memory structure that was missed somehow
in the 9 version of this file ( it is already in the 8 version ). The
remaining few changes are just some minor cleanup.

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

-Chris.

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

[Mark Sheppard]

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

linux and macos perviously without this setting.

regards
Mark

On 26/05/2016 14:33, Chris Hegarty wrote:

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

This is problematic as some interfaces like teredo on Windows, or awdl on Mac, 
are not
suitable. Rather than all this, the test can use the IPv6 loopback address, 
which can contain
a scope id, if retrieved from the NetworkInterface API.

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

-Chris.

Re: RFR 8158023: SocketExceptions contain too little information sometimes

[Mark Sheppard]

Hi,
wrt JNU_ThrowByNameWithMessaheAndLastError, it would appear that it 
doesn't allow for malloc to fail and hence
str1 could be null  and a problematic input to jio_snprintf which could 
result in a de-referencing of zero.


in the call flow would it not be more appropriate to manipulate native 
buffers first to produce the desired error string and

then allocate the java string object?
c.f. src/java.base/unic/native/libnet/net_util_md.c
NET_ThrowUnknownHostExceptionWithGaiError

should you allow for getLastErrorString not to return an error string or 
return an error result?

as in JNU_ThrowByNameWithLastError

regards
Mark

On 27/06/2016 08:42, Langer, Christoph wrote:

Hi,

eventually here is the - hopefully final - version of this fix:
http://cr.openjdk.java.net/~clanger/webrevs/8158023.3/

Now I leave JNU_ThrowByNameWithLastError untouched and I've also added conversion to the new 
function JNU_ThrowByNameWithMessageAndLastError. I've replaced JNU_ThrowByNameWithLastError with 
JNU_ThrowByNameWithMessageAndLastError in the java/net coding where I think it is appropriate 
(mostly in occasions when a SocketException is thrown kind of generically). @Paul: thanks for your 
suggestion regarding the output format but I would still prefer an output like 
"java.net.SocketException: ioctl SIOCGSIZIFCONF failed: Bad file number" vs. " 
java.net.SocketException: Bad file number (ioctl SIOCGSIZIFCONF failed)" as I'm always handing 
down a message to the new throw routine.

Hoping for a review :)

Best regards
Christoph


-Original Message-
From: Kenji Kazumura [mailto:k...@jp.fujitsu.com]
Sent: Mittwoch, 8. Juni 2016 02:51
To: Langer, Christoph 
Cc: net-dev@openjdk.java.net; nio-...@openjdk.java.net; core-libs-
d...@openjdk.java.net
Subject: Re: RFR 8158023: SocketExceptions contain too little information
sometimes

Christoph,

You should not remove conversion codes (platform string to Java String)
at JNU_ThrowByNameWithLastError,
and you have to add conversion codes at
JNU_ThrowByNameWithMessageAndLastError.

It seems that you assume strerror returns only ascii characters, but actually
not.
It depends on the locale of your environment where java programs runs.


-Kenji Kazumura


In message

RFR 8158023: SocketExceptions contain too little information sometimes
"Langer, Christoph"  wrote:


Hi all,

please review the following change:
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8158023.1/
Bug: https://bugs.openjdk.java.net/browse/JDK-8158023

During error analysis I stumbled over a place where I encountered a

SocketException which was thrown along with some strerror information as
message. I found it hard to find the originating code spot with that 
information.

So I looked at the places where we throw exceptions, namely JNU_Throw...

and NET_Throw... functions and came up with the following enhancement:

- NET_ThrowByNameWithLastError can go completely as it does not provide

any benefit over JNU_ThrowByNameWithLastError.

- JNU_ThrowByNameWithLastError can be cleaned up.

- I added JNU_ThrowByNameWithMessageAndLastError to print out a string

like message + ": " + last error.

- I went over all places where NET_ThrowByNameWithLastError is used and

replaced it appropriately.

Do you think this change is desirable/possible?

Though it's mainly a net topic, I'm posting it to nio-dev and core-libs-dev as

well as JNU_Throw... code affects all.

Best regards
Christoph

Re: RFR 8158023: SocketExceptions contain too little information sometimes

[Mark Sheppard]

Hi Christoph,
thanks for the response ... yes, you do check the 
getLastErrorString return ... sorry about that!


regards
Mark

On 27/06/2016 14:28, Langer, Christoph wrote:

Hi Mark,

thanks for looking at this. Please see my comments inline.


  wrt JNU_ThrowByNameWithMessaheAndLastError, it would appear that it
doesn't allow for malloc to fail and hence
str1 could be null  and a problematic input to jio_snprintf which could
result in a de-referencing of zero.

You are right, thanks. I should check for null and return "OutOfMemory" 
appropriately. I fixed that and updated the webrev in place.


in the call flow would it not be more appropriate to manipulate native
buffers first to produce the desired error string and
then allocate the java string object?
c.f. src/java.base/unic/native/libnet/net_util_md.c
NET_ThrowUnknownHostExceptionWithGaiError

Well, in the case of NET_ThrowUnknownHostExceptionWithGaiError you will 
probably have the hostname as well as the error text encoded in platform 
encoding. So for that it is perfectly fine to do the native buffers first and 
then convert to a Java String. But in my case for 
JNU_ThrowByNameWithMessageAndLastError, the errno text will be in platform 
encoding but the other message and formatting will be UTF-8 strings from the 
executable. So I have to handle both parts differently when creating a String 
object out of it. Or am I wrong in my assumption here?


should you allow for getLastErrorString not to return an error string or
return an error result?
as in JNU_ThrowByNameWithLastError

I do, don't I? I'm checking the return value of getLastErrorString: "if (n > 
0)".

Best regards
Christoph


On 27/06/2016 08:42, Langer, Christoph wrote:

Hi,

eventually here is the - hopefully final - version of this fix:
http://cr.openjdk.java.net/~clanger/webrevs/8158023.3/

Now I leave JNU_ThrowByNameWithLastError untouched and I've also added

conversion to the new function JNU_ThrowByNameWithMessageAndLastError.
I've replaced JNU_ThrowByNameWithLastError with
JNU_ThrowByNameWithMessageAndLastError in the java/net coding where I
think it is appropriate (mostly in occasions when a SocketException is thrown
kind of generically). @Paul: thanks for your suggestion regarding the output
format but I would still prefer an output like "java.net.SocketException: ioctl
SIOCGSIZIFCONF failed: Bad file number" vs. " java.net.SocketException: Bad
file number (ioctl SIOCGSIZIFCONF failed)" as I'm always handing down a
message to the new throw routine.

Hoping for a review :)

Best regards
Christoph


-Original Message-
From: Kenji Kazumura [mailto:k...@jp.fujitsu.com]
Sent: Mittwoch, 8. Juni 2016 02:51
To: Langer, Christoph 
Cc: net-dev@openjdk.java.net; nio-...@openjdk.java.net; core-libs-
d...@openjdk.java.net
Subject: Re: RFR 8158023: SocketExceptions contain too little information
sometimes

Christoph,

You should not remove conversion codes (platform string to Java String)
at JNU_ThrowByNameWithLastError,
and you have to add conversion codes at
JNU_ThrowByNameWithMessageAndLastError.

It seems that you assume strerror returns only ascii characters, but actually
not.
It depends on the locale of your environment where java programs runs.


-Kenji Kazumura


In message

 RFR 8158023: SocketExceptions contain too little information sometimes
 "Langer, Christoph"  wrote:


Hi all,

please review the following change:
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8158023.1/
Bug: https://bugs.openjdk.java.net/browse/JDK-8158023

During error analysis I stumbled over a place where I encountered a

SocketException which was thrown along with some strerror information as
message. I found it hard to find the originating code spot with that

information.

So I looked at the places where we throw exceptions, namely JNU_Throw...

and NET_Throw... functions and came up with the following enhancement:

- NET_ThrowByNameWithLastError can go completely as it does not

provide

any benefit over JNU_ThrowByNameWithLastError.

- JNU_ThrowByNameWithLastError can be cleaned up.

- I added JNU_ThrowByNameWithMessageAndLastError to print out a

string

like message + ": " + last error.

- I went over all places where NET_ThrowByNameWithLastError is used

and

replaced it appropriately.

Do you think this change is desirable/possible?

Though it's mainly a net topic, I'm posting it to nio-dev and core-libs-dev as

well as JNU_Throw... code affects all.

Best regards
Christoph

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

[Mark Sheppard]

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


along the lines

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


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


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


e&oe

regards
Mark


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

gentle reminder, please review the below code change.

Vyom


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

Hi All,

Please review the code changes for below issue.

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

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



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


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


Thanks,

Vyom

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

[Mark Sheppard]

Hi Vyom,
 thanks for doing the refactoring.
changes look OK to me

regards
Mark

On 01/09/2016 17:03, Vyom Tewari wrote:


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


Thanks,

Vyom


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


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


along the lines

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


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

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

e&oe

regards
Mark


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

gentle reminder, please review the below code change.

Vyom


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

Hi All,

Please review the code changes for below issue.

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

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


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


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


Thanks,

Vyom

Re: Ping: RFR(S): 8163181: Further improvements for Unix NetworkInterface native implementation

[Mark Sheppard]

have had a look through the changes twice, and they look fine ... i'll 
apply the patch and run a regression build to confirm


the moving of int flags on 919 to a conditional block,  I expect to 
cause a strict compile error in my solaris env, so need to check that


regards
Mark

On 02/09/2016 08:22, Langer, Christoph wrote:


Hi (Mark or Chris?),

as this RFR is outstanding for quite a while and it merely is a minor 
change which only cleans up coding, would you mind to review it that I 
can get it off my list?


The only major thing probably is, that calls to getMacAddress don’t 
need a socket anymore and sockets are only opened for platforms where 
an ioctl is done to determine the MacAddress.


As I’ve said, I’ve tested it on the Linux/Unix platforms.

Thanks a lot

Christoph

*From:*Langer, Christoph
*Sent:* Montag, 22. August 2016 15:30
*To:* 'net-dev@openjdk.java.net' 
*Subject:* RE: Ping: RFR(S): 8163181: Further improvements for Unix 
NetworkInterface native implementation


Hi,

I made a little update to my change: 
http://cr.openjdk.java.net/~clanger/webrevs/8163181.2/ 



Merely comments and a minor AIX specific correction.

Thanks in advance for reviewing

Christoph

*From:*Langer, Christoph
*Sent:* Dienstag, 16. August 2016 14:49
*To:* 'net-dev@openjdk.java.net' >
*Subject:* Ping: RFR(S): 8163181: Further improvements for Unix 
NetworkInterface native implementation


Ping: Can I get a review for this small set of changes please?

Thanks

Christoph

*From:*Langer, Christoph
*Sent:* Donnerstag, 4. August 2016 15:04
*To:* net-dev@openjdk.java.net 
*Subject:* RFR(S): 8163181: Further improvements for Unix 
NetworkInterface native implementation


Hi,

I had made a few more cleanups when I was working on 
NetworkInterface.c which I thought are worth contributing.


Please review: http://cr.openjdk.java.net/~clanger/webrevs/8163181.1/ 



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



As always, changes were built and tested on Linux, AIX, Solaris and Mac.

Thanks

Christoph

Re: Ping: RFR(S): 8163181: Further improvements for Unix NetworkInterface native implementation

[Mark Sheppard]

builds and regression tests appear to be OK with the proposed changes.

regards
Mark

On 02/09/2016 12:41, Mark Sheppard wrote:


have had a look through the changes twice, and they look fine ... i'll 
apply the patch and run a regression build to confirm


the moving of int flags on 919 to a conditional block,  I expect to 
cause a strict compile error in my solaris env, so need to check that


regards
Mark

On 02/09/2016 08:22, Langer, Christoph wrote:


Hi (Mark or Chris?),

as this RFR is outstanding for quite a while and it merely is a minor 
change which only cleans up coding, would you mind to review it that 
I can get it off my list?


The only major thing probably is, that calls to getMacAddress don’t 
need a socket anymore and sockets are only opened for platforms where 
an ioctl is done to determine the MacAddress.


As I’ve said, I’ve tested it on the Linux/Unix platforms.

Thanks a lot

Christoph

*From:*Langer, Christoph
*Sent:* Montag, 22. August 2016 15:30
*To:* 'net-dev@openjdk.java.net' 
*Subject:* RE: Ping: RFR(S): 8163181: Further improvements for Unix 
NetworkInterface native implementation


Hi,

I made a little update to my change: 
http://cr.openjdk.java.net/~clanger/webrevs/8163181.2/ 
<http://cr.openjdk.java.net/%7Eclanger/webrevs/8163181.2/>


Merely comments and a minor AIX specific correction.

Thanks in advance for reviewing

Christoph

*From:*Langer, Christoph
*Sent:* Dienstag, 16. August 2016 14:49
*To:* 'net-dev@openjdk.java.net' <mailto:net-dev@openjdk.java.net>>
*Subject:* Ping: RFR(S): 8163181: Further improvements for Unix 
NetworkInterface native implementation


Ping: Can I get a review for this small set of changes please?

Thanks

Christoph

*From:*Langer, Christoph
*Sent:* Donnerstag, 4. August 2016 15:04
*To:* net-dev@openjdk.java.net <mailto:net-dev@openjdk.java.net>
*Subject:* RFR(S): 8163181: Further improvements for Unix 
NetworkInterface native implementation


Hi,

I had made a few more cleanups when I was working on 
NetworkInterface.c which I thought are worth contributing.


Please review: http://cr.openjdk.java.net/~clanger/webrevs/8163181.1/ 
<http://cr.openjdk.java.net/%7Eclanger/webrevs/8163181.1/>


Bug: https://bugs.openjdk.java.net/browse/JDK-8163181 
<https://bugs.openjdk.java.net/browse/JDK-8163181>


As always, changes were built and tested on Linux, AIX, Solaris and Mac.

Thanks

Christoph

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

[Mark Sheppard]

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

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


int NET_TimeoutWithCurrentTime(int s, long timeout, stuct timeval * 
current_time) {

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

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

}

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

the NET_ReadWithTimeout is modified with.

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

in any case there is the potential for multiple invocation of 
gettimeofday due to a need to make
poll restartable, and adjust the originally specified timeout 
accordingly, and for the edge case

after the non blocking read.

regards
Mark



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

Vyom,

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


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

-Chris.

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

hi Dimitry,

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

Thanks,

Vyom


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

Vyom,

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

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

-Dmitry

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

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

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

incorporated the review comments.

Thanks,

Vyom


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

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

along the lines

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


the NET_ReadWithTimeout (...) function will contain a 
restructuring of

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

e&oe

regards
Mark


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

gentle reminder, please review the below code change.

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

[Mark Sheppard]

looks fine ... thanks

regards
Mark

On 06/09/2016 10:20, Vyom Tewari wrote:

Hi,

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


Thanks,

Vyom


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

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


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


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


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

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

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

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

the NET_ReadWithTimeout is modified with.

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

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


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

 and adjust the originally specified timeout

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


After an initial skim, your suggestion seems reasonable.

-Chris.


regards
Mark



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

Vyom,

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



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

-Chris.

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

hi Dimitry,

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

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

Thanks,

Vyom


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

Vyom,

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

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

syscall.

-Dmitry

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

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



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


I
incorporated the review comments.

Thanks,

Vyom


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

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

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

along the lines

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


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

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

[Mark Sheppard]

my error ... i perceived NET_Timeout as a common function across all 
platforms
otherwise I would have suggested a NET_TimeoutWithCurrentTime for each 
platform.


providing a version of NET_Timeout with a recv function sound good, but
will require a version per platform (NET_TimeoutWithRecv) and such as a 
change will affect the network
code more extensively, otherwise you need to retain the NET_Timeout 
also,  as it is used in other part of native network.


This change is focused on SocketInputStream, but that is not only place 
where NET_Timeout is called.


either way there is the possibility of replicated code

regards
Mark






On 06/09/2016 13:55, Chris Hegarty wrote:

Vyom,


On 6 Sep 2016, at 10:20, Vyom Tewari  wrote:

Hi,

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

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

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

-Chris.


Thanks,

Vyom


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

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

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

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


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

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

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

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

the NET_ReadWithTimeout is modified with.

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

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

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

and adjust the originally specified timeout

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

After an initial skim, your suggestion seems reasonable.

-Chris.


regards
Mark



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

Vyom,

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

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

-Chris.

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

hi Dimitry,

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

Thanks,

Vyom


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

Vyom,

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

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

-Dmitry

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

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

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

Thanks,

Vyom


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

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

[Mark Sheppard]

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

appropriate javadoc ?

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


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


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

rather than

// Enable SO_REUSEADDR before binding
setReuseAddress 
(*true*);


// Enable SO_REUSEPORT if supported before binding
*if*  (supportedOptions 
().contains 
(StandardSocketOptions 
.SO_REUSEPORT 
)) {
*this*.setOption 
(StandardSocketOptions 
.SO_REUSEPORT 
,*true*);

}


as the overridden setReuseAddress takes care of SO_REUSEPORT

regards
Mark

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

Vyom,

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

Hi All,

Please review the below code change.

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

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


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


This issue arises since the changes for 6432031 "Add support for 
SO_REUSEPORT" [1], which sets SO_REUSEPORT on MulticastSocket, if

the available. So setting setReuseAddress(false) alone is no longer
sufficient to disable address/port reuse, one must also set
SO_REUSEPORT to false.

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

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

If so, then code would have to:

setReuseAddress(false);

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

  , but at least it is explicit.

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

-Chris.

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

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

[Mark Sheppard]

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


I don't see Datagram.setReuseAddress(...) handling the SO_REUSEPORT 
option, this has to be done explicitly via setOption at this level of 
abstraction.


MulticastSocket is a subclass of DatagramSocket (that in itself is a 
questionable structuring), and as such
has specialized behaviour, and part  of that specialization is the 
setting of  the setting SO_REUSEADDR and SO_REUSEPORT
in its constructors, to enable address reuse semantics, prior to binding 
an address.
As part of that specialization, it would seem appropriate that 
MulticastSocket manipulate the SO_REUSEPORT
option in a consistent way. Adding an overridden setReuseAddress(...) 
method provides that consistency and

encapsulates the specialized behaviour.

Is alternatively proposal to NOT do anything to MulticastSocket, BUT 
document clearly how to handle the failing scenario, that an MulticastSocket
requires both setReuseAddress() and a setOption call to disable 
reuseaddress semantics on an unbound MulticastSocket ?



This then raises the question of why have a convenience method, such as 
setReuseAddress() in the first place, when it can be handled

adequately via the setOption

regards
Mark

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

Mark,

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


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


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


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


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


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


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


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

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



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

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

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

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

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

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

<https://java.se.oracle.com/source/s?defs=SO_REUSEPORT&project=jdk9-dev>, 
*true*);

}


as the overridden setReuseAddress takes care of SO_REUSEPORT


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

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

-Chris.



regards
Mark

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

Vyom,

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

Hi All,

Please review the below code change.

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

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

This change override the "get/setReuseAddress"  for MulticaseSocket 
and

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


This issue arises since the changes for 6432031 "Add support for
SO_REUSEPORT" [1], which sets SO_REUSEPORT on MulticastSocket, if
the available. So setting setReuseAddress(false) alone is no longer
sufficient to disable address/port reuse, one must also set
SO_REUSEPORT to false.

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

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

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

[Mark Sheppard]

that's true wrt SO_REUSEPORT in MulticastSocket constructor. But the 
same could have been argued for the original
invocation of setReuseAddress, by default , in the constructors, which 
is encapsulating, what pereceived as, common or defacto

practice wrt applying SO_REUSEADDR on mcast sockets at the system level.
As I understand it, it is generally perceived that SO_REUSEADDR and 
SO_REUSEPORT are semantically equivalent for multicast sockets.
As such, I think in the case of MulticastSocket, the fact that the 
setRuseAddress() is called in the constructor, it is appropriate

to set SO_REUSEPORT also when it exists in the OS.

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


regards
Mark



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

One additional remark.

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

-Chris.

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

Mark,

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


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


The concern is with setReuseAddress performing an operation that
is not clear from the method specification, e.g. from setReuseAddress()

 * Enable/disable the SO_REUSEADDR socket option.

This is no longer accurate. The proposed changes would affect
SO_REUSEPORT too.


I don't see Datagram.setReuseAddress(...) handling the SO_REUSEPORT
option, this has to be done explicitly via setOption at this level of
abstraction.


Yes, it is a bit odd, but these are legacy classes. I am not opposed
to adding a new method for this, or something else. I just want to
avoid future issues and confusion when setReuseAddress is called and
then it is noticed that, the somewhat orthogonal option, SO_REUSEPORT's
value has changed. setReuseAddress's spec is very clear about what it
does.


MulticastSocket is a subclass of DatagramSocket (that in itself is a
questionable structuring), and as such
has specialized behaviour, and part  of that specialization is the
setting of  the setting SO_REUSEADDR and SO_REUSEPORT
in its constructors, to enable address reuse semantics, prior to 
binding

an address.


Understood. Of course, the setting of SO_REUSEPORT is new in 9,
hence the problem.


As part of that specialization, it would seem appropriate that
MulticastSocket manipulate the SO_REUSEPORT
option in a consistent way. Adding an overridden setReuseAddress(...)
method provides that consistency and
encapsulates the specialized behaviour.


I agree with the abstraction, just not that setReuseAddress should
be used to achieve it. The name and spec of this method is so
tied to SO_REUSEADDR.


Is alternatively proposal to NOT do anything to MulticastSocket, BUT
document clearly how to handle the failing scenario, that an
MulticastSocket
requires both setReuseAddress() and a setOption call to disable
reuseaddress semantics on an unbound MulticastSocket ?


That is one option, and the option that I was suggesting as a possible
alternative.


This then raises the question of why have a convenience method, such as
setReuseAddress() in the first place, when it can be handled
adequately via the setOption


We are moving away from these option specific getter and setter
methods, in favor of the more general get/setOption methods, as
the latter are more adaptable.

If setReuseAddress is to operate on more than SO_REUSEADDR, then
its spec should be very clear about this.

-Chris.



regards
Mark

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

Mark,

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


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


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

or are you advocating pushing the handing of the SO_REUSEPORT into 
the

base DatagramSocket class ?


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

It is not clear how your code changes fit in the proposed fix i.e. 
the

exp