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 <michael.x.mcmahon at oracle.com 
> <mailto:michael.x.mcmahon at oracle.com>> wrote:
> 
>     gustav trede wrote:
> 
> 
>         On 18 August 2010 12:10, Chris Hegarty <chris.hegarty at oracle.com
>         <mailto:chris.hegarty at oracle.com>
>         <mailto:chris.hegarty at oracle.com
>         <mailto:chris.hegarty at 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/
>         <http://cr.openjdk.java.net/%7Echegar/6965924/webrev.00/webrev/>
>            <http://cr.openjdk.java.net/%7Echegar/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