Code Review Request: 7035556 DatagramSocket.java:183: warning: unreachable catch clause

[Kurchi Hazra]

Hi,

Due to a recent update in javac to issue a warning on detecting 
unreachable code, the following warning started showing up in the jdk 
networking code:
../../../src/share/classes/java/net/DatagramSocket.java:183: warning: 
unreachable catch clause.


This fix aims at removing this warning by removing the IOException. On 
inspection, it was found that currently, the native code does not throw 
any IOException.


The fix involves updates in:
jdk/src/share/classes/java/net/DatagramSocket.java


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

Thanks,
-Kurchi

Re: Code Review Request: 7035556 DatagramSocket.java:183: warning: unreachable catch clause

[Kurchi Hazra]

Hi,

  I changed the implementation according to Brad's comments. I am 
reposting the output of hg diff 
src/share/classes/java/net/DatagramSocket.java since I don't have an 
openjdk account:


bash-3.00$ hg diff src/share/classes/java/net/DatagramSocket.java
diff --git a/src/share/classes/java/net/DatagramSocket.java 
b/src/share/classes/java/net/DatagramSocket.java

--- a/src/share/classes/java/net/DatagramSocket.java
+++ b/src/share/classes/java/net/DatagramSocket.java
@@ -176,13 +176,7 @@ class DatagramSocket implements java.io.
 public DatagramSocket() throws SocketException {
 // create a datagram socket.
 createImpl();
-try {
-bind(new InetSocketAddress(0));
-} catch (SocketException se) {
-throw se;
-} catch(IOException e) {
-throw new SocketException(e.getMessage());
-}
+bind(new InetSocketAddress(0));
 }


Thanks,
Kurchi



On 7/22/2011 7:25 AM, Michael McMahon wrote:

On 22/07/11 14:55, Alan Bateman wrote:

Michael McMahon wrote:
But, bind() already closes the impl internally before throwing the 
exception.
I was wondering about that and whether this is a bug. Suppose someone 
creates an unbound DatagramSocket and then attempts to bind it to a 
port. If the bind fails (say port already in use) then it may be 
surprising that they can't retry with a different port.  Should this 
be specified? I see there are cases such as the security exception 
where it doesn't the close the impl.


-Alan.
It doesn't seem to be specified either way, though it does seem to be 
inconsistent.

I'd be wary about changing the behaviour though
unless there was a strong justification (more than a compiler warning :) )

- Michael.

Re: hg: jdk8/tl/jdk: 7035556: DatagramSocket.java:183: warning: unreachable catch clause

[Kurchi Hazra]

Thanks a lot and I look forward to making more contributions too!

Thanks,
Kurchi

On 7/25/2011 2:52 PM, Chris Hegarty wrote:
Congratulation Kurchi on your first contribution to OpenJDK. Hope to 
see more in the future.


-Chris.

On 07/25/11 10:42 PM, chris.hega...@oracle.com wrote:

Changeset: 07a12583d4ea
Author:chegar
Date:  2011-07-25 14:35 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/07a12583d4ea

7035556: DatagramSocket.java:183: warning: unreachable catch clause
Summary: Remove redundant catches in bind
Reviewed-by: alanb, michaelm, wetmore, chegar
Contributed-by: kurchi.subhra.ha...@oracle.com

! src/share/classes/java/net/DatagramSocket.java



--
-Kurchi

Code Review Request: 6978200 ServerSocket.toString include "port=0" in the returned String

[Kurchi Hazra]

Hi,

  The ServerSocket.toString method returns a string that, besides the 
ServerSocket's local port information, also contains "port=0". For 
example: "ServerSocket[addr=0.0.0.0/0.0.0.0,port=0,localport=7005]".


The fix aims at modifying the string returned by the toString method by 
removing impl.getPort() from the method and subsequently "port=0" from 
it. Although we know that anyone parsing toString (and ignoring port) 
may suffer from this change, but we don't expect that many (if anyone ) 
is doing this since accessor methods provide easier access to the local 
address and port.


 The fix involves updates in:
jdk/src/share/classes/java/net/ServerSocket.java



I am posting the output of hg diff here:

bash-3.00$ hg diff src/share/classes/java/net/ServerSocket.java
diff -r a80562f7ea50 src/share/classes/java/net/ServerSocket.java
--- a/src/share/classes/java/net/ServerSocket.java  Wed Jul 27 
18:10:10 2011 +0100
+++ b/src/share/classes/java/net/ServerSocket.java  Thu Jul 28 
14:16:10 2011 -0700

@@ -716,7 +716,6 @@ class ServerSocket implements java.io.Cl
 if (!isBound())
 return "ServerSocket[unbound]";
 return "ServerSocket[addr=" + impl.getInetAddress() +
-",port=" + impl.getPort() +
 ",localport=" + impl.getLocalPort()  + "]";
 }


Thanks!

--
-Kurchi

Code Review Request: 7084032: test/java/net/Inet6Address/B6558853.java fails on Windows XP/2003 if IPv6 enabled

[Kurchi Hazra]

Hi,

 test/java/net/Inet6Address/B6558853.java tests if the address obtained from 
getHostAddress() on connections
 using IPv6 link-local addresses contains the zone id. For 
Inet6Address.getHostAddress() to return the zone id,
 Inet6Address.scope_id_set needs to be set to true in addition to setting the 
appropriate Inet6Address.scope_id.
 In case of Windows XP/2003 with IPv6 enabled, the native socket implementation 
of socketAccept() method in
 src/windows/native/java/net/TwoStacksPlainSocketImpl.c does not set the 
scope_id_set to true which causes
 the zone id not to be returned, and B6558853 throws a RuntimeException.

The fix is to simply set the scopeidsetID in socketAccept() method to true if 
the scope_id is greater than 0.



Submitting hg diff:

diff --git a/src/windows/native/java/net/TwoStacksPlainSocketImpl.c 
b/src/windows/native/java/net/TwoStacksPlainSocketImpl.c
--- a/src/windows/native/java/net/TwoStacksPlainSocketImpl.c
+++ b/src/windows/native/java/net/TwoStacksPlainSocketImpl.c
@@ -576,6 +576,7 @@ Java_java_net_TwoStacksPlainSocketImpl_s
 {
 /* fields on this */
 jint port;
+jint scope;
 jint timeout = (*env)->GetIntField(env, this, psi_timeoutID);
 jobject fdObj = (*env)->GetObjectField(env, this, psi_fdID);
 jobject fd1Obj = (*env)->GetObjectField(env, this, psi_fd1ID);
@@ -755,7 +756,11 @@ Java_java_net_TwoStacksPlainSocketImpl_s
 addr = (*env)->GetObjectField (env, socketAddressObj, ia6_ipaddressID);
 (*env)->SetByteArrayRegion (env, addr, 0, 16, (const char 
*)&him.him6.sin6_addr);
 (*env)->SetIntField(env, socketAddressObj, ia_familyID, IPv6);
-(*env)->SetIntField(env, socketAddressObj, ia6_scopeidID, 
him.him6.sin6_scope_id);
+scope=him.him6.sin6_scope_id;
+(*env)->SetIntField(env, socketAddressObj, ia6_scopeidID, scope);
+if(scope>0) {
+(*env)->SetBooleanField(env, socketAddressObj, ia6_scopeidsetID, 
JNI_TRUE);
+}
 }
 /* fields common to AF_INET and AF_INET6 */


Thanks,

--
-Kurchi

Code Review Request: 7090158 Networking Libraries don't build with javac -Werror

[Kurchi Hazra]

Hi,

This is an attempt to remove all build warnings that are related to 
networking in jdk. This work contains


(a) Small changes in java files to remove already existing warnings
(b) Small changes in Makefiles to prevent re-introduction of such warnings.


Webrev: http://cr.openjdk.java.net/~chegar/7090158/webrev/


Thanks,
-Kurchi

Re: Code Review Request: 7090158 Networking Libraries don't build with javac -Werror

[Kurchi Hazra]

Something went wrong in the pasting. Can you check if this works fine: 
http://cr.openjdk.java.net/~chegar/7090158/webrev/


On 9/13/2011 12:07 PM, Alan Bateman wrote:

Kurchi Hazra wrote:


Hi,

This is an attempt to remove all build warnings that are related to 
networking in jdk. This work contains


(a) Small changes in java files to remove already existing warnings
(b) Small changes in Makefiles to prevent re-introduction of such 
warnings.



Webrev: http://cr.openjdk.java.net/~chegar/7090158/webrev/

I think Michael has removed this code as part of 7035556 [1]. Also 
this seems to be the only change in the webrev (there isn't any 
Makefile changes).


-Alan.

