commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simon Kitching <skitch...@apache.org>
Subject Re: Logging: SimpleLog not thread-safe
Date Sun, 15 Oct 2006 03:12:20 GMT
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
> 


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