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

[gustav trede]

On 19 August 2010 17:50, Chris Hegarty <chris.hegarty at oracle.com> wrote:

> 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/>
>>           <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?
>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/net-dev/attachments/20100819/b360c221/attachment.html