Re: Code Review 6965924: java.net.HttpCookie using static SimpleDateFormat which is not thread safe

[Michael McMahon]

gustav trede wrote:
On 19 August 2010 17:50, Chris Hegarty > wrote:


gustav trede wrote:


On 18 August 2010 13:22, Michael McMahon
mailto:michael.x.mcma...@oracle.com>
>> wrote:

   gustav trede wrote:


   On 18 August 2010 12:10, Chris Hegarty
mailto:chris.hega...@oracle.com>
   >
   
   
 
 
 





   Why not use a threadlocal dateformater ?.

   perhaps ...

   For certain cases Its also viable to exploit the fact
that its
   enough to generate a new value once per second for HTTP
timestamps.
   Even if its not "needed", it would imo be nice if the
JDK code
   itself could somehow act as reference / good examples
of how to
   THINK(design) and implement.

   I suspect you're looking at this from a server perspective.
This
   code is involved with parsing
   of incoming cookies. So, the generation of timestamps isn't
being
   done here.

Yes i am quite server focused =), servers can act as http
clients too, different kind of intermediate logic etc.
Anyhow, my apologies for wasting your time with this not so
important issue, my brain just pings on code it finds strange /.


We use ThreadLocal to store the formatter for the HttpServer
implementation, com.sun.net.httpserver, see 6967684 "httpserver
using a non thread-safe SimpleDateFormat" [1]. The threads
creating the HTTP timestamps are part of an execurtorService and
will need a timestamp per response. These threads will not be used
for anything else, and are most probably going to handle many
requests. I think adding a threadLocal makes sense for these.

For HttpCookie, it is client side and a thread may only ever
handle a few cookies for its lifetime. I think adding the overhead
of three formatters may just be wasteful since the thread may
never do any more than a few HTTP requests.

Are you ok with this change?

Sounds ok to me, considering how the current overall design and public 
API is, the hands are a bit tied for radical changes.


regards
 gustav trede

Me too.

- Michael.

Re: Code Review 6965924: java.net.HttpCookie using static SimpleDateFormat which is not thread safe

[gustav trede]

On 19 August 2010 17:50, Chris Hegarty  wrote:

> gustav trede wrote:
>
>>
>> On 18 August 2010 13:22, Michael McMahon 
>> > michael.x.mcma...@oracle.com>> wrote:
>>
>>gustav trede wrote:
>>
>>
>>On 18 August 2010 12:10, Chris Hegarty >
>>>>> wrote:
>>
>>   Michael,
>>
>>   java.net.HttpCookie uses static SimpleDateFormat which is not
>>   thread safe. I think the best solution here is to simply create
>>   local SimpleDateFormat as needed.
>>
>>   Webrev:
>>
>> http://cr.openjdk.java.net/~chegar/6965924/webrev.00/webrev/
>>
>>   > >
>>
>>
>>
>>Why not use a threadlocal dateformater ?.
>>
>>perhaps ...
>>
>>For certain cases Its also viable to exploit the fact that its
>>enough to generate a new value once per second for HTTP timestamps.
>>Even if its not "needed", it would imo be nice if the JDK code
>>itself could somehow act as reference / good examples of how to
>>THINK(design) and implement.
>>
>>I suspect you're looking at this from a server perspective. This
>>code is involved with parsing
>>of incoming cookies. So, the generation of timestamps isn't being
>>done here.
>>
>> Yes i am quite server focused =), servers can act as http clients too,
>> different kind of intermediate logic etc.
>> Anyhow, my apologies for wasting your time with this not so important
>> issue, my brain just pings on code it finds strange /.
>>
>
> We use ThreadLocal to store the formatter for the HttpServer
> implementation, com.sun.net.httpserver, see 6967684 "httpserver using a non
> thread-safe SimpleDateFormat" [1]. The threads creating the HTTP timestamps
> are part of an execurtorService and will need a timestamp per response.
> These threads will not be used for anything else, and are most probably
> going to handle many requests. I think adding a threadLocal makes sense for
> these.
>
> For HttpCookie, it is client side and a thread may only ever handle a few
> cookies for its lifetime. I think adding the overhead of three formatters
> may just be wasteful since the thread may never do any more than a few HTTP
> requests.
>
> Are you ok with this change?
>
> Sounds ok to me, considering how the current overall design and public API
is, the hands are a bit tied for radical changes.