[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/07a12583d4ea


--
-Kurchi

Re: Code Review Request: 7090158 Networking Libraries don't build with javac -Werror

[Kurchi Hazra]

Updated webrev : http://cr.openjdk.java.net/~weijun/7090158/webrev.00/. 
This should build correctly.


Thanks,
Kurchi


On 9/13/2011 7:55 PM, Weijun Wang wrote:
I apply the patch to my local repository and do a clean rebuild of 
jdk-only. It shows 1 error and 92 warnings in javax and stopped. Most 
in src/share/classes/javax/xml/crypto/dsig and I remember Sean said 
it's not easy to remove all warnings there because the codes are 
shared between JDK and some Apache projects.


-Max

On 09/14/2011 03:13 AM, Alan Bateman wrote:

Kurchi Hazra wrote:

Something went wrong in the pasting. Can you check if this works fine:
http://cr.openjdk.java.net/~chegar/7090158/webrev/

Yes, this webrev has what I expected to see.

-Alan


--
-Kurchi

Re: Code Review Request: 7090158 Networking Libraries don't build with javac -Werror

[Kurchi Hazra]

I remember having made this change as follows:
-bash-3.00$ hg diff src/share/classes/java/net/Socket.java
diff --git a/src/share/classes/java/net/Socket.java 
b/src/share/classes/java/net/Socket.java

--- a/src/share/classes/java/net/Socket.java
+++ b/src/share/classes/java/net/Socket.java
@@ -459,10 +459,10 @@ class Socket implements java.io.Closeabl
 oldImpl = AccessController.doPrivileged
 (new PrivilegedAction() {
 public Boolean run() {
-Class[] cl = new Class[2];
-cl[0] = SocketAddress.class;
-cl[1] = Integer.TYPE;
-Class clazz = impl.getClass();
+   Class[] cl = new Class[2];
+   cl[0] = SocketAddress.class;
+   cl[1] = Integer.TYPE;
+   Class clazz = impl.getClass();
 while (true) {
 try {
 clazz.getDeclaredMethod("connect", cl);


I am not sure why webrev is not picking these changes, but these 
warnings did show up.



Thanks,
Kurchi

On 9/14/2011 7:11 AM, Chris Hegarty wrote:

Maurizio and I noticed another issue in java.net.Socket

> Class[] cl = new Class[2];

should generate a warning, but does not. Maurizio filed CR 7090499 
against this. It is a compiler bug.


It should be: Class[] cl = new Class[2];

It is best that we fix this before pushing as it will break the build 
if we wait until after 7090499 is fixed. Also we may be better off 
waiting until after Maurizio runs a JPRT job with his fix for 7090499. 
We may have other failures in areas where Sasha enabled warnings.


-Chris.

On 14/09/2011 14:17, Chris Hegarty wrote:

Thanks for reviewing this Max, I also went through the changes.

MessageHeader.filterAndAddHeaders() and
HttpURLConnection.getRequestproperties() also confused me. Essentially
this never work quite right. I had Maurizio ( javac guy ) look at the
old code with me and we figured out that the generic type on
filterAndAddHeaders was never enforced by the compiler as it was being
passed a raw Map.

The code in filterAndAddHeaders assumes that the values in the map are
Strings even though the type is declared as List. Again this
cannot be enforced as the only client of filterAndAddHeaders is passing
a raw Map. Anyway, it worked somehow!

What Kurchi has now looks right to me.

-Chris.

On 14/09/2011 06:22, Weijun Wang wrote:



On 09/14/2011 12:14 PM, Kurchi Hazra wrote:
Updated webrev : 
http://cr.openjdk.java.net/~weijun/7090158/webrev.00/.

This should build correctly.


Yes, it does!

Some comments:

1. make/java/Makefile has no real change

2. make/javax/others/Makefile has only a new commented line

3. java/net/DatagramSocket.java and java/net/MulticastSocket.java have
some real code changes around bind(). Maybe they should go to another
fix?

4. sun/net/www/MessageHeader.java:

231 for(Map.Entry> entry : include.entrySet()) {

There should be a blank between "for" and "(", but probably not
necessary between "String," and "List" or "entry" and ":". You decide.

234 l = new ArrayList<>();

Do we have a consensus on whether diamond can be used here? i.e.
assignment not on declaration.

Another thing:

I'm confused by the use of MessageHeader.filterAndAddHeaders() inside
HttpURLConnection.getRequestproperties() methods. Looking at the old
codes, it seems the userCookiesMap (in
HttpURLConnection.getRequestproperties) variable is only a
Map, but the 2nd argument of filterAndAddHeaders() has
been declared as Map> for some time, then again, 
the
old filterAndAddHeaders() calls "l.add(entry.getValue())" which 
suggests

the value of the map is still only String.

My current understanding is that both the old codes and Kurchi's code
changes work but the old one's method declaration is not correct. Also,
if the include argument never contains multiple (or empty) string 
values

for the same key, we can simply use Map.

Thanks
Max




Thanks,
Kurchi


On 9/13/2011 7:55 PM, Weijun Wang wrote:

I apply the patch to my local repository and do a clean rebuild of
jdk-only. It shows 1 error and 92 warnings in javax and stopped. Most
in src/share/classes/javax/xml/crypto/dsig and I remember Sean said
it's not easy to remove all warnings there because the codes are
shared between JDK and some Apache projects.

-Max

On 09/14/2011 03:13 AM, Alan Bateman wrote:

Kurchi Hazra wrote:

Something went wrong in the pasting. Can you check if this works
fine:
http://cr.openjdk.java.net/~chegar/7090158/webrev/

Yes, this webrev has what I expected to see.

-Alan




--
-Kurchi

Re: Code Review Request: 7090158 Networking Libraries don't build with javac -Werror

[Kurchi Hazra]

3. java/net/DatagramSocket.java and java/net/MulticastSocket.java have
some real code changes around bind(). Maybe they should go to another fix?

I think there is some merging problem in my workspace. I will probably 
start with an updated copy of these files and insert my changes in them.



Some comments:

1. make/java/Makefile has no real change

2. make/javax/others/Makefile has only a new commented line


4. sun/net/www/MessageHeader.java:

231 for(Map.Entry> entry : include.entrySet()) {

There should be a blank between "for" and "(", but probably not
necessary between "String," and "List" or "entry" and ":". You decide.



Will take care of these and post a new webrev.




234 l = new ArrayList<>();

Do we have a consensus on whether diamond can be used here? i.e.
assignment not on declaration.





Waiting for someone's input on this. In retrospect, I feel this is not 
exactly how the diamond operator is meant to be used when I refer to 
http://download.oracle.com/javase/7/docs/technotes/guides/language/type-inference-generic-instance-creation.html, 
although this particular situation has not been commented upon.



Thanks,
Kurchi

Re: Code Review Request: 7090158 Networking Libraries don't build with javac -Werror

[Kurchi Hazra]

Updated webrev:

http://cr.openjdk.java.net/~sherman/kurchi/7090158/webrev/

Thanks,
Kurchi

On 9/14/2011 12:42 PM, chris hegarty wrote:

Kurchi,

The problem here is that due to a bug in the compiler, CR 7090499, we 
are not seeing raw type warnings for anonymous inner classes.


When Maurizio fixes this bug it is very likely that other areas of the 
jdk where Sasha enabled -Werror will break the build.


If you like you can complete what you have in the networking area and 
we can file a separate CR for this new issue. But we should make it a 
priority.


Tom:
>
>Or better, remove cl and change:
>
>clazz.getDeclaredMethod("connect", cl);
>
>to:
>clazz.getDeclaredMethod(
>"connect", SocketAddress.class, int.class
>);

Thanks Tom, this suggestion is much better. Kurchi we should probably 
run with this.


-Chris.

On 14/09/2011 16:46, Kurchi Hazra wrote:

I remember having made this change as follows:
-bash-3.00$ hg diff src/share/classes/java/net/Socket.java
diff --git a/src/share/classes/java/net/Socket.java
b/src/share/classes/java/net/Socket.java
--- a/src/share/classes/java/net/Socket.java
+++ b/src/share/classes/java/net/Socket.java
@@ -459,10 +459,10 @@ class Socket implements java.io.Closeabl
oldImpl = AccessController.doPrivileged
(new PrivilegedAction() {
public Boolean run() {
- Class[] cl = new Class[2];
- cl[0] = SocketAddress.class;
- cl[1] = Integer.TYPE;
- Class clazz = impl.getClass();
+ Class[] cl = new Class[2];
+ cl[0] = SocketAddress.class;
+ cl[1] = Integer.TYPE;
+ Class clazz = impl.getClass();
while (true) {
try {
clazz.getDeclaredMethod("connect", cl);


I am not sure why webrev is not picking these changes, but these
warnings did show up.


Thanks,
Kurchi

On 9/14/2011 7:11 AM, Chris Hegarty wrote:

Maurizio and I noticed another issue in java.net.Socket

> Class[] cl = new Class[2];

should generate a warning, but does not. Maurizio filed CR 7090499
against this. It is a compiler bug.

It should be: Class[] cl = new Class[2];

It is best that we fix this before pushing as it will break the build
if we wait until after 7090499 is fixed. Also we may be better off
waiting until after Maurizio runs a JPRT job with his fix for 7090499.
We may have other failures in areas where Sasha enabled warnings.

-Chris.

On 14/09/2011 14:17, Chris Hegarty wrote:

Thanks for reviewing this Max, I also went through the changes.

MessageHeader.filterAndAddHeaders() and
HttpURLConnection.getRequestproperties() also confused me. Essentially
this never work quite right. I had Maurizio ( javac guy ) look at the
old code with me and we figured out that the generic type on
filterAndAddHeaders was never enforced by the compiler as it was being
passed a raw Map.

The code in filterAndAddHeaders assumes that the values in the map are
Strings even though the type is declared as List. Again this
cannot be enforced as the only client of filterAndAddHeaders is 
passing

a raw Map. Anyway, it worked somehow!

What Kurchi has now looks right to me.

-Chris.

On 14/09/2011 06:22, Weijun Wang wrote:



On 09/14/2011 12:14 PM, Kurchi Hazra wrote:

Updated webrev :
http://cr.openjdk.java.net/~weijun/7090158/webrev.00/.
This should build correctly.


Yes, it does!

Some comments:

1. make/java/Makefile has no real change

2. make/javax/others/Makefile has only a new commented line

3. java/net/DatagramSocket.java and java/net/MulticastSocket.java 
have

some real code changes around bind(). Maybe they should go to another
fix?

4. sun/net/www/MessageHeader.java:

231 for(Map.Entry> entry : include.entrySet()) {

There should be a blank between "for" and "(", but probably not
necessary between "String," and "List" or "entry" and ":". You 
decide.


234 l = new ArrayList<>();

Do we have a consensus on whether diamond can be used here? i.e.
assignment not on declaration.

Another thing:

I'm confused by the use of MessageHeader.filterAndAddHeaders() inside
HttpURLConnection.getRequestproperties() methods. Looking at the old
codes, it seems the userCookiesMap (in
HttpURLConnection.getRequestproperties) variable is only a
Map, but the 2nd argument of filterAndAddHeaders() has
been declared as Map> for some time, then again,
the
old filterAndAddHeaders() calls "l.add(entry.getValue())" which
suggests
the value of the map is still only String.

My current understanding is that both the old codes and Kurchi's code
changes work but the old one's method declaration is not correct. 
Also,

if the include argument never contains multiple (or empty) string
values
for the same key, we can simply use Map.

Thanks
Max




Thanks,
Kurchi


On 9/13/2011 7:55 PM, Weijun Wang wrote:

I apply the patch to my local repository and do a clean rebuild of
jdk-only. It shows 1 error and 92 warnings in javax an

Code Review Request: 7084030 DatagramSocket.getLocalAddress inconsistent on XP/2003 when IPv6 enabled and socket is connected

[Kurchi Hazra]

Hi,

The DatagramSocket.getLocalAddress() method was returning a 
wildcard address when a DatagramSocket is created and then connected to 
a remote address, on windows XP/Server 2003 machines with IPv6 enabled. 
However, for adhering to the spec as well as being consistent across all 
platforms, it is desirable for DatagramSocket.getLocalAddress() to 
return the local address that it is bound to on being connected. The 
cause of the problem was incorrect handling of the file descriptors 
representing the two stacks of IPv4 and IPv6 in Windows XP/Server 2003 
specific native code.
In order to fix this, a new native method socketLocalAddress() has 
been introduced in 
src/windows/native/java/net/TwoStacksPlainDatagramSocketImpl.c which 
takes care of handling the file descriptors correctly when the 
DatagramSocket in question is connected to a remote address. The method 
accepts the family of the address to which it is connected (IPv4 or 
IPv6) and picks up the localAddress information from the concerned file 
descriptor (fd for IPv4 and fd1 for IPv6). 
src/windows/classes/java/net/TwoStacksPlainDatagramSocketImpl.java has 
been modified accordingly to call the socketLocalAddress() instead of 
socketGetOption() when the option passed to DatagramSocket.getOption() 
is SocketOptions.SO_BINDADDR as in the case of 
DatagramSocket.getLocalAddress().


The fix involves updates in:
src/share/classes/java/net/AbstractPlainDatagramSocket
src/windows/classes/java/net/TwoStacksPlainDatagramSocketImpl.java
src/windows/native/java/net/TwoStacksPlainDatagramSocketImpl.c

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


Thanks,
Kurchi

Code Review Request: 6953455 CookieStore.add() cannot handle null URI parameter, contrary to the API specification

[Kurchi Hazra]

Hi,

The CookieStore.add() method throws a Null Pointer Exception when 
null is passed as the uri parameter, although this is allowed according 
to the method spec.


 The exception is thrown because uri.getHost() is called on a null 
uri in an effort to add it to the uriIndex, one of the hash maps 
constituting the CookieStore. The fix would be to simply bypass adding 
the cookie to the uriIndex when uri is null.



The fix involves updates in:
src/share/classes/java/net/InMemoryCookieStore.java

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

Thanks,
Kurchi

Code Review Request: 7107020: java.net.PlainSocketImpl.socketSetOption() calls itself

[Kurchi Hazra]

Hi,

 As specified in the CR description, this is a bug in 
src/windows/java/net/PlainSocketImpl.java and impl.socketSetOption() should be 
called instead of socketSetOption().

 This bug did not create a problem yet since setOption() is usually called for 
setting socket options.

Submitting hg diff:

  diff --git a/src/windows/classes/java/net/PlainSocketImpl.java 
b/src/windows/classes/java/net/PlainSocketImpl.java
--- a/src/windows/classes/java/net/PlainSocketImpl.java
+++ b/src/windows/classes/java/net/PlainSocketImpl.java
@@ -314,7 +314,7 @@ class PlainSocketImpl extends AbstractPl

 void socketSetOption(int cmd, boolean on, Object value)
 throws SocketException {
-socketSetOption(cmd, on, value);
+impl.socketSetOption(cmd, on, value);
 }

 int socketGetOption(int opt, Object iaContainerObj) throws SocketException 
{

Thanks,

--
-Kurchi

Re: Review request: 7120875 fix macos ipv6 issue and update multiple test scripts

[Kurchi Hazra]

On 12/14/2011 11:54 AM, Alan Bateman wrote:

On 14/12/2011 19:39, Michael McMahon wrote:

Hi,

This webrev fixes a bunch of test script issues in networking and 
core-libs.
In most if not all cases, the change relates to recognising the OS 
that the test

is running on.

It also fixes an IPv6 issue, which requires Macos versions of a 
couple of java.net
classes. These are the new files at the end of the webrev. The change 
is to provide

a default sin6_scope_id when none is provided by the user.

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

Thanks,
Michael.
Michael - I haven't studied the IPv6 changes yet but I'm concerned 
that this creates a copy of java.net.MulticastSocket. Would some 
refactoring allow us to avoid this?


In test/sun/net/www/protocol/jar/jarbug/run.sh then maybe it would 
make sense to combine SunOS, Linux and Darin into the one case. More 
generally then I guess most of these tests could be changed so that 
they default to PS=":" FS="/" when not on Windows or cygwin. Did you 
check the java code for the tests in these areas for any cases where 
they test based on os.name?


I made that shell script change - I agree that all can be combined into 
one case but I just stuck to the general format in those files.





-Alan.




--
-Kurchi

Re: Review request: 7120875 fix macos ipv6 issue and update multiple test scripts

[Kurchi Hazra]

On 12/14/2011 12:15 PM, Alan Bateman wrote:

On 14/12/2011 20:05, Kurchi Hazra wrote:


On 12/14/2011 11:54 AM, Alan Bateman wrote:

:

In test/sun/net/www/protocol/jar/jarbug/run.sh then maybe it would 
make sense to combine SunOS, Linux and Darin into the one case. More 
generally then I guess most of these tests could be changed so that 
they default to PS=":" FS="/" when not on Windows or cygwin. Did you 
check the java code for the tests in these areas for any cases where 
they test based on os.name?


I made that shell script change - I agree that all can be combined 
into one case but I just stuck to the general format in those files.


That's okay, it's just an inconsistency in 
test/sun/net/www/protocol/jar/jarbug/run.sh. When doing through the 
tests did you look for tests in these areas that look at os.name? I 
know we have a few.


- I did not look through individual tests, but only the ones that failed 
- I assumed that they will throw some error message if the platform is 
not recognized.




-Alan


--
-Kurchi

Re: Review request: 7120875 fix macos ipv6 issue and update multiple test scripts

[Kurchi Hazra]

The changes in the following files are not required I guess - perhaps 
they are my bad


test/java/net/DatagramSocket/B6411513.java

||test/java/net/DatagramSocket/SetDatagramSocketImplFactory/ADatagramSocket.java

- Kurchi*


*


On 12/16/2011 2:36 AM, Michael McMahon wrote:

Updated webrev after Alan's comments.

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

Thanks,
Michael.

On 14/12/11 19:39, Michael McMahon wrote:

Hi,

This webrev fixes a bunch of test script issues in networking and 
core-libs.
In most if not all cases, the change relates to recognising the OS 
that the test

is running on.

It also fixes an IPv6 issue, which requires Macos versions of a 
couple of java.net
classes. These are the new files at the end of the webrev. The change 
is to provide

a default sin6_scope_id when none is provided by the user.

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

Thanks,
Michael.




--
-Kurchi

Code Review Request: 7127771: (macosx)test/java/net/Socket/TrafficClass.java fails on Mac OS X

[Kurchi Hazra]

Hi,

For IPv6 on solaris or linux, setting the traffic class option is 
handled in the java layer.
The native calls to set socket option simply returns a success and get 
socket option

returns a dummy -1 value. test/java/net/Socket/TrafficClass.java
was failing on Mac OS X when using the IPv6 stack with an Invalid 
Argument Socket

Exception since this native handling was missing when setting socket option.

The following change incorporates the required native behavior for Mac OS.

Bug :  http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7127771 (Not 
available yet)

Webrev : http://cr.openjdk.java.net/~khazra/7127771/webrev.00/

Thanks,
Kurchi

Re: Code Review Request: 7127771: (macosx)test/java/net/Socket/TrafficClass.java fails on Mac OS X

[Kurchi Hazra]

Resending this request

- Kurchi



On 1/11/2012 11:15 AM, Kurchi Hazra wrote:


Hi,

For IPv6 on solaris or linux, setting the traffic class option is 
handled in the java layer.
The native calls to set socket option simply returns a success and get 
socket option

returns a dummy -1 value. test/java/net/Socket/TrafficClass.java
was failing on Mac OS X when using the IPv6 stack with an Invalid 
Argument Socket
Exception since this native handling was missing when setting socket 
option.


The following change incorporates the required native behavior for Mac 
OS.


Bug :  http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7127771
Webrev : http://cr.openjdk.java.net/~khazra/7127771/webrev.00/

Thanks,
Kurchi



--
-Kurchi

Re: Code Review Request: 7127771: (macosx)test/java/net/Socket/TrafficClass.java fails on Mac OS X

[Kurchi Hazra]

How does this look: http://cr.openjdk.java.net/~khazra/7127771/webrev.01/

- Kurchi



On 1/13/2012 12:14 PM, Alan Bateman wrote:



Bug :  http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7127771
Webrev : http://cr.openjdk.java.net/~khazra/7127771/webrev.00/
What you have is fine although you could combine with the Solaris 
code? Should the __ALLBSD_SOURCE XXX be removed while you are there?


-Alan


--
-Kurchi

Re: Code Review Request: 7127771: (macosx)test/java/net/Socket/TrafficClass.java fails on Mac OS X

[Kurchi Hazra]

I updated the comment: http://cr.openjdk.java.net/~khazra/7127771/webrev.02/

- Kurchi



On 1/16/2012 2:43 AM, Michael McMahon wrote:

Yes, looks fine to me too. I would just update the comment above this
code to add Mac OS to the Solaris case.

Thanks
Michael

On 13/01/12 21:02, Kurchi Hazra wrote:
How does this look: 
http://cr.openjdk.java.net/~khazra/7127771/webrev.01/


- Kurchi



On 1/13/2012 12:14 PM, Alan Bateman wrote:



Bug :  http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7127771
Webrev : http://cr.openjdk.java.net/~khazra/7127771/webrev.00/
What you have is fine although you could combine with the Solaris 
code? Should the __ALLBSD_SOURCE XXX be removed while you are there?


-Alan






--
-Kurchi

Code Review Request: 7144274: [macosx] Default IPv6 multicast interface is not being set when calling MulticastSocket.joinGroup()

[Kurchi Hazra]

Hi,

Before joining multicast groups with IPv6 addresses on Mac OS, the
network interface to be used should also be set correctly. This
change enables using a default network interface on Mac OS X if no interface
has been specified.

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

Webrev: http://cr.openjdk.java.net/~khazra/7144274/webrev.01/


Thanks,
Kurchi

Re: Code Review Request: 7144274: [macosx] Default IPv6 multicast interface is not being set when calling MulticastSocket.joinGroup()

[Kurchi Hazra]

Updated webrev: http://cr.openjdk.java.net/~khazra/7144274/webrev.02/

- Kurchi



On 2/14/2012 12:58 AM, Chris Hegarty wrote:
Trivially, I don't think getDefaultScopeID needs it's implementation 
to be ifdef'ed with MACOSX, at all. Then won't need the 'else return 0;'.


It is just a general utility method to get the value of the 
defaultIndex field of NetworkInterace. Yes, the value will be 0 on all 
platforms other than Mac, but equally getDefaultScopeID will not be 
called on any platform, other than Mac.


-Chris.

On 02/14/12 07:46 AM, Alan Bateman wrote:

On 13/02/2012 22:03, Kurchi Hazra wrote:


Hi,

Before joining multicast groups with IPv6 addresses on Mac OS, the
network interface to be used should also be set correctly. This
change enables using a default network interface on Mac OS X if no
interface
has been specified.

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

Webrev: http://cr.openjdk.java.net/~khazra/7144274/webrev.01/

Kurchi - I think you should include an #else return 0; in
getDefaultScopeID as otherwise I will guess you will get warnings when
this code is compiled on other platforms.

-Alan.


--
-Kurchi

Code Review Request: 7144268: [macosx] ProblemList.txt updates to exclude networking tests failing on Mac OS X

[Kurchi Hazra]

Hi,

There are a few networking tests failing on Mac OS X due to the following 
issues:

(a) http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7122846
(b) http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7145658 (Not available 
yet)
(c) http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7143960


For the time being, we plan to exclude the problematic tests when
running tests via the Makefile, until these issues are resolved.

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

Webrev: http://cr.openjdk.java.net/~khazra/7144268/webrev.00/


Thanks,
Kurchi

Code Review Request: 7045655: An empty InMemoryCookieStore should not return true for removeAll

[Kurchi Hazra]

The CookieStore.removeAll() is supposed to return true according to its 
spec, only if the CookieStore changes as a

result of the call.

InMemoryCookieStore:removeAll() was returning true by default, even if 
the CookieStore object was already empty,
and no changes were being done by the call. This fix is to simply return 
false in case the CookieStore is empty.


Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7045655
Webrev: http://cr.openjdk.java.net/~khazra/7045655/webrev.01/


Thanks,
Kurchi

Re: Code Review Request: 7045655: An empty InMemoryCookieStore should not return true for removeAll

[Kurchi Hazra]

Hi Chris,

Updated webrev: http://cr.openjdk.java.net/~khazra/7045655/webrev.02/

- Kurchi


On 3/15/2012 2:49 AM, Chris Hegarty wrote:

Thanks Kurchi, the change look good to me.

This is a corner case and already covered by JCK, but it may be useful 
to amend an existing test to check for this. Maybe 
test/java/net/CookieHandler/NullUriCookieTest.java


CookieStore cookieStore = (new CookieManager()).getCookieStore();
+if (cookieStore.removeAll())
+fail = true;
+checkFail("removeAll on an empty store should return false");
+

-Chris.

On 14/03/12 22:36, Kurchi Hazra wrote:

The CookieStore.removeAll() is supposed to return true according to its
spec, only if the CookieStore changes as a
result of the call.

InMemoryCookieStore:removeAll() was returning true by default, even if
the CookieStore object was already empty,
and no changes were being done by the call. This fix is to simply return
false in case the CookieStore is empty.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7045655
Webrev: http://cr.openjdk.java.net/~khazra/7045655/webrev.01/


Thanks,
Kurchi



--
-Kurchi

Code Review Request: 7152856: TEST_BUG: sun/net/www/protocol/jar/B4957695.java failing on Windows

[Kurchi Hazra]

Hi,

  This test was failing on Windows since it was using the HttpServer in 
test/sun/net/www/httptest. The HttpServer implementation
there is buggy and does not close the connection properly, resulting in 
the test hanging. We therefore write our own server, which sends back
10 bytes less than what the client expects, and see if the client raises 
an IOException.


Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7152856
Webrev: http://cr.openjdk.java.net/~khazra/7152856/webrev.00

Thanks,
Kurchi

Re: Code Review Request: 7152856: TEST_BUG: sun/net/www/protocol/jar/B4957695.java failing on Windows

[Kurchi Hazra]

Hi,

  Thanks for the reviews. Here is an updated webrev: 
http://cr.openjdk.java.net/~khazra/7152856/webrev.01


Thanks,
Kurchi


On 4/15/2012 12:35 AM, Chris Hegarty wrote:

On 14/04/12 16:53, Alan Bateman wrote:

On 13/04/2012 17:59, Kurchi Hazra wrote:

Hi,

This test was failing on Windows since it was using the HttpServer in
test/sun/net/www/httptest. The HttpServer implementation
there is buggy and does not close the connection properly, resulting
in the test hanging. We therefore write our own server, which sends 
back

10 bytes less than what the client expects, and see if the client
raises an IOException.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7152856
Webrev: http://cr.openjdk.java.net/~khazra/7152856/webrev.00

Thanks,
Kurchi

Thanks for sorting out this test. A couple of comments:

- I don't think the @run is right as samevm or agentvm is specified to
jtreg rather than on specific tests (it is possible to add /othervm to
force a test to run in its own VM).

- "Server" might be better than XServer (as X server normally means a
X11 server).

- XServer.srv should be final.

- It looks like the server socket is closed when the test terminates.
Also to ensure that the accepted connection is closed I would suggest
that run be changed to try (Socket s = srv.accept()) { ... }.

Otherwise I think it's okay.


I agree with Alan's comments.

Just to add, no @run tag is needed in this test. The default "@run 
main " [1] should be fine, and allow the test be run in the mode 
specified by the caller. I think this is best where possible.


  "If no @run tags are present in a defining file, a default is assumed
   depending upon the file's filename extension.  For a ".java" file,
   "@run main " is assumed, where  is the name of the file
   without the ".java" suffix.  For a ".sh" file, "@run shell "
   is assumed.  For an ".html" file, "@run applet " is assumed."

-Chris.

[1] http://openjdk.java.net/jtreg/tag-spec.txt



-Alan.






--
-Kurchi

Re: Code Review Request: 7152856: TEST_BUG: sun/net/www/protocol/jar/B4957695.java failing on Windows

[Kurchi Hazra]

Updated webrev:
http://cr.openjdk.java.net/~khazra/7152856/webrev.02/

Alan: On second thoughts, both is and os were not required at all. I 
removed them.


- Kurchi


On 4/17/2012 8:47 AM, Kurchi Subhra Hazra wrote:
I think the HTTP spec needs an http server to handle LF gracefully, 
although it expects a CRLF, and that is how this is working.

It is a small change, I will anyway correct it.

- Kurchi


On 4/17/12 3:24 AM, Chris Hegarty wrote:

On 16/04/12 22:18, Kurchi Hazra wrote:

Hi,

Thanks for the reviews. Here is an updated webrev:
http://cr.openjdk.java.net/~khazra/7152856/webrev.01


The updated webrev looks ok, but the "canned" HTTP response looks funny.

Each HTTP header must be followed by a CRLF ( '\r\n' ), and the end 
of the headers ( just before the response body ) marked by CRLF CRLF. 
Is OutputStreamWriter somehow appending a new line?


Sorry, I think at one time I pointed you to another test that may not 
have been sending a valid HTTP response.


-Chris.



Thanks,
Kurchi


On 4/15/2012 12:35 AM, Chris Hegarty wrote:

On 14/04/12 16:53, Alan Bateman wrote:

On 13/04/2012 17:59, Kurchi Hazra wrote:

Hi,

This test was failing on Windows since it was using the 
HttpServer in

test/sun/net/www/httptest. The HttpServer implementation
there is buggy and does not close the connection properly, resulting
in the test hanging. We therefore write our own server, which sends
back
10 bytes less than what the client expects, and see if the client
raises an IOException.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7152856
Webrev: http://cr.openjdk.java.net/~khazra/7152856/webrev.00

Thanks,
Kurchi

Thanks for sorting out this test. A couple of comments:

- I don't think the @run is right as samevm or agentvm is 
specified to
jtreg rather than on specific tests (it is possible to add 
/othervm to

force a test to run in its own VM).

- "Server" might be better than XServer (as X server normally means a
X11 server).

- XServer.srv should be final.

- It looks like the server socket is closed when the test terminates.
Also to ensure that the accepted connection is closed I would suggest
that run be changed to try (Socket s = srv.accept()) { ... }.

Otherwise I think it's okay.


I agree with Alan's comments.

Just to add, no @run tag is needed in this test. The default "@run
main " [1] should be fine, and allow the test be run in the mode
specified by the caller. I think this is best where possible.

"If no @run tags are present in a defining file, a default is assumed
depending upon the file's filename extension. For a ".java" file,
"@run main " is assumed, where  is the name of the file
without the ".java" suffix. For a ".sh" file, "@run shell "
is assumed. For an ".html" file, "@run applet " is assumed."

-Chris.

[1] http://openjdk.java.net/jtreg/tag-spec.txt



-Alan.










--
-Kurchi

Code Review Request: 7158636: InterfaceAddress.getBroadcast() returns invalid broadcast address on WLAN

[Kurchi Hazra]

Hi,

In Windows Vista and later, InterfaceAddress.getBroadcast() returns 
0.0.0.0 , since these platforms return IF_TYPE_IEEE80211 instead of
MIB_IF_TYPE_ETHERNET for wireless interface type now. The fix is to 
handle IF_TYPE_IEEE80211 in the relevant switch
statement. While doing this, I also updated some other parts of the code 
in the same file that depended on the value of the wireless interface type.


Although I have defined IF_TYPE_IEEE80211 in NetworkInterface.c, I am 
not sure if it is better to put it in NetworkInterface.h instead.


Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7158636
Webrev: http://cr.openjdk.java.net/~khazra/7158636/webrev.00/

Thanks,
Kurchi

Re: Code Review Request: 7158636: InterfaceAddress.getBroadcast() returns invalid broadcast address on WLAN

[Kurchi Hazra]

Updated webrev: http://cr.openjdk.java.net/~khazra/7158636/webrev.01/

Windows has a more secure version of snprintf and I used that: 
http://msdn.microsoft.com/en-us/library/f30dzcf6%28v=vs.110%29.aspx


Thanks,
Kurchi


On 4/17/2012 12:24 PM, Chris Hegarty wrote:

On 17/04/12 20:14, Alan Bateman wrote:

On 17/04/2012 19:47, Kurchi Hazra wrote:

Hi,

In Windows Vista and later, InterfaceAddress.getBroadcast() returns
0.0.0.0 , since these platforms return IF_TYPE_IEEE80211 instead of
MIB_IF_TYPE_ETHERNET for wireless interface type now. The fix is to
handle IF_TYPE_IEEE80211 in the relevant switch
statement. While doing this, I also updated some other parts of the
code in the same file that depended on the value of the wireless
interface type.

Although I have defined IF_TYPE_IEEE80211 in NetworkInterface.c, I am
not sure if it is better to put it in NetworkInterface.h instead.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7158636
Webrev: http://cr.openjdk.java.net/~khazra/7158636/webrev.00/

Thanks,
Kurchi


Looks okay to me although I would like to see this code changed to
snprintf.


D'oh, good catch.

-Chris.



I don't have a strong opinion on where IF_TYPE_IEEE80211 is defined.

-Alan





--
-Kurchi

Code Review Request: 7162385: TEST_BUG: sun/net/www/protocol/jar/B4957695.java failing again

[Kurchi Hazra]

Hi,

  This test was recently fixed via:
http://hg.openjdk.java.net/jdk8/tl/jdk/rev/31c15e2f51ba

  However, it was failing again with a FileNotFoundException since it 
could not find foo1.jar (wrt the test source).

This webrev fixes the test.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7162385
Webrev: http://cr.openjdk.java.net/~khazra/7162385/webrev.00/

Thanks,
Kurchi

Code Review Request : 7144274: [macosx] Default IPv6 multicast interface is not being set when calling MulticastSocket.joinGroup()

[Kurchi Hazra]

Hi,

Before joining multicast groups with IPv6 addresses on Mac OS, the
network interface to be used should also be set correctly. This
change enables using a default network interface on Mac OS X if no
interface has been specified.

 Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7144274
 Webrev: http://cr.openjdk.java.net/~khazra/7144274/webrev_jdk8/webrev.00/

 This change was originally pushed into 7u4[1], but needs to be pushed 
into jdk 8 also. There are no additional changes required for Jdk 8.


 Thanks,
 Kurchi

 [1] > 
http://mail.openjdk.java.net/pipermail/net-dev/2012-February/004078.html

Re: Code Review Request : 7144274: [macosx] Default IPv6 multicast interface is not being set when calling MulticastSocket.joinGroup()

[Kurchi Hazra]

Thanks Chris. I will take your comments into account and push the patch.

- Kurchi


On 4/24/2012 5:46 AM, Chris Hegarty wrote:

Kurchi,

The change looks fine to me. Thanks for forward porting.

Trivially, the indentation of getDefaultScopeID looks a little off. 
Specifically, L121 in the new file:

  ni_defaultIndexID = (*env)->GetStaticFieldID(env, c,
   "defaultIndex", "I");

And indented 5 (one to many) spaces after the closing if bracket.

No need to regenerate the webrev, I'm happy for you to push.

-Chris.

On 23/04/2012 21:46, Kurchi Hazra wrote:

Hi,

Before joining multicast groups with IPv6 addresses on Mac OS, the
network interface to be used should also be set correctly. This
change enables using a default network interface on Mac OS X if no
interface has been specified.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7144274
Webrev: 
http://cr.openjdk.java.net/~khazra/7144274/webrev_jdk8/webrev.00/


This change was originally pushed into 7u4[1], but needs to be pushed
into jdk 8 also. There are no additional changes required for Jdk 8.

Thanks,
Kurchi

[1] >
http://mail.openjdk.java.net/pipermail/net-dev/2012-February/004078.html


--
-Kurchi

Code Review Request: 7171591: getDefaultScopeID() in src/solaris/native/java/net/net_util_md.c should return a value

[Kurchi Hazra]

Hi,

   This is an oversight in the fix for 7144274, where I introduced a method
getDefaultScopeID() to retrieve the default scope id in Mac OS X. The macro
used for checking whether the variable passed is null should return an 
integer

value, since the method getDefaultScopeID() is declared to return an int.

Bug: http://bugs.sun.com/view_bug.do?bug_id=7171591 (Not available yet)
Webrev: http://cr.openjdk.java.net/~khazra/7171591/webrev.00/

Thanks,
Kurchi

Re: Code Review Request: 6953455 CookieStore.add() cannot handle null URI parameter, contrary to the API specification

[Kurchi Hazra]

Hi Neil,

I do not have a problem with you pushing this fix.  Thanks for taking 
this up.


- Kurchi

On 7/5/2012 8:48 AM, Neil Richards wrote:

Hi Chris,
Some QA folk round these parts observed the problem in 7.

I told them of the fix in 8 [1]&  they expressed interest in it being
backported to 7u.

I've confirmed that the problem still exists in the jdk7u code.

I've uploaded a webrev of the fix applied back onto jdk7u-dev [2], for
review.

If approved, I could push the change to jdk7u-dev too, unless that's
considered to be stepping on Kurchi's toes or otherwise bad form.

Regards,
Neil

[1] http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/74f5fef1d961
[2] http://cr.openjdk.java.net/~ngmr/6953455.7u/webrev.00/

On Wed, 2012-07-04 at 21:34 +0100, Chris Hegarty wrote:

Seems like a reasonable candidate for a backport. Are you encountering it in 7?

-Chris

On 4 Jul 2012, at 15:19, Neil Richards  wrote:


On Fri, 2011-09-30 at 10:08 -0700, Kurchi Hazra wrote:


Hi,

The CookieStore.add() method throws a Null Pointer Exception when
null is passed as the uri parameter, although this is allowed
according to the method spec.

 The exception is thrown because uri.getHost() is called on a null
uri in an effort to add it to the uriIndex, one of the hash maps
constituting the CookieStore. The fix would be to simply bypass adding
the cookie to the uriIndex when uri is null.


The fix involves updates in:
src/share/classes/java/net/InMemoryCookieStore.java

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

Thanks,
Kurchi


Hi,
This bug fix looks to have been well bedded into the openjdk 8 code
stream at this point.

Would this be a good item to be applied to the jdk7u code stream ?

Regards,
Neil

--
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU





--
-Kurchi

Re: CR: 7183292: HttpURLConnection.getHeaderFields() throws IllegalArgumentException: Illegal cookie name

[Kurchi Hazra]

- Looks fine to me.

- Kurchi


On 7/18/2012 10:38 AM, Michael McMahon wrote:

Thanks Kurchi.

I have made one small change to another test, which was specifically 
testing the $name assertion.

So, that test had to be removed.

The new webrev is at :

http://cr.openjdk.java.net/~michaelm/7183292/webrev.3/

- Michael

On 17/07/12 18:15, Kurchi Subhra Hazra wrote:
I have read the sections dealing with cookie-name in 6265, and these 
changes look good to me.


- Kurchi

On 7/17/12 7:32 AM, Michael McMahon wrote:


Thanks for reviewing this Chris. On the question of whether $ should 
be allowed
in cookie names, it appears like that restriction has been removed 
from RFC 6265,
which is evidently a fairly comprehensive description of actual 
cookie usage on the web.
So, maybe we should just leave that out as well - assuming that it 
is being used in places

(albeit in contravention of the older RFC). What do you think?

- Michael

On 17/07/2012 14:18, Chris Hegarty wrote:

On 17/07/2012 10:17, Michael McMahon wrote:

Hi,

Could I get the following change reviewed please?

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

Since 7u4, we are parsing all incoming cookies via the HttpCookie 
class.

This class has had a restriction on cookie names that is causing this
problem
and which is not required by any of the cookie specifications, as 
far as

I can see,
(rfc 2965, and 6265 which obsoletes 2965).


Right, this is my reading of the RFC's also. In fact, RFC 2965 
explicitly states that "the NAME of a cookie MAY be the same as one 
of the attributes in this specification".



The restriction was that cookie names could not be the same (case
insensitively)
as any of the attribute names (eg. Domain). So, the change is to 
remove

the restriction.


Yes, this makes sense to me.

One comment on the webrev is that isReserved also enforces that the 
name cannot start with a '$', from 2965: "NAMEs that begin with $ 
are reserved and MUST NOT be used by applications." I think you may 
need to minimally reintroduce this. Otherwise, the changes look 
good to me.


-Chris.



Thanks,
Michael








--
-Kurchi

Re:

[Kurchi Hazra]

Forwarding this to the networking mailing list.

Do you know of any way to reproduce the problem, like a small test case 
that we(I) could try?


Thanks,
Kurchi

On 8/29/2012 4:45 PM, Earle Nietzel wrote:

Seeing the following Warning multiple times every few seconds.

Aug 29, 2012 7:09:26 PM sun.rmi.transport.tcp.TCPTransport$AcceptLoop
executeAcceptLoop
WARNING: RMI TCP Accept-1099: accept loop for
ServerSocket[addr=0.0.0.0/0.0.0.0,port=0,localport=1099] throws
java.net.SocketException: Invalid argument
at java.net.PlainSocketImpl.socketAccept(Native Method)
at 
java.net.AbstractPlainSocketImpl.accept(AbstractPlainSocketImpl.java:398)
at java.net.ServerSocket.implAccept(ServerSocket.java:522)
at java.net.ServerSocket.accept(ServerSocket.java:490)
at 
sun.rmi.transport.tcp.TCPTransport$AcceptLoop.executeAcceptLoop(TCPTransport.java:387)
at 
sun.rmi.transport.tcp.TCPTransport$AcceptLoop.run(TCPTransport.java:359)
at java.lang.Thread.run(Thread.java:722)


OSX 10.8.1

java version "1.7.0_06"
Java(TM) SE Runtime Environment (build 1.7.0_06-b24)
Java HotSpot(TM) 64-Bit Server VM (build 23.2-b09, mixed mode)

The closest thread I found seems to be this one:
http://mail.openjdk.java.net/pipermail/bsd-port-dev/2008-December/000229.html

No issues when using JAVA 6 JDK's.

These warnings appear constantly when working with ActiveMQ 5.4.0, 5.6.0.

Forgive for me if this post seems irrelevant to this list :)

Earle


--
-Kurchi

Re: Change to: ./src/solaris/native/java/net/PlainDatagramSocketImpl.c

[Kurchi Hazra]

Hi John,

  This change looks good to me.

Thanks,
- Kurchi

On 18.10.2012 14:18, Dmitry Samersoff wrote:

John,

Looks good for me.

-Dmitry

On 2012-10-19 00:59, John Zavgren wrote:

Greetings:
I'm proposing the following change:

http://cr.openjdk.java.net/~khazra/john/8000206/webrev.00/

because it simplifies the code by eliminating an unnecessary union data 
structure. This change eliminates
a false positive from our static code analyzer: parfait. This modification 
doesn't change any of the externally-visible behavior in the procedure.

I look forward to your comments and suggestions.

Thanks!
John Zavgren
john.zavg...@oracle.com





--
-Kurchi

Re: RFR 8000203: file descriptor leak, src/solaris/native/java/net/net_util_md.c ... AND a potential realloc()-related memory leak.

[Kurchi Hazra]

Just for the sake of understanding the fix better, loRoutesTemp will 
point to 0 if the realloc() request fails,
and we still need a reference to the older allocated memory (loRoutes in 
this case) in order to free it. Hence

the need for a temporary variable here?

- Kurchi


On 24.10.2012 06:27, Chris Hegarty wrote:

Looks good to me. Thanks for going the extra mile here.

-Chris.

On 24/10/2012 14:16, John Zavgren wrote:


Greetings:

I'm requesting a review of a software change that fixes a file 
descriptor leak AND a potential memory leak that involves memory 
reallocation (realloc()). The webrev image is in the following location:


http://cr.openjdk.java.net/~chegar/8000203/webrev.01/

Thanks!
John Zavgren
john.zavg...@oracle.com


--
-Kurchi

Re: RFR 8000203: file descriptor leak, src/solaris/native/java/net/net_util_md.c ... AND a potential realloc()-related memory leak.

[Kurchi Hazra]

Sounds good, thanks a lot.

- Kurchi


On 24.10.2012 10:47, John Zavgren wrote:

That's right Kurchi. If realloc were to fail, in the original code, then the 
pointer: loRoutes would be set to the value zero, but the memory that this 
variable previously pointed to would still be allocated. So, basically, if 
realloc failed, then we'd leak memory.

John
- Original Message -
From: kurchi.subhra.ha...@oracle.com
To: john.zavg...@oracle.com
Cc: net-dev@openjdk.java.net
Sent: Wednesday, October 24, 2012 1:31:36 PM GMT -05:00 US/Canada Eastern
Subject: Re: RFR 8000203: file descriptor leak, 
src/solaris/native/java/net/net_util_md.c ... AND a potential realloc()-related 
memory leak.

Just for the sake of understanding the fix better, loRoutesTemp will
point to 0 if the realloc() request fails,
and we still need a reference to the older allocated memory (loRoutes in
this case) in order to free it. Hence
the need for a temporary variable here?

- Kurchi


On 24.10.2012 06:27, Chris Hegarty wrote:

Looks good to me. Thanks for going the extra mile here.

-Chris.

On 24/10/2012 14:16, John Zavgren wrote:

Greetings:

I'm requesting a review of a software change that fixes a file
descriptor leak AND a potential memory leak that involves memory
reallocation (realloc()). The webrev image is in the following location:

http://cr.openjdk.java.net/~chegar/8000203/webrev.01/

Thanks!
John Zavgren
john.zavg...@oracle.com


--
-Kurchi

Re: getportbyname in Java?

[Kurchi Hazra]

I don't think so...

Thanks,
- Kurchi

On 13.11.2012 16:40, Weijun Wang wrote:

Is there a Java API I can translate "http" to 80?

Thanks
Max


--
-Kurchi

Re: RFR8003991 (take two)

[Kurchi Hazra]

Aren't we effectively calling fclose(NULL) here now?

- Kurchi

On 26.11.2012 15:19, John Zavgren wrote:

Greetings:

I'm posting the webrev image of a change that I made that eliminates a 
potential file descriptor leak. (The previous RFR referenced a webrev image 
with the right file name but the wrong content. 'sorry about that.)

http://cr.openjdk.java.net/~mullan/webrevs/8003991/webrev.00/

Thanks!
John Zavgren


--
-Kurchi

Re: RFR 8004675: Inet6Address.getHostAddress should use string scope identifier where available

[Kurchi Hazra]

Looks good to me.

Not related to this bug, but do we need scope_id_set then? From what I 
infer, scope_id_set is being set in native code, only when
scope_id is not 0, and so a check with scope_id == 0 can serve the 
purpose of scope_id_set too.


Thanks,
Kurchi

On 10.12.2012 08:01, Chris Hegarty wrote:


Inet6Address.getHostAddress() is specified to return the IP address 
string in textual presentation, followed by a '%' character and the 
scope identifier. This scope identifier can be either a numeric value 
or a string, depending on how the instance was created (if it was 
created with a scoped interface).


This change proposes to remove the boolean field, 'scope_ifname_set', 
since it is not always correctly set when the instance contains a 
valid scoped interface. For example, when iterating over the 
NetworkInterface's on the system. 'scope_ifname_set' was never 
accessed from native code, so it can simply be removed.


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

-Chris.


--
-Kurchi

Fwd: InetAddress.getAllByName()/getByName() fails to resolve IPv6 via ssh

[Kurchi Hazra]

Forwarding to net-dev

Hi Charis,

I tried to recreate this,  I ran your code remotely (from both 
Linux and Windows onto a Linux machine) but could not
get an UnkownHostException if the code works without an exception when 
run locally on a machine.
Can you be more specific about the operating systems running on both 
systems?


Thanks,
Kurchi


 Original Message 
Subject: 	InetAddress.getAllByName()/getByName() fails to resolve IPv6 
via ssh

Date:   Mon, 17 Dec 2012 21:55:40 -0800 (PST)
From:   charis 
To: nio-...@openjdk.java.net



I am facing the following issue:
Say there is a simple standlone code Test.java which just calls
InetAddress.getAllByName() :

*
import java.net.Inet4Address;
import java.net.Inet6Address;
import java.net.InetAddress;
import java.net.UnknownHostException;


public class TestNameResolution {
public static void main(String args[])  {
String name = args[0];
System.out.println("Trying to resolve name \"" + name + "\" to IP
addresses");
try {
InetAddress[] addresses = InetAddress.getAllByName(name);

for (int i = 0; i<  addresses.length; i++) {
if (addresses[i] instanceof Inet4Address) {
 System.out.println("IPv4 address -->" +
addresses[i].getHostAddress());
}
else if (addresses[i] instanceof Inet6Address) {
 System.out.println("IPv6 address -->" +
addresses[i].getHostAddress());
}
}
}
catch (UnknownHostException uhe) {
System.out.println("Cannot resolve name \"" + name + "\"");
}
}
}
*

I run this via a simple single-line script 'run':
*
java -classpath
TestNameResolution
*

When try this locally it resolves the name to its IPv6 address as expected.
However, when from a different machine to execute this via ssh (i.e.,  ssh
  ) I often get the following output:

/
Trying to resolve name "" to IP addresses
Cannot resolve name ""
java.net.UnknownHostException:
at java.net.Inet4AddressImpl.lookupAllHostAddr(Native Method)
at java.net.InetAddress$1.lookupAllHostAddr(InetAddress.java:849)
at
java.net.InetAddress.getAddressFromNameService(InetAddress.java:1202)
at java.net.InetAddress.getAllByName0(InetAddress.java:1153)
at java.net.InetAddress.getAllByName(InetAddress.java:1083)
at java.net.InetAddress.getAllByName(InetAddress.java:1019)
at TestNameResolution.main(TestNameResolution.java:14
/

Note:
- I tried the java command that invokes the standalone also with i)
'-Djava.net.preferIPv6Stack=true' , ii) and
'-Djava.net.preferIPv6Addresses=true' and iii) with both of them at the same
time.
Still, the output is the same
- The same standalone succeeds in resolving a name which maps to IPv4 giving
the same (correct) output on the local and the remote node (where the ssh is
initiated)

Java environment details:
$ java -version
java version "1.6.0_37"
Java(TM) SE Runtime Environment (build 1.6.0_37-b06)


Is there any known issue with IPv6 name resolution via ssh? Any ideas what
may be causing this?



--
View this message in context: 
http://nio-dev.3157472.n2.nabble.com/InetAddress-getAllByName-getByName-fails-to-resolve-IPv6-via-ssh-tp7574957.html
Sent from the nio-dev mailing list archive at Nabble.com.

Code Review Request: 7171415: java.net.URI.equals/hashCode not consistent for some URIs

[Kurchi Hazra]

Hi,

According to RFC 3986[1], hexadecimal digits encoded by a '%' 
should be case-insensitive, for example,%A2 and %a2 should be
considered equal. Although, URI.equals() does take this into 
consideration, the implementation of URI.hashCode() does not and
returns different hashcodes for two URIs that are similar in all 
respects except for the case of the percent-encoded hexadecimal digits.
This fix attempts to construct a normalized string from the string 
representing a component before calculating its hashCode.
I converted to upper case for the normalization(and not lower case) as 
required by [1].


For testing the fix, I added an additional test scenario to an 
existing test (jdk/test/java/net/URI/Test.java). While I was there, I 
also made
minor changes to the test so that it does not produce rawtype and other 
lint warnings.


Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7171415
Webrev: http://cr.openjdk.java.net/~khazra/7171415/webrev.00/

URI.compareTo() still suffers from the same problem - I am not sure if 
it should be dealt with as a separate bug.


Thanks,
Kurchi

[1] http://tools.ietf.org/html/rfc3986#section-6.2.2.1

Re: Code Review Request: 7171415: java.net.URI.equals/hashCode not consistent for some URIs

[Kurchi Hazra]

Thank you all for your comments. Here is an updated webrev where 
URI.hashCode() calculates the hashCode itself

instead of relying on String.hashCode():

http://cr.openjdk.java.net/~khazra/7171415/webrev.01/

Thanks,
Kurchi

On 08.01.2013 20:29, Vitaly Davidovich wrote:


Yes, I also thought about range checks not being eliminated if using 
charAt() but I guess that depends on how smart the JIT is - if charAt 
is inlined there's technically enough info there for the compiler to 
see that range checks aren't needed.  Whether that happens or not I 
haven't checked.  toCharArray brings us back to having allocations 
unless, again, EA helps out.  I think a microbenchmark would help here 
(along with verbose GC logging) to see which is better if this is a 
concern.


Why do you say you need to duplicate String.hashCode to be consistent 
with what people are using already? As long as the hash quality is at 
least as good as today (or not significantly worse) shouldn't you be 
able to change the impl? If someone's relying on specific value for 
some input then their code is broken.  Besides, doing toUpper will 
change the hash for URIs with % anyway.  Perhaps I misunderstood your 
point though ...


Vitaly

Sent from my phone

On Jan 8, 2013 11:04 PM, "Kurchi Subhra Hazra" 
<mailto:kurchi.subhra.ha...@oracle.com>> wrote:


On 1/8/13 6:55 PM, Vitaly Davidovich wrote:


Also, I'm not sure how hot this method is in practice but
allocating StringBuilder seems a bit heavy (maybe it gets
partially escape analyzed out though).  Can you instead loop over
all the chars in the string and build up the hash code as you go
along? If you see a % then you handle next 2 chars specially,
like you do now.  Or are you trying to take advantage of String
intrinsic support in the JIT? I guess if perf is a concern you
can write a micro benchmark comparing the approaches ...


That did occur to me, but I guess we have to be consistent with
the value that people have already been using, and that means I have
to duplicate the code in String.hashCode() (that is what the
original implementation was calling) - I was trying to avoid that.
Also,
String.hashCode() uses its internal char[] - whereas charAt() will
involve several additional bound checks - but
using toCharArray() may be better. Let me take another look at
this, and get back with another webrev.


Sent from my phone

On Jan 8, 2013 9:45 PM, "Vitaly Davidovich" mailto:vita...@gmail.com>> wrote:

Hi Kurchi,

In the hash method, I suggest you move handling of strings
with % into a separate method to keep the hash method small
for common case (no %).  Otherwise, there's a chance this
method won't get inlined anymore due to its (newly increased)
size.


- Yep, will do.


Also, I realize toLower does the same thing, but why does
toUpper return an int whereas it's really a char? Might be
cleaner to declare return type as char and do the cast inside
the method as needed.


- I followed the format of toLower(). But I agree this way it will
be cleaner.

Thanks a lot,
Kurchi




    Thanks

Sent from my phone

On Jan 8, 2013 8:20 PM, "Kurchi Hazra"
mailto:kurchi.subhra.ha...@oracle.com>> wrote:

Hi,

According to RFC 3986[1], hexadecimal digits encoded
by a '%' should be case-insensitive, for example,%A2 and
%a2 should be
considered equal. Although, URI.equals() does take this
into consideration, the implementation of URI.hashCode()
does not and
returns different hashcodes for two URIs that are similar
in all respects except for the case of the
percent-encoded hexadecimal digits.
This fix attempts to construct a normalized string from
the string representing a component before calculating
its hashCode.
I converted to upper case for the normalization(and not
lower case) as required by [1].

For testing the fix, I added an additional test
scenario to an existing test
(jdk/test/java/net/URI/Test.java). While I was there, I
also made
minor changes to the test so that it does not produce
rawtype and other lint warnings.

Bug:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7171415
Webrev:
http://cr.openjdk.java.net/~khazra/7171415/webrev.00/
<http://cr.openjdk.java.net/%7Ekhazra/7171415/webrev.00/>

URI.compareTo() still suffers from the same problem - I
am not sure if it should be dealt with as a separate bug.

Thanks,
Kurchi

[1] http://tools.ietf.org/html/rfc3986#section-6.2.2.1





--
-Kurchi

Code Review Request: 7017962: Obsolete link is used in URL class level spec

[Kurchi Hazra]

The current javadoc for URL.java points to a non-existent link [1] to 
explain different types of URLs
used by different protocols. The same document can currently be found at 
[2].


Bug: http://bugs.sun.com/view_bug.do?bug_id=7017962
Webrev: http://cr.openjdk.java.net/~khazra/7017962/webrev.00/

Although I have used [2] as a substitute in the webrev above, here are 
some other alternatives that I could find:

1. http://www.w3.org/TR/WD-html40-970917/htmlweb.html#h-5.1
2. http://www.utoronto.ca/web/HTMLdocs/NewHTML/url.html
3. Simply remove reference to the document in the javadoc.

Any suggestions are welcome.

Thanks,
Kurchi

[1] http://www.socs.uts.edu.au/MosaicDocs-old/url-primer.html
[2] http://www.mathematik.uni-halle.de/doc/NCSA/Mosaic-Docs/url-primer.html

Re: Code Review Request: 7017962: Obsolete link is used in URL class level spec

[Kurchi Hazra]

Apologies, but I have a new suggestion. It is safer to redirect to an 
archived

version of the document that we want to link to.

http://cr.openjdk.java.net/~khazra/7017962/webrev.01/src/share/classes/java/net/URL.java.sdiff.html

Thanks to Mike for pointing this out.

- Kurchi

On 23.01.2013 05:52, Chris Hegarty wrote:

Thanks Kurchi, I'm ok with the changes you have.

-Chris.

On 23/01/2013 00:20, Kurchi Hazra wrote:

The current javadoc for URL.java points to a non-existent link [1] to
explain different types of URLs
used by different protocols. The same document can currently be found at
[2].

Bug: http://bugs.sun.com/view_bug.do?bug_id=7017962
Webrev: http://cr.openjdk.java.net/~khazra/7017962/webrev.00/

Although I have used [2] as a substitute in the webrev above, here are
some other alternatives that I could find:
1. http://www.w3.org/TR/WD-html40-970917/htmlweb.html#h-5.1
2. http://www.utoronto.ca/web/HTMLdocs/NewHTML/url.html
3. Simply remove reference to the document in the javadoc.

Any suggestions are welcome.

Thanks,
Kurchi

[1] http://www.socs.uts.edu.au/MosaicDocs-old/url-primer.html
[2] 
http://www.mathematik.uni-halle.de/doc/NCSA/Mosaic-Docs/url-primer.html


--
-Kurchi

Re: Code Review Request: 7017962: Obsolete link is used in URL class level spec

[Kurchi Hazra]

Thanks Mike, makes sense - I'll change the port number on line 68 to 
1080 before pushing.


- Kurchi

On 24.01.2013 09:38, Mike Duigou wrote:

Looks good to me. The alternative port example at line 68 uses port 80 which is 
the default. 1080 or 8080 perhaps?

Mike

On Jan 24 2013, at 09:27 , Kurchi Hazra wrote:


Apologies, but I have a new suggestion. It is safer to redirect to an archived
version of the document that we want to link to.

http://cr.openjdk.java.net/~khazra/7017962/webrev.01/src/share/classes/java/net/URL.java.sdiff.html

Thanks to Mike for pointing this out.

- Kurchi

On 23.01.2013 05:52, Chris Hegarty wrote:

Thanks Kurchi, I'm ok with the changes you have.

-Chris.

On 23/01/2013 00:20, Kurchi Hazra wrote:

The current javadoc for URL.java points to a non-existent link [1] to
explain different types of URLs
used by different protocols. The same document can currently be found at
[2].

Bug: http://bugs.sun.com/view_bug.do?bug_id=7017962
Webrev: http://cr.openjdk.java.net/~khazra/7017962/webrev.00/

Although I have used [2] as a substitute in the webrev above, here are
some other alternatives that I could find:
1. http://www.w3.org/TR/WD-html40-970917/htmlweb.html#h-5.1
2. http://www.utoronto.ca/web/HTMLdocs/NewHTML/url.html
3. Simply remove reference to the document in the javadoc.

Any suggestions are welcome.

Thanks,
Kurchi

[1] http://www.socs.uts.edu.au/MosaicDocs-old/url-primer.html
[2] http://www.mathematik.uni-halle.de/doc/NCSA/Mosaic-Docs/url-primer.html

--
-Kurchi



--
-Kurchi

Re: RFR 8008223: java/net/BindException/Test.java fails rarely

[Kurchi Hazra]

I tried out your fix and your solution looks good to me.

Thanks,
Kurchi

On 2/14/2013 6:43 AM, Chris Hegarty wrote:

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

This test has been around a long time, and rarely fails, but has had 
several bugs closed as 'not reproducible' marked against it.


The test tries to verify that a BindException is thrown, or not 
thrown, in the appropriate circumstances. It does this by 
creating/binding a first socket to a specific socket address, then 
attempting to bind a second socket to possibly the same address. If 
the second socket attempts to bind to the same address as the first 
socket, then a BindException is expected to be thrown.


The problem appears to be an issue with the test itself. A local 
variable holds a reference to the first socket. This local variable is 
never accessed again within its scope (or elsewhere), therefore 
becomes instantly unreachable and eligible for collection by the GC. 
The second socket, if binding with the same address, is expected to 
throw BindException, but the first socket is no longer guaranteed to 
be live and therefore the address is not guaranteed to be unbindable.


I managed to reproduce the failure above, relatively consistently, by 
inserting 'System.runFinalization(); System.gc();' between the 
creation/binding of the first socket and the binding of the second 
one. This gives weight to the above analysis.


The solution is to simply add some trivial statement that references 
the first socket, after the second one is bound. This will ensure that 
the first socket remains reachable until after the second socket 
attempts to bind.


-Chris.


--
-Kurchi

Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed

[Kurchi Hazra]

Thanks Rob.

Also, I am now looking at the previous changeset[1] where the regression 
was introduced, once available() figures out
that a cached connection is no longer active, it should probably be 
removed from the cache. (setting ret to null doesn't remove it

from the cache).

If there were other reasons for not removing it from the cache that I am 
missing, that is fine - otherwise maybe we can file a separate bug for this.


Thanks,
Kurchi

[1] 
http://hg.openjdk.java.net/jdk7u/jdk7u/jdk/diff/e6dc1d9bc70b/src/share/classes/sun/net/www/http/HttpClient.java


On 3/7/2013 1:19 PM, Rob McKenna wrote:
I've fleshed out the bug report a little to make that clearer, sorry 
Kurchi!


Also, I'll add a testcase to this review soon.

-Rob

On 07/03/13 16:51, Kurchi Subhra Hazra wrote:

I am wondering why do you need two try-catch blocks here.

- Kurchi

On 3/7/13 8:18 AM, Rob McKenna wrote:

Hi folks,

This is a slight alteration of the fix contributed by Stuart 
Douglas. This fix deals with a SocketException caused by 
getSoTimeout() on a closed connection.


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

-Rob






--
-Kurchi

Re: Request for review: 8009650 - HttpClient available() check throws SocketException when connection has been closed

[Kurchi Hazra]

I looked at the source code changes, and it looks good.

Thanks,
- Kurchi


On 3/13/2013 7:42 AM, Chris Hegarty wrote:

The source code changes look fine to me.

I'm not sure why you enabled a security manager in the test. I don't 
think that it needs one. You can remove the explicit setting of the SM 
from the test code, remove the policy file, and the also the jtreg 
policy tag. Otherwise looks fine.


-Chris.

On 13/03/2013 12:53, Dmitry Samersoff wrote:

Rob,

Looks good for me.

-Dmitry

On 2013-03-13 04:35, Rob McKenna wrote:

Hi folks,

New webrev at:

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

Apologies for the delay.

 -Rob

On 07/03/13 23:19, Rob McKenna wrote:

Ah, I see what you mean. Can do.

 -Rob

On 07/03/13 23:13, Dmitry Samersoff wrote:

Rob,

Sorry for not being clean enough. We have repeated pattern:

   if (logger.isLoggable(PlatformLogger.FINEST)) {
   logger.finest("HttpClient.available(): " + msg
   }

so it makes code better readable if we can put it to some common 
place.


-Dmitry

On 2013-03-08 02:31, Rob McKenna wrote:

Hi Dmitry,

I'm not 100% sure what you mean by duplication, the exceptions and
their
messages are distinct. I think it would be best to keep it that way.

  -Rob

On 07/03/13 22:00, Dmitry Samersoff wrote:

Rob,

Is it possible to avoid code duplication?

i.e. do something like this:

  int r;

  try {
 ...
  } catch (SocketException e) {
   // Comments goes here
   r = -1
  }

  if (r == -1){
 if (logger. ...
 available = false;
  }

 return available;

-Dmitry


On 2013-03-07 20:18, Rob McKenna wrote:

Hi folks,

This is a slight alteration of the fix contributed by Stuart 
Douglas.
This fix deals with a SocketException caused by getSoTimeout() 
on a

closed connection.

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

   -Rob











--
-Kurchi

Re: RFR 8010282: sun.net.www.protocol.jar.JarFileFactory.close(JarFile) should be thread-safe

[Kurchi Hazra]

The if statements in the platform specific close methods do not have the 
spaces as we usually prefer to have them (I realize from before your 
changes), but otherwise this looks good to me.


- Kurchi

On 3/19/2013 10:43 AM, Chris Hegarty wrote:
JarFileFactory has two Maps that it uses to implement caching of jar 
files. Access to these maps should always be done while holding the 
instance lock, as multiple threads can be simultaneously updating the 
maps.


The close() method was added some time after the original 
implementation, and it looks like the locking was forgotten. Also, the 
locking strategy assumes that JarFileFactory is a singleton. It should 
be reworked to make it more robust. There is much more cleanup that 
can be done here, and I intend to do it as a separate bug, so as not 
to confuse the specifics of this issue.


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

-Chris.


--
-Kurchi

Re: DefaultProxySelector socks override

[Kurchi Hazra]

Hi Christos/Chris,

   Here is a webrev for this change: 
http://cr.openjdk.java.net/~khazra/5001942/webrev.00/


Thanks,
- Kurchi



On 3/27/2013 10:49 AM, chris...@zoulas.com wrote:

On Mar 27,  5:30pm, chris.hega...@oracle.com (Chris Hegarty) wrote:
-- Subject: Re: DefaultProxySelector socks override

| On 03/27/2013 05:22 PM, chris...@zoulas.com wrote:
| > 
| > Sure, I just requested a subscription to net-dev so I might not see the
| > first few messages. To clarify:
| >
| >  1. I will add socks.proxyHost and socks.proxyPort for consistency
| > with the other protocols, leaving as is socksProxyHost and
| > socksProxyPort for compatibility.
| >  2. I will add socks.nonProxyHosts and not socksNonProxyHosts.
| >
| > Is that what you had in mind?
|
| Re-checking the code I take back my previous comment. We already have
|
| socksProxyHost, socksProxyPort, socksProxyVersion
|
| so your original proposal of 'socksNonProxyHosts' is probably best, and
| consistent with existing properties.

I concur. Nothing for me to do :-)

Best,

christos


--
-Kurchi

Re: DefaultProxySelector socks override

[Kurchi Hazra]

Also, I should have clarified why I am changing the test. Since now we 
are defining the socks non-proxy property -
localhost gets added to the list of non-proxy hosts by default in our 
implementation (as is the case with other protocols

too). So "localhost" no more acts as the socksProxyHost.

On 4/2/2013 5:13 PM, Kurchi Hazra wrote:

Hi Christos/Chris,

   Here is a webrev for this change: 
http://cr.openjdk.java.net/~khazra/5001942/webrev.00/


Thanks,
- Kurchi



On 3/27/2013 10:49 AM, chris...@zoulas.com wrote:

On Mar 27,  5:30pm, chris.hega...@oracle.com (Chris Hegarty) wrote:
-- Subject: Re: DefaultProxySelector socks override

| On 03/27/2013 05:22 PM, chris...@zoulas.com wrote:
| > 
| > Sure, I just requested a subscription to net-dev so I might not 
see the

| > first few messages. To clarify:
| >
| > 1. I will add socks.proxyHost and socks.proxyPort for 
consistency

| >with the other protocols, leaving as is socksProxyHost and
| >socksProxyPort for compatibility.
| > 2. I will add socks.nonProxyHosts and not socksNonProxyHosts.
| >
| > Is that what you had in mind?
|
| Re-checking the code I take back my previous comment. We already have
|
| socksProxyHost, socksProxyPort, socksProxyVersion
|
| so your original proposal of 'socksNonProxyHosts' is probably best, 
and

| consistent with existing properties.

I concur. Nothing for me to do :-)

Best,

christos




--
-Kurchi

Re: RFR-8012108

[Kurchi Hazra]

Hi John,

  Does this actually fix the leak? When curr is null, you set start to 
curr, and hence start to null. So nothing is every freed

in freeAllocatedMemory is what I see. Let me know if I am missing something.

- Kurchi

On 4/17/2013 8:02 AM, John Zavgren wrote:

Greetings:
Please review the following webrev image. I fixed a memory leak that 
can occur when calloc() fails.


http://cr.openjdk.java.net/~jzavgren/8012108/webrev.01/

Thanks!
John Zavgren
john.zavg...@oracle.com


--
-Kurchi

Re: RFR-JDK8012108

[Kurchi Hazra]

Thanks John, I think the leak is taken care of now, I can sponsor the 
change for you once you have

a thumbs up from everyone.

- Kurchi

On 4/18/2013 1:56 PM, John Zavgren wrote:

Greetings:

I fixed a case in the windows native code where calloc() was being 
used without checking it's returned value.


http://cr.openjdk.java.net/~jzavgren/8012108/webrev.01/ 



Thanks!
John Zavgren


--
-Kurchi

Re: API change for 8010464: Evolve java networking same origin policy

[Kurchi Hazra]

Hi Michael,

 From the documentation, it is not clear to me how to represent 
both request-headers and method list together in an actions string for 
two or more methods. (Say I have two methods GET and POST and I want
to specify a request-headers list for each, how do I do it?) Maybe 
another example will help.


Thanks,
Kurchi

On 4/29/2013 3:53 AM, Michael McMahon wrote:

On 28/04/13 09:01, Chris Hegarty wrote:

In the main I link the new HttpURLPermission class.

When reading the docs I found the references to "the URL" and "URL 
string" confusing ( it could be just me ). When I see capital 'URL' 
my mind instantly, and incorrectly, goes to java.net.URL. In all 
cases you mean the URL string given when constructing the 
HttpURLPermission, right?




Yes, that is what is meant. The class does not use j.n.URL at all, as 
that would bring us back
to the old (undesirable) behavior with DNS lookups required for basic 
operations like equals() and hashCode()



Another example is the equals method
  "Returns true if, this.getActions().equals(p.getActions()) and p's
   URL equals this's URL. Returns false otherwise."

this is referring so a simple string comparison of the given URL 
string, right? This should be case insensitive too. Does it take into 
account default protocol ports, e.g. http://foo.com/ equal 
http://foo.com:80/




The implementation uses a java.net.URI internally. So URI takes care 
of that.


implies() makes reference to the URL scheme, and other specific parts 
of the URL. Also, the constructors throw IAE  'if url is not a valid 
URL', but what does this mean. Should we just bite the bullet and 
just say that URI is used to parse the given string into its specific 
parts? Otherwise, how can this be validated.




I originally didn't want to mention URI in the apidoc due to potential 
confusion surrounding the use of URL in the permission
class name. But, maybe it would be clearer to be explicit about it, 
particularly for the equals() behavior.

Otherwise we have to specify all of it in this class.

As for the additions to HttpURLConnection, what are the implications 
on proxies? Permissions, etc...




There's no change in behavior with respect to proxies. Permission is 
given to connect to proxies implicitly
except in cases where the caller specifies the proxy through the 
URL.openConnection(Proxy) api.
There are other unusual cases like the Http "Use Proxy" response. 
Explicit permission is required

for that case also.

Thanks!

Michael


-Chris.

On 04/26/2013 03:36 PM, Michael McMahon wrote:

Hi,

The is the suggested API for one of the two new JEPs recently 
submitted.


This is for JEP 184: HTTP URL Permissions

The idea here is to define a higher level http permission class
which "knows about" URLs, HTTP request methods and headers.
So, it is no longer necessary to grant blanket permission for any kind
of TCP connection to a host/port. Instead a HttpURLPermission restricts
access to only the Http protocol itself. Restrictions can also be 
imposed

based on URL paths, specific request methods and request headers.

The API change can be seen at the URL below:

http://cr.openjdk.java.net/~michaelm/8010464/api/

In addition to defining a new permission class, HttpURLConnection
is modified to make use of it and the documentation change 
describing this

can be seen at the link below:

http://cr.openjdk.java.net/~michaelm/8010464/api/blender.html

All comments welcome.

Thanks

Michael.




--
-Kurchi

Re: Code Review Request: 8013140: Heap corruption with NetworkInterface.getByInetAddress() and long i/f name

[Kurchi Hazra]

Thank you, committed now.

- Kurchi

On 5/2/2013 3:50 AM, Alan Bateman wrote:

On 02/05/2013 01:03, Kurchi Subhra Hazra wrote:


Hi,

   NetworkInterface.getByInetAddress() was crashing on solaris when 
the system had a network
interface name longer than 15 characters, due to two instances in the 
native
code for NetworkInterface where we were copying a char array of size 
32 (LIFNAMSIZ)
into a char array of size 16 (IFNAMSIZ), resulting in a buffer 
overflow with long names.
The fix is to make sure that the space allocated for the interface 
name is consistent (16/32
bytes depending on the system), and to prevent overflows by using 
strncpy instead of strcpy.


Bug: http://bugs.sun.com/view_bug.do?bug_id=8013140
Webrev: http://cr.openjdk.java.net/~khazra/8013140/webrev.00/


Thanks,
- Kurchi
A good fine, looks okay to me. An alternative would be sizeof(name) 
but what you have is fine.


-Alan



--
-Kurchi

Re: RFR JDK7188517

[Kurchi Hazra]

This looks okay to me.

- Kurchi


On 5/2/2013 10:09 AM, John Zavgren wrote:

All:
My original email was mangled by my email program (stbeehive/zimbra) 
... so I'm sending a second correctly formatted copy.


I'm sorry for the inconvenience.

John
---

Please consider the following change to the cookie constructor: 
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.01/ 



Basically there are two issues:

1.) the existing cookie constructor was allowing cookie names to have 
a dollar sign as their leading character,
which is "illegal". The constructor code was modified to disallow 
these illegal names.


2.) the API document (notice the specdiff:
http://cr.openjdk.java.net/~jzavgren/7188517/specDiff/ 
) prohibited 
the use of cookie names that are one of the tokens reserved for use by 
the cookie protocol, and this restriction is not necessary.


Thanks!
John Zavgren


- Original Message -
From: john.zavg...@oracle.com
To: net-dev@openjdk.java.net
Sent: Thursday, May 2, 2013 10:36:38 AM GMT -05:00 US/Canada Eastern
Subject: RFR JDK7188517

Greetings: Please consider the following change to the cookie 
constructor: http://cr.openjdk.java.net/~jzavgren/7188517/webrev.01/ 
Basically there are two issues: 1.) the existing cookie constructor 
was allowing cookie names to have a dollar sign as their leading 
character, which is "illegal". The constructor code was modified to 
disallow these illegal names. 2.) the API document (notice the 
specdiff: http://cr.openjdk.java.net/~jzavgren/7188517/specDiff/) 
prohibited the use of cookie names that are one of the tokens reserved 
for use by the cookie protocol, and this restriction is not necessary. 
Thanks! John Zavgren


--
-Kurchi

Re: RFR JDK7188517

[Kurchi Hazra]

I would have thrown an exception if the IllegalArgumentException is not 
obtained, otherwise the test looses its
purpose, and will pass silently if someone mistakenly removes the $ 
check in our code.


- Kurchi

On 5/8/2013 12:10 PM, John Zavgren wrote:

Greetings:

I added a test for the leading dollar sign character in cookie names:
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.03/

Thanks!
John


On 05/08/2013 08:33 AM, Michael McMahon wrote:

On 08/05/13 09:50, Chris Hegarty wrote:

On 08/05/2013 10:35, Michael McMahon wrote:

On 07/05/13 14:43, Chris Hegarty wrote:

On 05/06/2013 10:28 PM, Kurchi Hazra wrote:

This looks okay to me.


Source changes look fine to me too. Probably best to add a test 
for '$'


In fact, Michael actually removed such a test [1] during another
change. We should get positive agreement from Michael before pushing
this.



Yes, that was a positive test for for a cookie whose name began 
with '$'.

I agree we should add a negative test now for a similar cookie.

Source changes look fine to me too.


Thanks Michael,

In which case, I believe the check that a cookie the name 
'$Customer' throws IAE can be re-instated in TestHttpCookie.java




Right. I didn't realise the test was able to handle the IAE. I see 
now that it does and it should

be possible to put the same test back.

Michael

-Chris.



Michael


-Chris.

[1] http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/7bd32bfc0539



- Kurchi


On 5/2/2013 10:09 AM, John Zavgren wrote:

All:
My original email was mangled by my email program 
(stbeehive/zimbra)

... so I'm sending a second correctly formatted copy.

I'm sorry for the inconvenience.

John
---

Please consider the following change to the cookie constructor:
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.01/
<http://cr.openjdk.java.net/%7Ejzavgren/7188517/webrev.01/>

Basically there are two issues:

1.) the existing cookie constructor was allowing cookie names to 
have

a dollar sign as their leading character,
which is "illegal". The constructor code was modified to disallow
these illegal names.

2.) the API document (notice the specdiff:
http://cr.openjdk.java.net/~jzavgren/7188517/specDiff/
<http://cr.openjdk.java.net/%7Ejzavgren/7188517/specDiff/>) 
prohibited
the use of cookie names that are one of the tokens reserved for 
use by

the cookie protocol, and this restriction is not necessary.

Thanks!
John Zavgren


- Original Message -
From: john.zavg...@oracle.com
To: net-dev@openjdk.java.net
Sent: Thursday, May 2, 2013 10:36:38 AM GMT -05:00 US/Canada 
Eastern

Subject: RFR JDK7188517

Greetings: Please consider the following change to the cookie
constructor: 
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.01/

Basically there are two issues: 1.) the existing cookie constructor
was allowing cookie names to have a dollar sign as their leading
character, which is "illegal". The constructor code was modified to
disallow these illegal names. 2.) the API document (notice the
specdiff: http://cr.openjdk.java.net/~jzavgren/7188517/specDiff/)
prohibited the use of cookie names that are one of the tokens 
reserved
for use by the cookie protocol, and this restriction is not 
necessary.

Thanks! John Zavgren


--
-Kurchi










--
-Kurchi

Code Review Request: 8014254: Selector in HttpServer introduces a 1000 ms delay when using KeepAlive

[Kurchi Hazra]

Hi,

   com.sun.net.httpServer uses a selector to get notified about interesting
events (such as arrival of a new connection, or data available to read
on an existing connection when using keep-alive), but imposes a timeout of
1000 ms on the select() operation. Although this is not a problem when the 
server uses a
ThreadPool with more than one thread, but for a single threaded server, this 
timeout
gives rise to a bottleneck and should be reduced to a lower value.

I have proposed 200 ms for the timeout here, but if anyone has preference for a 
greater
or lower value, I am open to that too.

Bug:http://bugs.sun.com/view_bug.do?bug_id=8014254  (To appear)
Webrev:http://cr.openjdk.java.net/~khazra/8014254/webrev.00/

 Thanks,
- Kurchi

Re: Code Review Request: 8014254: Selector in HttpServer introduces a 1000 ms delay when using KeepAlive

[Kurchi Hazra]

On 5/8/2013 4:35 PM, Matthew Hall wrote:

On Wed, May 08, 2013 at 04:06:10PM -0700, Kurchi Hazra wrote:

com.sun.net.httpServer uses a selector to get notified about interesting
events (such as arrival of a new connection, or data available to read
on an existing connection when using keep-alive), but imposes a timeout of
1000 ms on the select() operation.

Maybe I'm missing something since the bug is not viewable to the community,
but, if I'm reading properly, this design by itself is not quite right, and
the fix of 200 msec selector timeout is just a band-aid solution.

Shouldn't it be registering interestOps for reading on the existing
connections and accepting connections from the server socket in the selector?
Otherwise why use any selector in the first place if it's not really selecting
across all the right sockets and right events on them?


- If you look at the implementation, this is exactly what is done. 
However, for keep-alive, the implementation delays the re-registering of 
a used channel
for reading until after one select call, which results in the 
bottleneck. The other option is to re-register existing channels before 
the select call, this is what
was done in jdk6. But I would need to understand why we walked away from 
that. Michael or Chris can shed some light on this.




Thanks,
- Kurchi

Re: RFR JDK7188517

[Kurchi Hazra]

I am guessing you meant to send 
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.04/

which looks ok to me.

- Kurchi

On 5/9/2013 8:09 AM, Chris Hegarty wrote:

Looks ok to me.

-Chris.

On 05/09/2013 03:42 PM, John Zavgren wrote:

Chris, et alia:
I put the reinstated the old test.
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.03/

John

- Original Message -
From: chris.hega...@oracle.com
To: john.zavg...@oracle.com
Cc: kurchi.subhra.ha...@oracle.com, net-dev@openjdk.java.net
Sent: Thursday, May 9, 2013 4:39:29 AM GMT -05:00 US/Canada Eastern
Subject: Re: RFR JDK7188517

John,

I think you can simply reinstate

http://hg.openjdk.java.net/jdk8/jdk8/jdk/diff/7bd32bfc0539/test/java/net/CookieHandler/TestHttpCookie.java 



-Chris.

On 05/08/2013 09:02 PM, Kurchi Hazra wrote:

I would have thrown an exception if the IllegalArgumentException is not
obtained, otherwise the test looses its
purpose, and will pass silently if someone mistakenly removes the $
check in our code.

- Kurchi

On 5/8/2013 12:10 PM, John Zavgren wrote:

Greetings:

I added a test for the leading dollar sign character in cookie names:
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.03/

Thanks!
John


On 05/08/2013 08:33 AM, Michael McMahon wrote:

On 08/05/13 09:50, Chris Hegarty wrote:

On 08/05/2013 10:35, Michael McMahon wrote:

On 07/05/13 14:43, Chris Hegarty wrote:

On 05/06/2013 10:28 PM, Kurchi Hazra wrote:

This looks okay to me.


Source changes look fine to me too. Probably best to add a test
for '$'

In fact, Michael actually removed such a test [1] during another
change. We should get positive agreement from Michael before 
pushing

this.



Yes, that was a positive test for for a cookie whose name began
with '$'.
I agree we should add a negative test now for a similar cookie.

Source changes look fine to me too.


Thanks Michael,

In which case, I believe the check that a cookie the name
'$Customer' throws IAE can be re-instated in TestHttpCookie.java



Right. I didn't realise the test was able to handle the IAE. I see
now that it does and it should
be possible to put the same test back.

Michael

-Chris.



Michael


-Chris.

[1] http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/7bd32bfc0539



- Kurchi


On 5/2/2013 10:09 AM, John Zavgren wrote:

All:
My original email was mangled by my email program
(stbeehive/zimbra)
... so I'm sending a second correctly formatted copy.

I'm sorry for the inconvenience.

John
---

Please consider the following change to the cookie constructor:
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.01/
<http://cr.openjdk.java.net/%7Ejzavgren/7188517/webrev.01/>

Basically there are two issues:

1.) the existing cookie constructor was allowing cookie names to
have
a dollar sign as their leading character,
which is "illegal". The constructor code was modified to 
disallow

these illegal names.

2.) the API document (notice the specdiff:
http://cr.openjdk.java.net/~jzavgren/7188517/specDiff/
<http://cr.openjdk.java.net/%7Ejzavgren/7188517/specDiff/>)
prohibited
the use of cookie names that are one of the tokens reserved for
use by
the cookie protocol, and this restriction is not necessary.

Thanks!
John Zavgren


- Original Message -
From: john.zavg...@oracle.com
To: net-dev@openjdk.java.net
Sent: Thursday, May 2, 2013 10:36:38 AM GMT -05:00 US/Canada
Eastern
Subject: RFR JDK7188517

Greetings: Please consider the following change to the cookie
constructor:
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.01/
Basically there are two issues: 1.) the existing cookie 
constructor

was allowing cookie names to have a dollar sign as their leading
character, which is "illegal". The constructor code was 
modified to

disallow these illegal names. 2.) the API document (notice the
specdiff: 
http://cr.openjdk.java.net/~jzavgren/7188517/specDiff/)

prohibited the use of cookie names that are one of the tokens
reserved
for use by the cookie protocol, and this restriction is not
necessary.
Thanks! John Zavgren


--
-Kurchi












--
-Kurchi

Re: Code Review Request: 8014254: Selector in HttpServer introduces a 1000 ms delay when using KeepAlive

[Kurchi Hazra]

I have re-arranged the code a little, so that the events are handled 
first, any freed up
connections are then registered with the selector, post which select() 
is called. This
removes the bottleneck in a single threaded server receiving requests 
from the same client

with a high frequency.

http://cr.openjdk.java.net/~khazra/8014254/webrev.01/

- Kurchi

On 5/9/13 1:45 AM, Chris Hegarty wrote:

On 05/09/2013 01:56 AM, Kurchi Hazra wrote:


On 5/8/2013 4:35 PM, Matthew Hall wrote:

On Wed, May 08, 2013 at 04:06:10PM -0700, Kurchi Hazra wrote:
com.sun.net.httpServer uses a selector to get notified about 
interesting

events (such as arrival of a new connection, or data available to read
on an existing connection when using keep-alive), but imposes a
timeout of
1000 ms on the select() operation.

Maybe I'm missing something since the bug is not viewable to the
community,
but, if I'm reading properly, this design by itself is not quite
right, and
the fix of 200 msec selector timeout is just a band-aid solution.

Shouldn't it be registering interestOps for reading on the existing
connections and accepting connections from the server socket in the
selector?
Otherwise why use any selector in the first place if it's not really
selecting
across all the right sockets and right events on them?


- If you look at the implementation, this is exactly what is done.
However, for keep-alive, the implementation delays the re-registering of
a used channel
for reading until after one select call, which results in the
bottleneck. The other option is to re-register existing channels before
the select call, this is what
was done in jdk6. But I would need to understand why we walked away from
that. Michael or Chris can shed some light on this.


I'm not sure why this changed from jdk6, but I don't see any reason it 
cannot be reverted back.


-Chris.





Thanks,
- Kurchi

Code Review Request: 6328537: Improve javadocs for Socket class by adding references to SocketOptions

[Kurchi Hazra]

Hi,
 
   This is a simple improvement in the javadocs of the Socket and ServerSocket

classes, to cross-reference the SocketOptions class where appropriate. It looks
like a lot of changes in the webrev, only because I have re-justified the text
in some cases.

Bug: http://bugs.sun.com/view_bug.do?bug_id=6328537
Webrev:http://cr.openjdk.java.net/~khazra/6328537/webrev.00/

The specdiff isn't very helpful, but here it is: 
http://cr.openjdk.java.net/~khazra/6328537/specdiff.00/

 Thanks,
- Kurchi

Re: Code Review Request: 6328537: Improve javadocs for Socket class by adding references to SocketOptions

[Kurchi Hazra]

On 5/13/2013 3:19 AM, Chris Hegarty wrote:

On 11/05/2013 00:35, Kurchi Hazra wrote:
This is a simple improvement in the javadocs of the Socket and 
ServerSocket

classes, to cross-reference the SocketOptions class where appropriate.
It looks
like a lot of changes in the webrev, only because I have re-justified
the text
in some cases.

Bug: http://bugs.sun.com/view_bug.do?bug_id=6328537
Webrev:http://cr.openjdk.java.net/~khazra/6328537/webrev.00/


This will make navigating from these API's to the specification for 
their respective options much easier. A simple change, but very user 
friendly.



The specdiff isn't very helpful, but here it is:
http://cr.openjdk.java.net/~khazra/6328537/specdiff.00/


Not at all. It gives confidence that nothing untoward has accidentally 
changed. Especially, as you say, when there is significant realignment 
of the method descriptions.

- Right, I did not think of it in that way.



As Alan pointed out, at some point we need to sort out the 
relationship to StandardSocketOptions, but what you have is fine for now.


- Thanks for reviewing, I'll push it post a CCC approval.

- Kurchi

Code Review Request: 7150552: network test hangs [macosx]

[Kurchi Hazra]

Hi,

Two regression tests in jdk/test were hanging on Mac OS X 
intermittently. Although it is difficult to reproduce
the problem, I suspect it is the use of the buggy httpserver test 
library (jdk/test/sun/net/www/httptest) - we anyway want
to move away from using it. I have re-written parts of these tests to 
use com.sun.net.httpserver instead.


I have also unleashed the tests from the ProblemList and will keep a 
closer watch on test runs.


Bug: http://bugs.sun.com/view_bug.do?bug_id=7150552
Webrev: http://cr.openjdk.java.net/~khazra/7150552/webrev.00/


Thanks!
Kurchi

Re: RFR JDK7188517

[Kurchi Hazra]

Looks good to me too!

- Kurchi

On 5/31/2013 7:42 AM, Chris Hegarty wrote:

Looks fine to me John.

-Chris.

On 05/31/2013 01:11 PM, John Zavgren wrote:

All:

I'd like to give everyone another chance to weigh in on this change:
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.04/
so that I can wrap up this fix ASAP.

(It makes HTTP cookies that begin with a dollar sign "illegal".)

Thanks!
John

On 05/15/2013 07:54 AM, John Zavgren wrote:

Greetings:

Can I get a show of hands on this RFR
(http://cr.openjdk.java.net/~jzavgren/7188517/webrev.04/)?
It's CCC request has been accepted and I'd like to wrap it up ASAP.

Thanks!
John Zavgren

On 05/09/2013 02:22 PM, John Zavgren wrote:

Greetings:
I made a mistake in my last RFR posting... the URL for the latest
modifications is:
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.04/
instead of:
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.03/

The most recent change is to reinstate the original test that tested
for cookie names that lead with a dollar sign.

I'm sorry about the confusion.

Thanks!
John


On 05/09/2013 03:42 PM, John Zavgren wrote:

Chris, et alia:
I put the reinstated the old test.
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.03/

John

- Original Message -
From: chris.hega...@oracle.com
To: john.zavg...@oracle.com
Cc: kurchi.subhra.ha...@oracle.com, net-dev@openjdk.java.net
Sent: Thursday, May 9, 2013 4:39:29 AM GMT -05:00 US/Canada Eastern
Subject: Re: RFR JDK7188517

John,

I think you can simply reinstate

http://hg.openjdk.java.net/jdk8/jdk8/jdk/diff/7bd32bfc0539/test/java/net/CookieHandler/TestHttpCookie.java 




-Chris.

On 05/08/2013 09:02 PM, Kurchi Hazra wrote:

I would have thrown an exception if the IllegalArgumentException is
not
obtained, otherwise the test looses its
purpose, and will pass silently if someone mistakenly removes the $
check in our code.

- Kurchi

On 5/8/2013 12:10 PM, John Zavgren wrote:

Greetings:

I added a test for the leading dollar sign character in cookie 
names:

http://cr.openjdk.java.net/~jzavgren/7188517/webrev.03/

Thanks!
John


On 05/08/2013 08:33 AM, Michael McMahon wrote:

On 08/05/13 09:50, Chris Hegarty wrote:

On 08/05/2013 10:35, Michael McMahon wrote:

On 07/05/13 14:43, Chris Hegarty wrote:

On 05/06/2013 10:28 PM, Kurchi Hazra wrote:

This looks okay to me.

Source changes look fine to me too. Probably best to add a test
for '$'

In fact, Michael actually removed such a test [1] during 
another

change. We should get positive agreement from Michael before
pushing
this.


Yes, that was a positive test for for a cookie whose name began
with '$'.
I agree we should add a negative test now for a similar cookie.

Source changes look fine to me too.

Thanks Michael,

In which case, I believe the check that a cookie the name
'$Customer' throws IAE can be re-instated in TestHttpCookie.java


Right. I didn't realise the test was able to handle the IAE. I see
now that it does and it should
be possible to put the same test back.

Michael

-Chris.


Michael


-Chris.

[1] http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/7bd32bfc0539


- Kurchi


On 5/2/2013 10:09 AM, John Zavgren wrote:

All:
My original email was mangled by my email program
(stbeehive/zimbra)
... so I'm sending a second correctly formatted copy.

I'm sorry for the inconvenience.

John
---

Please consider the following change to the cookie 
constructor:

http://cr.openjdk.java.net/~jzavgren/7188517/webrev.01/
<http://cr.openjdk.java.net/%7Ejzavgren/7188517/webrev.01/>

Basically there are two issues:

1.) the existing cookie constructor was allowing cookie
names to
have
a dollar sign as their leading character,
which is "illegal". The constructor code was modified to
disallow
these illegal names.

2.) the API document (notice the specdiff:
http://cr.openjdk.java.net/~jzavgren/7188517/specDiff/
<http://cr.openjdk.java.net/%7Ejzavgren/7188517/specDiff/>)
prohibited
the use of cookie names that are one of the tokens 
reserved for

use by
the cookie protocol, and this restriction is not necessary.

Thanks!
John Zavgren


- Original Message -
From: john.zavg...@oracle.com
To: net-dev@openjdk.java.net
Sent: Thursday, May 2, 2013 10:36:38 AM GMT -05:00 US/Canada
Eastern
Subject: RFR JDK7188517

Greetings: Please consider the following change to the cookie
constructor:
http://cr.openjdk.java.net/~jzavgren/7188517/webrev.01/
Basically there are two issues: 1.) the existing cookie
constructor
was allowing cookie names to have a dollar sign as their
leading
character, which is "illegal". The constructor code was
modified to
disallow these illegal names. 2.) the API document (notice 
the

specdiff:
http://cr.openjdk.java.net/~jzavgren/7188517/specDiff/)
prohibited the use of cookie names that are one of the tokens
reserved
for use by the cookie protocol, and this restriction is not
necessary.
Thanks! John Zavgren

--
-Kurchi











--
-Kurchi

Code Review Request: 7051862: CookiePolicy spec conflicts with CookiePolicy.ACCEPT_ORIGINAL_SERVER

[Kurchi Hazra]

Hi,

Please review this change to fix 7051862. For ACCEPT_ORIGINAL_SERVER, 
shouldAccept() throws a NullPointerException for null arguments.
Out of the many options to fix this, I think the best way is to return 
false if either of the arguments is null - based on the fact that

HttpCookie.domainMathes() returns false for null arguments.

 Bug: http://bugs.sun.com/view_bug.do?bug_id=7051862
 Webrev: http://cr.openjdk.java.net/~khazra/7051862/webrev.00/

Thanks,
Kurchi

Re: Code Review Request: 7051862: CookiePolicy spec conflicts with CookiePolicy.ACCEPT_ORIGINAL_SERVER

[Kurchi Hazra]

On 6/6/2013 2:07 AM, Chris Hegarty wrote:

On 06/06/2013 01:06 AM, Kurchi Hazra wrote:


Hi,

Please review this change to fix 7051862. For ACCEPT_ORIGINAL_SERVER,
shouldAccept() throws a NullPointerException for null arguments.
Out of the many options to fix this, I think the best way is to return
false if either of the arguments is null - based on the fact that
HttpCookie.domainMathes() returns false for null arguments.


I agree with this change. It is a change in behavior ( returns false 
where use to throw NPE ) but unlikely to cause surprise. It is also 
worth noting that we cannot, easily, specify that shouldAccept throw 
NPE since there are two other concrete CookiePolicy instances that 
would need to change ( they would then throw NPE where previously 
returned true/false ), and that would arguably be more surprising.

- Thanks Chris, those indded are the other fixes I had considered.



Many a simple test? Or amend an existing one?


- Working on it, I'll get back today.




-Chris.



  Bug: http://bugs.sun.com/view_bug.do?bug_id=7051862
  Webrev: http://cr.openjdk.java.net/~khazra/7051862/webrev.00/

Thanks,
Kurchi 

Re: Code Review Request: 7051862: CookiePolicy spec conflicts with CookiePolicy.ACCEPT_ORIGINAL_SERVER

[Kurchi Hazra]

How does this look:
http://cr.openjdk.java.net/~khazra/7051862/webrev.01/

- Kurchi


On 6/6/2013 11:15 AM, Kurchi Hazra wrote:


On 6/6/2013 2:07 AM, Chris Hegarty wrote:

On 06/06/2013 01:06 AM, Kurchi Hazra wrote:


Hi,

Please review this change to fix 7051862. For ACCEPT_ORIGINAL_SERVER,
shouldAccept() throws a NullPointerException for null arguments.
Out of the many options to fix this, I think the best way is to return
false if either of the arguments is null - based on the fact that
HttpCookie.domainMathes() returns false for null arguments.


I agree with this change. It is a change in behavior ( returns false 
where use to throw NPE ) but unlikely to cause surprise. It is also 
worth noting that we cannot, easily, specify that shouldAccept throw 
NPE since there are two other concrete CookiePolicy instances that 
would need to change ( they would then throw NPE where previously 
returned true/false ), and that would arguably be more surprising.

- Thanks Chris, those indded are the other fixes I had considered.



Many a simple test? Or amend an existing one?


- Working on it, I'll get back today.




-Chris.



  Bug: http://bugs.sun.com/view_bug.do?bug_id=7051862
  Webrev: http://cr.openjdk.java.net/~khazra/7051862/webrev.00/

Thanks,
Kurchi

Code Review Request: 7051862: CookiePolicy spec conflicts with CookiePolicy.ACCEPT_ORIGINAL_SERVER

[Kurchi Hazra]

Hi,

In HttpUrlConnection, if the chunk length is set to Integer.MAX_VALUE, 
our code was trying to initialize a buffer of size
greater than that, which was resulting in an integer overflow, and 
consequently a NegativeArraySizeException. This fix
ensures that while initializing the internal buffer, we never exceed the 
maximum size of the chunk length fixed by the user,

and hence avert the NegativeArraySizeException.

  I have added a test to cover the case. The test will however try to 
create a buffer of size Integer.MAX_VALUE. I could
change the value of the JVM heap, but the required size (~2 GB) will be 
too large for most machines to support.

However, I am open to advice as to what I should do in this case.

Bug: http://bugs.sun.com/view_bug.do?bug_id=8015421
Webrev: http://cr.openjdk.java.net/~khazra/8015421/webrev.00/



Thanks,
Kurchi

Re: Code Review Request: 7051862: CookiePolicy spec conflicts with CookiePolicy.ACCEPT_ORIGINAL_SERVER

[Kurchi Hazra]

Thanks Matthew! That is what I have currently.

- Kurchi

On 6/12/2013 11:07 AM, Matthew Hall wrote:

Put the relevant part of the test in try-catch. If you get OOME skip / ignore. 
If you get NASE, then you have a regression.

Re: Code Review Request: 7051862: CookiePolicy spec conflicts with CookiePolicy.ACCEPT_ORIGINAL_SERVER

[Kurchi Hazra]

Right, what I have now should not be really problematic, it will throw 
an OutOfMemoryError and I pass the test in that case. But

I am ok with skipping it too.

I'll just push the source code change then.

- Kurchi

On 6/12/2013 11:37 AM, Chris Hegarty wrote:

On 12 Jun 2013, at 19:29, Chris Hegarty  wrote:


The source change looks fine to me, since our implementation treats the given chunk 
length as the complete chunk size ( including header & footer ).

I would prefer to not add a test, if the test will end up being problematic, 
given this is a corner case.

I mean, when running tests in the samevm concurrently.

-Chris


-Chris

On 12 Jun 2013, at 18:48, Kurchi Hazra  wrote:




Hi,

In HttpUrlConnection, if the chunk length is set to Integer.MAX_VALUE, our code 
was trying to initialize a buffer of size
greater than that, which was resulting in an integer overflow, and consequently 
a NegativeArraySizeException. This fix
ensures that while initializing the internal buffer, we never exceed the 
maximum size of the chunk length fixed by the user,
and hence avert the NegativeArraySizeException.

I have added a test to cover the case. The test will however try to create a 
buffer of size Integer.MAX_VALUE. I could
change the value of the JVM heap, but the required size (~2 GB) will be too 
large for most machines to support.
However, I am open to advice as to what I should do in this case.

Bug: http://bugs.sun.com/view_bug.do?bug_id=8015421
Webrev: http://cr.openjdk.java.net/~khazra/8015421/webrev.00/



Thanks,
Kurchi


--
-Kurchi

Code Review Request: 7169142: CookieHandler does not work with localhost

[Kurchi Hazra]

Hi,

The problem that this bug points out is that when a cookie is from 
a localhost and
no domain is set, our implementation sets the default domain of the 
cookie to its uri.getHost() [1], (which is
localhost here), and since the domain now does not contain .local, the 
cookie gets rejected.


Section 3.3.1 of RFC 2965[2] mentions that a domain should default to 
"effective request-host", where the
effective host name is derived from a host name containing no dots by 
adding the string ".local" to it.

This patch proposes adding a .local to hosts not containing any dots.

Bug: http://bugs.sun.com/view_bug.do?bug_id=7169142
Webrev: http://cr.openjdk.java.net/~khazra/7169142/webrev.00/

Thanks,
Kurchi

[1] http://mail.openjdk.java.net/pipermail/net-dev/2009-May/000873.html
[2] http://www.ietf.org/rfc/rfc2965.txt

Re: Code Review Request: 8015421: NegativeArraySizeException occurs in ChunkedOutputStream() with Integer.MAX_VALUE

[Kurchi Hazra]

Apologies, I had used the wrong subject line - correcting it here for 
records.


On 6/12/2013 11:41 AM, Kurchi Hazra wrote:
Right, what I have now should not be really problematic, it will throw 
an OutOfMemoryError and I pass the test in that case. But

I am ok with skipping it too.

I'll just push the source code change then.

- Kurchi

On 6/12/2013 11:37 AM, Chris Hegarty wrote:
On 12 Jun 2013, at 19:29, Chris Hegarty  
wrote:


The source change looks fine to me, since our implementation treats 
the given chunk length as the complete chunk size ( including header 
& footer ).


I would prefer to not add a test, if the test will end up being 
problematic, given this is a corner case.

I mean, when running tests in the samevm concurrently.

-Chris


-Chris

On 12 Jun 2013, at 18:48, Kurchi Hazra 
 wrote:





Hi,

In HttpUrlConnection, if the chunk length is set to 
Integer.MAX_VALUE, our code was trying to initialize a buffer of size
greater than that, which was resulting in an integer overflow, and 
consequently a NegativeArraySizeException. This fix
ensures that while initializing the internal buffer, we never 
exceed the maximum size of the chunk length fixed by the user,

and hence avert the NegativeArraySizeException.

I have added a test to cover the case. The test will however try to 
create a buffer of size Integer.MAX_VALUE. I could
change the value of the JVM heap, but the required size (~2 GB) 
will be too large for most machines to support.

However, I am open to advice as to what I should do in this case.

Bug: http://bugs.sun.com/view_bug.do?bug_id=8015421
Webrev: http://cr.openjdk.java.net/~khazra/8015421/webrev.00/



Thanks,
Kurchi




--
-Kurchi

Re: 7025238 : HttpURLConnection does not handle URLs with an empty path component

[Kurchi Hazra]

Hi Andreas,

  I looked at your changes, and they look good to me. Although we are 
not changing the path of the URL itself (it is not modifiable
too), but from what I see the only other relevant place where URL.path 
is logically used in http client is in ParseUtil.toURI(), which 
basically does

the same thing as your fix.

 I'll run the fix against all networking tests on all platforms and let 
you know how things look.


Thanks!
Kurchi

On 6/19/2013 7:14 AM, Andreas Rieber wrote:

Hi Chris and Kurchi,

i have updated and rerun the test (removed the "@run main/othervm 
B7025238").


New webrev is here:
http://cr.openjdk.java.net/~arieber/7025238/webrev.01/

thanks
Andreas


On 19.06.13 15:33, Chris Hegarty wrote:

Hi Andreas,

On 18/06/2013 20:19, Andreas Rieber wrote:

Hi,

i am looking for a sponsor of this issue.

The bug is here:
http://bugs.sun.com/view_bug.do?bug_id=7025238

First i verified that the problem still exists. Then i checked the
problem against some other web servers. Apache handles a missing "/" in
the path. Tomcat, Microsoft-HTTPAPI/2.0 and the openjdk build in http
server behave with same response: 400 Bad Request.


Nice. Thanks for checking this.

I checked the URL specification but could not see any problem with 
empty

path. The HTTP/1.1 specification is there a bit more detailed. So i
checked HttpURLconnection.java and HttpClient.java where i found the
problem. If the path/file from url.getFile() is null or empty, a "/" is
used but not if the url.getFile() returns only a query string. In that
case the path is empty and should have also a "/".


Sounds reasonable.

A webrev can be found here (to be discussed, i am still new to 
openjdk):

http://cr.openjdk.java.net/~arieber/7025238/webrev.00/


The source changes look good to me.


To write the jtreg test and run them all took longer than the fix ;-) I


Yes, this can often be the case, but thanks for adding a test.

Trivially, the test does not need to be run in other VM mode. You can 
simply remove the line "@run main/othervm B7025238". The default 
action for jtreg is to run the test, essentially "@run main B7025238".


did run jtreg on: |test/java/net, | |test/sun/net, | 
|test/java/security
and | |test/sun/security but sure i don't have all relevant 
platfo||rms.|


Kurchi sent me mail offline, she has agreed to sponsor this change 
for you. I would request that she runs all the networking tests on 
all the platforms before pushing. Not a big problem for us here, we 
have access to all supported platforms.


Thanks again,
-Chris.



thanks
Andreas





--
-Kurchi

Re: 7025238 : HttpURLConnection does not handle URLs with an empty path component

[Kurchi Hazra]

Hi Andreas,

I was not talking about changing the URL implementation, but just 
pointing out that your change doesn't cause incompatibilities in the way we

store elements in the cookiehandler, which is good.

I ran all networking tests on all supported platforms, and things look 
green. I'll push the fix for you in sometime.


Thanks,
Kurchi


On 6/19/2013 11:29 AM, Andreas Rieber wrote:

Hi Kurchi,

to change the path in URL.java would not be a good idea, it supports 
many other protocols as well and what i can read out of the URL 
specification is that path can be empty. True, ParserUtil.toUri() uses 
the path and query elements separate. Here in HttpClient.java i guess 
it saves at least one null check ;-)


cheers
Andreas


On 19.06.13 20:02, Kurchi Hazra wrote:

Hi Andreas,

  I looked at your changes, and they look good to me. Although we are 
not changing the path of the URL itself (it is not modifiable
too), but from what I see the only other relevant place where 
URL.path is logically used in http client is in ParseUtil.toURI(), 
which basically does

the same thing as your fix.

 I'll run the fix against all networking tests on all platforms and 
let you know how things look.


Thanks!
Kurchi

On 6/19/2013 7:14 AM, Andreas Rieber wrote:

Hi Chris and Kurchi,

i have updated and rerun the test (removed the "@run main/othervm 
B7025238").


New webrev is here:
http://cr.openjdk.java.net/~arieber/7025238/webrev.01/

thanks
Andreas


On 19.06.13 15:33, Chris Hegarty wrote:

Hi Andreas,

On 18/06/2013 20:19, Andreas Rieber wrote:

Hi,

i am looking for a sponsor of this issue.

The bug is here:
http://bugs.sun.com/view_bug.do?bug_id=7025238

First i verified that the problem still exists. Then i checked the
problem against some other web servers. Apache handles a missing 
"/" in

the path. Tomcat, Microsoft-HTTPAPI/2.0 and the openjdk build in http
server behave with same response: 400 Bad Request.


Nice. Thanks for checking this.

I checked the URL specification but could not see any problem with 
empty

path. The HTTP/1.1 specification is there a bit more detailed. So i
checked HttpURLconnection.java and HttpClient.java where i found the
problem. If the path/file from url.getFile() is null or empty, a 
"/" is
used but not if the url.getFile() returns only a query string. In 
that

case the path is empty and should have also a "/".


Sounds reasonable.

A webrev can be found here (to be discussed, i am still new to 
openjdk):

http://cr.openjdk.java.net/~arieber/7025238/webrev.00/


The source changes look good to me.

To write the jtreg test and run them all took longer than the fix 
;-) I


Yes, this can often be the case, but thanks for adding a test.

Trivially, the test does not need to be run in other VM mode. You 
can simply remove the line "@run main/othervm B7025238". The 
default action for jtreg is to run the test, essentially "@run main 
B7025238".


did run jtreg on: |test/java/net, | |test/sun/net, | 
|test/java/security
and | |test/sun/security but sure i don't have all relevant 
platfo||rms.|


Kurchi sent me mail offline, she has agreed to sponsor this change 
for you. I would request that she runs all the networking tests on 
all the platforms before pushing. Not a big problem for us here, we 
have access to all supported platforms.


Thanks again,
-Chris.



thanks
Andreas









--
-Kurchi

Re: RFR 8017271: Crash may occur in java.net.DualStackPlainSocketImpl::initIDs due to unchecked values returned from JNI functions

[Kurchi Hazra]

Looks good to me too.

On 6/21/2013 7:03 AM, Alan Bateman wrote:

On 21/06/2013 14:50, Chris Hegarty wrote:
There is a remote possibility that FindClass can return NULL, for a 
class we expect to exist, OOM, etc. Best practice is to check the 
return value before attempting to use it.


CHECK_NULL [1] is a macro used in other places in the networking 
native code for such checks. There will be a pending exception on the 
stack if FindClass fails.


diff -r 4503e04141f7 
src/windows/native/java/net/DualStackPlainSocketImpl.c
--- a/src/windows/native/java/net/DualStackPlainSocketImpl.c Fri Jun 
21 18:26:13 2013 +0800
+++ b/src/windows/native/java/net/DualStackPlainSocketImpl.c Fri Jun 
21 14:39:52 2013 +0100

@@ -43,6 +43,7 @@ JNIEXPORT void JNICALL Java_java_net_Dua
   (JNIEnv *env, jclass clazz) {

 jclass cls = (*env)->FindClass(env, "java/net/InetSocketAddress");
+CHECK_NULL(cls);
 isa_class = (*env)->NewGlobalRef(env, cls);
 isa_ctorID = (*env)->GetMethodID(env, cls, "",
"(Ljava/net/InetAddress;I)V");

-Chris.

[1] #define CHECK_NULL(x) if ((x) == NULL) return;

I can only assume that memory is at exhaustion point for this to 
happen but what you have is fine (we should be checking it everywhere).


-Alan


--
-Kurchi

Re: 6563286: HttpURLConnection.followRedirect(..) follows malformed url.

[Kurchi Hazra]

Hi Andreas,

   Your changes look good to me.  5069130 had been closed as not a 
defect - since we throw an
unchecked exception, and was decided to be the right course of action. 
Now, I don't have a problem with changing
the code to throw a checked exception in this case - but we have to wait 
for an OpenJDK reviewer to agree as well.


 My only suggestion is to add a good message to the IOException being 
thrown, so that it is clear that the URL sent by
the server is problematic and make debugging easier. In the test, the 
first RuntimeException message being

printed in line 46 is probably misleading, but this is a minor issue.

  I'll wait for a JDK reviewer's input on this. I can sponsor the 
change for you. We may need an internal approval here

for the minor API change.

Thanks,
- Kurchi

On 6/24/2013 12:42 PM, Andreas Rieber wrote:

Hi,

here a small fix for 2 older issues. First i wrote the test for wrong
URL at connection time and at redirect time.

Bug(s):
http://bugs.sun.com/view_bug.do?bug_id=6563286
http://bugs.sun.com/view_bug.do?bug_id=5069130

Webrev:
http://cr.openjdk.java.net/~arieber/6563286/webrev.00/

What happens is that the java.net.ProxySelector.select(URI uri) is
called, which throws the IllegalArgumentException if uri is null. But
it uses sun.net.spi.DefaultProxySelector.java and also throws the
exception if uri.getScheme() is null or uri.getHost() is null. I
updated the javadoc in ProxySelector.

To change the exception to an IOException would mean a wider
refactoring and API change, not good. So i checked down to
net.www.protocol.http.HttpURLConnection.java.

There the IllegalArgumentException can be caught and thrown as
IOException. This will handle wrong URLs in both cases (connect and
redirect). I checked also all other possible cases but they are
handled with correct exceptions.

I run all tests on ubuntu, so a test run on all relevant platforms is
required.

thanks for checking this one
Andreas


--
-Kurchi

Re: RFR JDK8015799

[Kurchi Hazra]

Hi John,

  Why not change lines 2810-2811 to:
if (value == null || value.length() == 0)
return value;

Also, lots of formatting issue in the test, especially in 
TestCookieHandler, try-catch block indentation is off in line 54.
 Its also best to stop the server in a finally clause at line 58. 
Alternatively, I also liked Andreas' use of autocloseable in his test 
for 6563286. See [1].


- Kurchi

[1] 
http://cr.openjdk.java.net/~arieber/6563286/webrev.00/test/sun/net/www/http/HttpURLConnection/MalformedFollowRedirect.java.html 



On 6/26/2013 10:54 AM, John Zavgren wrote:

Please consider the following changes to the Java cookie code.

http://cr.openjdk.java.net/~jzavgren/8015799/webrev.02/ 



The problem I fixed occurs when a server returns an array of cookies 
that contains a null cookie.


Thanks
John
--
John Zavgren
john.zavg...@oracle.com
603-821-0904
US-Burlington-MA

Re: RFR JDK8015799

[Kurchi Hazra]

On 6/26/2013 12:17 PM, Kurchi Hazra wrote:

Hi John,

  Why not change lines 2810-2811 to:
if (value == null || value.length() == 0)
return value;
I meant return null. For other cookie-headers too, is there any reason 
for us not returning null if the length of value is 0?


Also, lots of formatting issue in the test, especially in 
TestCookieHandler, try-catch block indentation is off in line 54.
 Its also best to stop the server in a finally clause at line 58. 
Alternatively, I also liked Andreas' use of autocloseable in his test 
for 6563286. See [1].


- Kurchi

[1] 
http://cr.openjdk.java.net/~arieber/6563286/webrev.00/test/sun/net/www/http/HttpURLConnection/MalformedFollowRedirect.java.html 
<http://cr.openjdk.java.net/%7Earieber/6563286/webrev.00/test/sun/net/www/http/HttpURLConnection/MalformedFollowRedirect.java.html>


On 6/26/2013 10:54 AM, John Zavgren wrote:

Please consider the following changes to the Java cookie code.

http://cr.openjdk.java.net/~jzavgren/8015799/webrev.02/ 
<http://cr.openjdk.java.net/%7Ejzavgren/8015799/webrev.02/>


The problem I fixed occurs when a server returns an array of cookies 
that contains a null cookie.


Thanks
John
--
John Zavgren
john.zavg...@oracle.com
603-821-0904
US-Burlington-MA




--
-Kurchi

Re: RFR JDK8015799

[Kurchi Hazra]

Alright, thanks for the clarification - the source code changes are good 
as they are then.


- Kurchi

On 6/26/2013 1:49 PM, Chris Hegarty wrote:
To link this email thread, both in the archives, and for others. The 
call for review on this bug started with:

http://mail.openjdk.java.net/pipermail/net-dev/2013-June/006607.html

On 06/26/2013 08:22 PM, Kurchi Hazra wrote:


On 6/26/2013 12:17 PM, Kurchi Hazra wrote:

Hi John,

  Why not change lines 2810-2811 to:
if (value == null || value.length() == 0)
return value;

I meant return null. For other cookie-headers too, is there any reason
for us not returning null if the length of value is 0?


In the first webrev John had made this change, but I asked him to 
revert it and only change the Set-Cookie(2) headers.


"Since all header retrieval passes through filterHeaderField, in one 
way or another, I'm a little concerned about changing this. Also, as 
the only issue we know of is with Set-Cookie(2), maybe you could add 
the empty string check to these headers only? ( that is to say, move 
the 'value.length() == 0' check into the ' if 
(SET_COOKIE.equalsIgnoreCase(name). '  "


The difference is, currently if a header value is non-null and has a 
length of 0 ( i.e. empty string ), then empty string is returned. With 
the original change then null is returned.


We have been bitten by subtle changes in this area before. Returning 
null from such an API, URLConnection.getHeaderField(s), for cases 
where it did not return null before may lead to NPE's in some 
applications.


-Chris.



Also, lots of formatting issue in the test, especially in
TestCookieHandler, try-catch block indentation is off in line 54.
 Its also best to stop the server in a finally clause at line 58.
Alternatively, I also liked Andreas' use of autocloseable in his test
for 6563286. See [1].


Yes, please.

-Chris.



- Kurchi

[1]
http://cr.openjdk.java.net/~arieber/6563286/webrev.00/test/sun/net/www/http/HttpURLConnection/MalformedFollowRedirect.java.html 

<http://cr.openjdk.java.net/%7Earieber/6563286/webrev.00/test/sun/net/www/http/HttpURLConnection/MalformedFollowRedirect.java.html> 



On 6/26/2013 10:54 AM, John Zavgren wrote:

Please consider the following changes to the Java cookie code.

http://cr.openjdk.java.net/~jzavgren/8015799/webrev.02/
<http://cr.openjdk.java.net/%7Ejzavgren/8015799/webrev.02/>

The problem I fixed occurs when a server returns an array of cookies
that contains a null cookie.

Thanks
John
--
John Zavgren
john.zavg...@oracle.com
603-821-0904
US-Burlington-MA




--
-Kurchi 

Re: RFR JDK8017079

[Kurchi Hazra]

Looks good.

- Kurchi

On 6/27/2013 10:09 AM, Chris Hegarty wrote:

Looks good to me John.

-Chris.

On 27/06/2013 18:02, John Zavgren wrote:

Greetings:
Please consider the following socket constructor changes:

http://cr.openjdk.java.net/~jzavgren/8017079/webrev.01/


Thanks!
John

--
John Zavgren
john.zavg...@oracle.com
603-821-0904
US-Burlington-MA

Re: [8] Request for review: 8020318: Fix doclint issues in java.net

[Kurchi Hazra]

Hi Jason,

I see just one minor issue:
CookieStore.java - the license year should be 2005, 2013?

Else looks good.

Thanks,
Kurchi

On 7/10/2013 5:11 PM, Joe Darcy wrote:

Hi Jason,

Looks good; approved to go back.

Thanks,

-Joe


On 07/10/2013 05:00 PM, Jason Uh wrote:

Hi Joe,

Please review this changeset, which fixes doclint issues in java.net.

Webrev: http://cr.openjdk.java.net/~juh/8020318/webrev.00/

No undesirable changes detected by specdiff.

Thanks,
Jason

Code Review Request: 8017779: java/net/Authenticator/B4769350.java fails

[Kurchi Hazra]

Hi,
 We have observed that test/java/net/Authenticator/B4769350.java 
fails intermittently. Although not reproducible always,
the bug could be in the test/sun/net/www/httptest library that this test 
uses. I have rewritten the test to use
com.sun.net.httpserver instead since we anyway want to move away from 
using the httptest library.


I have used CyclicBarriers to mimic TestHttpServer.rendezvous() and 
CountDownLatches to
mimic TestHttpServer.waitForCondition() and hopefully preserved the rest 
of the logic in the test.

I have not seen the test failing after these changes.

Bug: http://bugs.sun.com/view_bug.do?bug_id=8017779
Webrev: http://cr.openjdk.java.net/~khazra/8017779/webrev.00/

Thanks,
Kurchi

Re: Code Review Request: 8017779: java/net/Authenticator/B4769350.java fails

[Kurchi Hazra]

On 7/17/2013 12:27 AM, Michael McMahon wrote:

On 16/07/13 20:11, Kurchi Hazra wrote:

Hi,
 We have observed that test/java/net/Authenticator/B4769350.java 
fails intermittently. Although not reproducible always,
the bug could be in the test/sun/net/www/httptest library that this 
test uses. I have rewritten the test to use
com.sun.net.httpserver instead since we anyway want to move away from 
using the httptest library.


I have used CyclicBarriers to mimic TestHttpServer.rendezvous() and 
CountDownLatches to
mimic TestHttpServer.waitForCondition() and hopefully preserved the 
rest of the logic in the test.

I have not seen the test failing after these changes.

Bug: http://bugs.sun.com/view_bug.do?bug_id=8017779
Webrev: http://cr.openjdk.java.net/~khazra/8017779/webrev.00/

Thanks,
Kurchi



Kurchi,

Since this is a fairly complicated test, and it's great to see it 
being rewritten,
is there any possibility of adding some commentary that explains the 
purpose
of the synchronization code. For instance, I can't see the purpose of 
the call
on line 163 as it just blocks a thread that has already completed its 
work


Michael


Hi Michael,

  I have just preserved whatever logic the test originally had. The 
specific instance you point out waits
for the T1b() handle to be executed for case 0 before moving forward. 
But I guess it is hard to understand in a

glance and I'll add some more comments and get back with a new webrev.

Thanks,
Kurchi

Code Review Request: 8020498: Crash when both libnet.so and libmawt.so are loaded

[Kurchi Hazra]

Hi,

  We are seeing a crash when both libmawt.so and libnet.so are loaded, and the 
init()
method in src/share/native/java/net/net_util.c is called, but an init() method 
in
libmawt.so gets invoked instead. Although it is difficult to reproduce the 
problem,
declaring the init() in net_util.c can resolve the issue.

This low-risk fix will hopefully be going into 7u40.


Bug: http://bugs.sun.com/view_bug.do?bug_id=8020498
Webrev: http://cr.openjdk.java.net/~khazra/8020498/webrev.00/

Thanks,
Kurchi

Re: Code Review Request: 8020498: Crash when both libnet.so and libmawt.so are loaded

[Kurchi Hazra]

Hi Chris,

Thanks for the additional information.  How about this fix: 
http://cr.openjdk.java.net/~khazra/8020498/webrev.01/


- Kurchi

On 7/18/2013 2:44 AM, Chris Hegarty wrote:

On 18/07/2013 01:19, Kurchi Hazra wrote:


Hi,

We are seeing a crash when both libmawt.so and libnet.so are loaded, and
the init()
method in src/share/native/java/net/net_util.c is called, but an init()
method in
libmawt.so gets invoked instead. Although it is difficult to reproduce
the problem,
declaring the init() in net_util.c can resolve the issue.


Thanks Kurchi,

I have seen this, what appears to be a bizarre linking issue, a few 
times now over the past year but was unable to get to the bottom of it.


Snipped of a stacktrace and comment from 8005450 ( I was unable to get 
the bottom of it, or reproduce ).

---
This looks like a strange linking issue. Relevant part of the stack 
below.

  ...
  C [libmawt.so+0x26c12] init+0x8e;; init+0x8e
  C [libnet.so+0x8b53] NET_SockaddrToInetAddress+0x23;; 
NET_SockaddrToInetAddress+0x23
  C [libnet.so+0xc388] 
Java_java_net_PlainSocketImpl_socketAccept+0x41c;; 
Java_java_net_PlainSocketImpl_socketAccept+0x41c

  j java.net.PlainSocketImpl.socketAccept(Ljava/net/SocketImpl;)V+0
  ...

NET_SockaddrToInetAddress is somehow triggering libmawt init() to be 
called. NET_SockaddrToInetAddress does call a local init() function, 
that is defined within net_util.c, but it is as if the linker has 
decided to call libmawt init().

---

I think your fix is probably ok, but I wonder if we should air on the 
side of caution and maybe give this function a unique name, say 
initInetAddrs ( or something similar ).


-Chris.




This low-risk fix will hopefully be going into 7u40.


Bug: http://bugs.sun.com/view_bug.do?bug_id=8020498
Webrev: http://cr.openjdk.java.net/~khazra/8020498/webrev.00/

Thanks,
Kurchi 

Re: Code Review Request: 8020498: Crash when both libnet.so and libmawt.so are loaded

[Kurchi Hazra]

Alright, I'll add it before asking for approval from 7u-dev.

- Kurchi

On 7/18/2013 11:11 AM, Dmitry Samersoff wrote:

Kurchi,

As you don't plan to use this function outside of net_util.c it's better
to add static keyword as well.

-Dmitry

On 2013-07-18 21:55, Kurchi Hazra wrote:

Hi Chris,

Thanks for the additional information.  How about this fix:
http://cr.openjdk.java.net/~khazra/8020498/webrev.01/

- Kurchi

On 7/18/2013 2:44 AM, Chris Hegarty wrote:

On 18/07/2013 01:19, Kurchi Hazra wrote:

Hi,

We are seeing a crash when both libmawt.so and libnet.so are loaded, and
the init()
method in src/share/native/java/net/net_util.c is called, but an init()
method in
libmawt.so gets invoked instead. Although it is difficult to reproduce
the problem,
declaring the init() in net_util.c can resolve the issue.

Thanks Kurchi,

I have seen this, what appears to be a bizarre linking issue, a few
times now over the past year but was unable to get to the bottom of it.

Snipped of a stacktrace and comment from 8005450 ( I was unable to get
the bottom of it, or reproduce ).
---
This looks like a strange linking issue. Relevant part of the stack
below.
   ...
   C [libmawt.so+0x26c12] init+0x8e;; init+0x8e
   C [libnet.so+0x8b53] NET_SockaddrToInetAddress+0x23;;
NET_SockaddrToInetAddress+0x23
   C [libnet.so+0xc388]
Java_java_net_PlainSocketImpl_socketAccept+0x41c;;
Java_java_net_PlainSocketImpl_socketAccept+0x41c
   j java.net.PlainSocketImpl.socketAccept(Ljava/net/SocketImpl;)V+0
   ...

NET_SockaddrToInetAddress is somehow triggering libmawt init() to be
called. NET_SockaddrToInetAddress does call a local init() function,
that is defined within net_util.c, but it is as if the linker has
decided to call libmawt init().
---

I think your fix is probably ok, but I wonder if we should air on the
side of caution and maybe give this function a unique name, say
initInetAddrs ( or something similar ).

-Chris.



This low-risk fix will hopefully be going into 7u40.


Bug: http://bugs.sun.com/view_bug.do?bug_id=8020498
Webrev: http://cr.openjdk.java.net/~khazra/8020498/webrev.00/

Thanks,
Kurchi

Re: Code Review Request: 8017779: java/net/Authenticator/B4769350.java fails

[Kurchi Hazra]

Hi Michael,

   I added some comments as to what is the purpose of the latches and 
barriers. Those comments alongwith the
comments describing the purpose of the handlers should make the 
synchronization logic more clear. Let me know what

you think: http://cr.openjdk.java.net/~khazra/8017779/webrev.01/

Thanks,
Kurchi

On 7/17/2013 2:07 PM, Kurchi Hazra wrote:


On 7/17/2013 12:27 AM, Michael McMahon wrote:

On 16/07/13 20:11, Kurchi Hazra wrote:

Hi,
 We have observed that test/java/net/Authenticator/B4769350.java 
fails intermittently. Although not reproducible always,
the bug could be in the test/sun/net/www/httptest library that this 
test uses. I have rewritten the test to use
com.sun.net.httpserver instead since we anyway want to move away 
from using the httptest library.


I have used CyclicBarriers to mimic TestHttpServer.rendezvous() and 
CountDownLatches to
mimic TestHttpServer.waitForCondition() and hopefully preserved the 
rest of the logic in the test.

I have not seen the test failing after these changes.

Bug: http://bugs.sun.com/view_bug.do?bug_id=8017779
Webrev: http://cr.openjdk.java.net/~khazra/8017779/webrev.00/

Thanks,
Kurchi



Kurchi,

Since this is a fairly complicated test, and it's great to see it 
being rewritten,
is there any possibility of adding some commentary that explains the 
purpose
of the synchronization code. For instance, I can't see the purpose of 
the call
on line 163 as it just blocks a thread that has already completed its 
work


Michael


Hi Michael,

  I have just preserved whatever logic the test originally had. The 
specific instance you point out waits
for the T1b() handle to be executed for case 0 before moving forward. 
But I guess it is hard to understand in a

glance and I'll add some more comments and get back with a new webrev.

Thanks,
Kurchi


--
-Kurchi