commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dennis Lundberg <denn...@apache.org>
Subject Re: Logging: SimpleLog not thread-safe
Date Sun, 15 Oct 2006 07:14:54 GMT
Hi Simon

I like this solution. It's short, easy to understand and avoids binary 
incompatibility.

-- 
Dennis Lundberg

Simon Kitching wrote:
> After some thought, I've changed my mind about the solution.
> 
> I think the best choice is instead to use a synchronized block:
> 
>        // Append date-time if so configured
>         if(showDateTime) {
>             Date now = new Date();
>             String dateText;
>             synchronized(dateFormatter) {
>                 dateText = dateFormatter.format(now);
>             }
>             buf.append(dateText);
>             buf.append(" ");
>         }
> 
> This:
> (a) Avoids any binary-compatibility issues, as the member is still
> present and still used. There is a potential race condition in that
> subclasses of SimpleLog theoretically need to synchronize on the object
> if they access it, which is a new requirement. However as there is
> *currently* a thread race condition I don't feel we've made anything
> worse in that area.
> (b) works correctly if a subclass replaces the dateFormatter with some
> other object; creating the SimpleDateFormat object in the log method
> would break that.
> (c) Avoids creating the SimpleDateFormat object. In cases where the
> format string is complicated, this construction might actually take some
> significant time. Performing synchronization should actually be faster.
> 
> I've committed R464108 which implements the synchronized fix.
> All comments welcome.
> 
> Regards,
> 
> Simon
> 
> 
> On Sun, 2006-10-15 at 15:50 +1300, Simon Kitching wrote:
>> On Sat, 2006-10-14 at 08:07 -0400, James Carman wrote:
>>> Tomcat had this same issue a while back.  It was trying to use a single
>>> SimpleDateFormat object to parse/format date-valued HTTP headers.  I
>>> submitted the patch for it and I think we decided to just instantiate as
>>> needed.  Local variables are garbage collected much more reliably from
>>> what I understand.  And, as Simon pointed out, the probable disk i/o would
>>> be the big bottleneck, not the object instantiation.
>> Ok, so next issue: this member is declared protected:
>>      static protected DateFormat dateFormatter = null;
>> (and initialised via a static block on the class).
>>
>> This means that removing it is a binary-incompatible change; any
>> subclasses will be broken. Sigh. Once again we learn the lesson that
>> protected members should be avoided just like public ones, as the
>> binary-compatibility issue they cause are exactly the same.
>>
>> I'd be willing to bet money that no-one in the world has ever subclassed
>> SimpleLog, but the issue is there. And of course binary compatibility is
>> a very big issue for such a core lib as logging.
>>
>> Options:
>>  (1) leave this member here, but don't use it.
>>  (2) remove it, and release as 1.1.1
>>  (3) remove it, and increment logging version to 1.2
>>
>> This option does mean that code will continue to run, but if anyone
>> *had* subclassed SimpleLog then they would get output in the default
>> format, not their customised format. As they presumably had a good
>> reason for customising the output, their app may misbehave due to a
>> bugfix-level change.
>>
>> Option 2 could potentially cause an existing application to throw an
>> exception when the SimpleLog subclass tries to access member
>> dateFormatter.
>>
>> Option 3 is the theoretically correct one, but is rather overkill for
>> such a small change.
>>
>> I'm tempted to choose (1), though none of the options are terribly
>> appealing.
>>
>> Comments anyone?
>>
>> Cheers,
>>
>> Simon


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


Mime
View raw message