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

[Michael McMahon]

Chris Hegarty wrote:
>
>
> 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.
>
What data could be corrupted?

I'm pretty sure object references can't be corrupted. Callers will see 
either a valid reference
to the old string or a valid reference to the new string - the string 
being the fully formatted
date.

- Michael.