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

[Chris Hegarty]

On 07/09/10 11:27, Michael McMahon wrote:
> 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

The test includes the 'new Date()'. I don't think 
System.currentTimeMillis() is really that expensive. It effectively 
calls gettimeofday, which is a system call but typically is quick since 
it just reads a register value and doesn't need to call into the kernel.

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

It's not just the old ref. You could get corrupted data, and are never 
guaranteed to every see a more recent version. Not using synchronization 
here would potentially be worse than the original bug.

-Chris.

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