Code Review 6967684: httpserver using a non thread-safe SimpleDateFormat

[Michael McMahon]

Chris Hegarty wrote:
>
>
> On 07/09/10 10:10, Michael McMahon wrote:
>> Chris Hegarty wrote:
>>> Michael,
>>>
>>> CR 6967684: httpserver using a non thread-safe SimpleDateFormat
>>>
>>> Bug description says it all. I guess in reality this may have gone
>>> unseen for so long because, if two or more threads were servicing
>>> requests the dates they would be generating should be fairly similar,
>>> but worst case they could actually see partial of corrupt dates. This
>>> bug causes random httpserver tests to fail when run with assertions
>>> enabled.
>>>
>>> The most reasonable solution is to use a thread-local instance of the
>>> formatter, rather than creating one per request.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~chegar/6967684/webrev.00/webrev/
>>>
>>> Thanks,
>>> -Chris.
>> Interesting bug.
>>
>> I wonder would it be better to use the ServerTimerTask to read the time
>> of day each time
>> it wakes up and the thread handlers would just read this value each time
>> they need it?
>
> To clarify, the ServerTimerTask would provide the already formatted 
> date String, right?
>
yes.
>> That's probably what should have been done originally, as it scales
>> better with more traffic
>> and formatting dates is probably quite compute intensive.
>
> A simple test shows 1,000,000 formats of new Date() takes 4407 millis, 
> I don't think this is too expensive.
>
Does that include the cost of the new Date() itself? in the 
ServerTimerTask we're already calling
System.currentTimeMillis() and that could be used to create the Date, 
avoid another call to get the time
from the OS
> If ServerTimerTask is to provide the date then we'll have to 
> synchronize its access. This may actually be more expensive for 
> servers with many many threads. It would be a highly contended monitor 
> since every thread will be required to hold it, albeit not for long.
>
I'm not sure it would need to be synchronized. Any thread would get a 
valid reference to either the new
value or the old one. It wouldn't really matter.
> Also, The default clock tick is 10 secs. This seems like a quite a 
> large value for caching the date timestamp.
>
10 seconds is possibly a bit high alright. But, we could change that.

- Michael.