regards
 gustav trede

Re: Code Review 6965924: java.net.HttpCookie using static SimpleDateFormat which is not thread safe

[Chris Hegarty]

gustav trede wrote:


On 18 August 2010 13:22, Michael McMahon > wrote:


gustav trede wrote:


On 18 August 2010 12:10, Chris Hegarty mailto:chris.hega...@oracle.com>
>> wrote:

   Michael,

   java.net.HttpCookie uses static SimpleDateFormat which is not
   thread safe. I think the best solution here is to simply create
   local SimpleDateFormat as needed.

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

   



Why not use a threadlocal dateformater ?.

perhaps ...

For certain cases Its also viable to exploit the fact that its
enough to generate a new value once per second for HTTP timestamps.
Even if its not "needed", it would imo be nice if the JDK code
itself could somehow act as reference / good examples of how to
THINK(design) and implement.

I suspect you're looking at this from a server perspective. This
code is involved with parsing
of incoming cookies. So, the generation of timestamps isn't being
done here.

Yes i am quite server focused =), servers can act as http clients too, 
different kind of intermediate logic etc.
Anyhow, my apologies for wasting your time with this not so important 
issue, my brain just pings on code it finds strange /.


We use ThreadLocal to store the formatter for the HttpServer 
implementation, com.sun.net.httpserver, see 6967684 "httpserver using a 
non thread-safe SimpleDateFormat" [1]. The threads creating the HTTP 
timestamps are part of an execurtorService and will need a timestamp per 
response. These threads will not be used for anything else, and are most 
probably going to handle many requests. I think adding a threadLocal 
makes sense for these.


For HttpCookie, it is client side and a thread may only ever handle a 
few cookies for its lifetime. I think adding the overhead of three 
formatters may just be wasteful since the thread may never do any more 
than a few HTTP requests.


Are you ok with this change?

-Chris.

[1] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6967684




regards
  gustav trede

hg: jdk7/tl/langtools: 3 new changesets

[maurizio . cimadamore]

Changeset: c655e0280bdc
Author:mcimadamore
Date:  2010-08-19 11:50 +0100
URL:   http://hg.openjdk.java.net/jdk7/tl/langtools/rev/c655e0280bdc

6886247: regression: javac crashes with an assertion error in Attr.java
Summary: capture conversion does not work on nested types
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/code/Types.java
+ test/tools/javac/generics/wildcards/6886247/T6886247_1.java
+ test/tools/javac/generics/wildcards/6886247/T6886247_2.java
+ test/tools/javac/generics/wildcards/6886247/T6886247_2.out

Changeset: d6fe0ea070aa
Author:mcimadamore
Date:  2010-08-19 11:52 +0100
URL:   http://hg.openjdk.java.net/jdk7/tl/langtools/rev/d6fe0ea070aa

6885255: Improve usability of raw warnings
Summary: raw warnings should be disabled in (i) instanceof expressions and (ii) 
when java.lang.Class is not parameterized
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/comp/Attr.java
! src/share/classes/com/sun/tools/javac/comp/Check.java
! test/tools/javac/warnings/6747671/T6747671.java
! test/tools/javac/warnings/6747671/T6747671.out
+ test/tools/javac/warnings/6885255/T6885255.java
+ test/tools/javac/warnings/6885255/T6885255.out

Changeset: a75770c0d7f6
Author:mcimadamore
Date:  2010-08-19 11:54 +0100
URL:   http://hg.openjdk.java.net/jdk7/tl/langtools/rev/a75770c0d7f6

6977800: Regression: invalid resolution of supertype for local class
Summary: resolution of superclass/superinterfaces in extends/implements clause 
skips local classes
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/comp/Check.java
! src/share/classes/com/sun/tools/javac/comp/MemberEnter.java
+ test/tools/javac/T6977800.java
! test/tools/javac/generics/typevars/5060485/Compatibility.java
+ test/tools/javac/generics/typevars/5060485/Compatibility.out
+ test/tools/javac/generics/typevars/5060485/Compatibility02.java
+ test/tools/javac/generics/typevars/5060485/Compatibility02.out