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

[Michael McMahon]

Chris Hegarty wrote:
> On 07/09/10 12:10, Michael McMahon wrote:
>> 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.
>
> It would really depend on how it is implemented, but my other point 
> stands. Another thread will never be guaranteed to see an updated date 
> string. This goes beyond the previously generated one, theoretically a 
> worker thread may get the date once and never see any update.
>
Ok, as discussed offline. Let's go with your proposal.

- Michael.