tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Remy Maucherat <r...@apache.org>
Subject Re: cvs commit: jakarta-tomcat-catalina/catalina/src/share/org/apache/catalina/connector Request.java
Date Mon, 09 Aug 2004 17:32:12 GMT
Jan Luehe wrote:

> Remy Maucherat wrote:
>
>> Bill Barker wrote:
>>
>>> ----- Original Message ----- From: <luehe@apache.org>
>>> To: <jakarta-tomcat-catalina-cvs@apache.org>
>>> Sent: Thursday, August 05, 2004 6:27 PM
>>> Subject: cvs commit:
>>> jakarta-tomcat-catalina/catalina/src/share/org/apache/catalina/connector 
>>>
>>> Request.java
>>>
>>>
>>>  
>>>
>>>> luehe       2004/08/05 18:27:50
>>>>
>>>>  Modified:    catalina/src/share/org/apache/catalina/connector
>>>>                        Request.java
>>>>  Log:
>>>>  Avoid allocating SimpleDateFormat[] for each request. Instead, 
>>>> declare
>>>>   
>>>
>>>
>>> SimpleDateFormat[] as static and use static initializer to 
>>> initialize it.
>>>  
>>>
>>>
>>> -1.  SimpleDateFormat isn't thread-safe, so you can't have multiple 
>>> threads
>>> accessing them.  That's why they were instance variables, so that 
>>> one one
>>> thread would ever be accessing them.
>>
>
> OK.
>
>>>>  This is consistent with SimpleDateFormat[] in
>>>>   
>>>
>>>
>>> org.apache.tomcat.util.http.FastHttpDateFormat.
>>>
>>> Which is well known to be broken ;-).
>>>  
>>>
>> There's a misunderstanding about the design (I don't think 
>> FastHttpDateFormat is that broken anyway ;) ). The formats passed in 
>> argument has to be thread local, and FHDF won't sync.
>>
>> So -1 too.
>
>
>
> actually, FHDF.parseDate() does sync around SimpleDateFormat.parse()
> if the "threadLocalformats" passed to it is null, in which case it
> falls back to its own static "formats" (notice that in this case,
> FHDF.internalParseDate() is within the synchronized block):
>
>     public static final long parseDate(String value,
>                                        DateFormat[] threadLocalformats) {
>
>         ....
>
>         if (threadLocalformats != null) {
>             date = internalParseDate(value, threadLocalformats);
>             synchronized (parseCache) {
>                 updateCache(parseCache, value, date);
>             }
>         } else {
>             synchronized (parseCache) {
>                 date = internalParseDate(value, formats);
>                 updateCache(parseCache, value, date);
>             }
>         }
>     }
>
> The DateFormat[] passed in from Request.parseDate() is *identical* to
> the static "formats" in FHDF, so why are we not just passing "null"
> from Request.parseDate() and leverage FHDF.formats (instead of having
> each Request object provide its own SimpleDateFormat[] instance var)?
>
> Is it to avoid that the potentially expensive date parsing is
> performed inside the synchronized block?

Yes, it's sort of obvious ;)
How much memory does a formatter uses ? I'd like to point out that 
pasing of dates will happen relatively often (last modified headers in 
requests, etc).

To summarize:
- passing null is correct, but syncs (otherwise it wouldn't work)
- passing a thread local formatter is correct
- passing a shared formatter isn't correct (as there would be thread 
safety issues)

Rémy


---------------------------------------------------------------------
To unsubscribe, e-mail: tomcat-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: tomcat-dev-help@jakarta.apache.org


Mime
View raw message