Re: RFR JDK7188517

2013-05-08 Thread Michael McMahon 

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.

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/


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

2013-05-08 Thread Chris Hegarty 

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


-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/


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

2013-05-08 Thread Michael McMahon 

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/


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

hg: jdk8/tl/jdk: 8013652: (profiles) Add javax.script to compact1

2013-05-08 Thread alan . bateman 
Changeset: c8f47674d105
Author:alanb
Date:  2013-05-08 18:00 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c8f47674d105

8013652: (profiles) Add javax.script to compact1
Reviewed-by: mchung, dholmes

! makefiles/profile-rtjar-includes.txt

Re: RFR JDK7188517

2013-05-08 Thread John Zavgren 

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/


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








--
John Zavgren
john.zavg...@oracle.com
603-821-0904
US-Burlington-MA

Re: RFR JDK7188517

2013-05-08 Thread 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/


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










--
-Kurchi

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

2013-05-08 Thread 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

2013-05-08 Thread Matthew Hall 
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?

Regards,
Matthew.

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

2013-05-08 Thread 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