You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Martin Wilson <ma...@bright-interactive.com> on 2006/10/06 18:15:10 UTC
Logging: SimpleLog not thread-safe
Hi,
I'm not sure if anyone else uses the SimpleLog class - anyway I've
noticed that SimpleLog.log is not thread-safe. The following code
(starting on line 282):
if(showDateTime) {
buf.append(dateFormatter.format(new Date()));
buf.append(" ");
}
makes an unsynchronized call to dateFormatter.format. As dateFormatter
is an instance variable, and DateFormat.format is not thread-safe, this
will cause problems if more than one thread tried to log at the same
time.
Solution: remove the dateFormatter instance variable and instantiate a
new DateFormat each time in the log method, e.g.
DateFormat dateFormatter = null;
try {
dateFormatter = new
SimpleDateFormat(dateTimeFormat);
}
catch(IllegalArgumentException e) {
dateFormatter = new
SimpleDateFormat(DEFAULT_DATE_TIME_FORMAT);
}
Is anyone available who could make this change?
Thanks,
Martin
___________
Martin Wilson
martinw@bright-interactive.com
Bright Interactive: successfully delivering interactive websites and
business applications
<http://www.bright-interactive.com/> http://www.bright-interactive.com
0870 240 6520
Re: Logging: SimpleLog not thread-safe
Posted by James Carman <ja...@carmanconsulting.com>.
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.
> You could just take a private copy of FastDateFormat from commons-lang
> which is thread-safe. Might bloat your jar-file size though.
> Stephen
>
> Simon Kitching wrote:
>> HI,
>>
>> On Fri, 2006-10-13 at 00:10 -0400, Kenneth Xu wrote:
>>
>>>Yes, it'll be GC'ed when thread is. And initialized when first use in a
>>> new
>>>thread. Here is the test code:
>>
>>
>> But if an application has long-running threads then the object won't be
>> recycled until the thread dies. So an app with 100 threads has 100
>> SimpleDateFormat objects long-term.
>>
>> And as James noted, when using frameworks like Application Servers,
>> threads could be pooled in unexpected ways.
>>
>> I also suspect that in order to fetch data from a ThreadLocal, the JVM
>> effectively performs a get on a synchronised map, ie that ThreadLocal is
>> not much more efficient than having a synchronised static DateFormat on
>> SimpleLog (nb: I have no proof of this, just a hunch).
>>
>> I think the easiest & most reliable solution is to simply create a
>> SimpleDateFormat object in the method that needs it. Note that this is
>> only done after it has been determined that a message *will* be output,
>> which in most cases means that there will be disk io that will have far
>> more impact on the system than creating a simple object. Optimising the
>> path in commons-logging that determines *if* a message is to be logged
>> is important; optimising the actual logging operation is much less
>> critical.
>>
>> Of course I'm open to persuasion on this (eg performance stats)..
>>
>> 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
>
James Carman, President
Carman Consulting, Inc.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: Logging: SimpleLog not thread-safe
Posted by Mario Ivankovits <ma...@ops.co.at>.
Hi Simon!
> I think the best choice is instead to use a synchronized block:
>
> This:
> (a) Avoids any binary-compatibility issues, as the member is still
> present and still used.
Maybe you would like to deprecate the member too, so this can be cleaned
up any time later.Though, its not that important.
Ciao,
Mario
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: Logging: SimpleLog not thread-safe
Posted by Dennis Lundberg <de...@apache.org>.
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
RE: Logging: SimpleLog not thread-safe
Posted by James Carman <ja...@carmanconsulting.com>.
+1. I like it.
-----Original Message-----
From: Simon Kitching [mailto:skitching@apache.org]
Sent: Saturday, October 14, 2006 11:12 PM
To: Jakarta Commons Developers List
Subject: Re: Logging: SimpleLog not thread-safe
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: Logging: SimpleLog not thread-safe
Posted by Simon Kitching <sk...@apache.org>.
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
Re: Logging: SimpleLog not thread-safe
Posted by Simon Kitching <sk...@apache.org>.
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
Re: Logging: SimpleLog not thread-safe
Posted by Stephen Colebourne <sc...@btopenworld.com>.
You could just take a private copy of FastDateFormat from commons-lang
which is thread-safe. Might bloat your jar-file size though.
Stephen
Simon Kitching wrote:
> HI,
>
> On Fri, 2006-10-13 at 00:10 -0400, Kenneth Xu wrote:
>
>>Yes, it'll be GC'ed when thread is. And initialized when first use in a new
>>thread. Here is the test code:
>
>
> But if an application has long-running threads then the object won't be
> recycled until the thread dies. So an app with 100 threads has 100
> SimpleDateFormat objects long-term.
>
> And as James noted, when using frameworks like Application Servers,
> threads could be pooled in unexpected ways.
>
> I also suspect that in order to fetch data from a ThreadLocal, the JVM
> effectively performs a get on a synchronised map, ie that ThreadLocal is
> not much more efficient than having a synchronised static DateFormat on
> SimpleLog (nb: I have no proof of this, just a hunch).
>
> I think the easiest & most reliable solution is to simply create a
> SimpleDateFormat object in the method that needs it. Note that this is
> only done after it has been determined that a message *will* be output,
> which in most cases means that there will be disk io that will have far
> more impact on the system than creating a simple object. Optimising the
> path in commons-logging that determines *if* a message is to be logged
> is important; optimising the actual logging operation is much less
> critical.
>
> Of course I'm open to persuasion on this (eg performance stats)..
>
> 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
RE: Logging: SimpleLog not thread-safe
Posted by Simon Kitching <sk...@apache.org>.
HI,
On Fri, 2006-10-13 at 00:10 -0400, Kenneth Xu wrote:
> Yes, it'll be GC'ed when thread is. And initialized when first use in a new
> thread. Here is the test code:
But if an application has long-running threads then the object won't be
recycled until the thread dies. So an app with 100 threads has 100
SimpleDateFormat objects long-term.
And as James noted, when using frameworks like Application Servers,
threads could be pooled in unexpected ways.
I also suspect that in order to fetch data from a ThreadLocal, the JVM
effectively performs a get on a synchronised map, ie that ThreadLocal is
not much more efficient than having a synchronised static DateFormat on
SimpleLog (nb: I have no proof of this, just a hunch).
I think the easiest & most reliable solution is to simply create a
SimpleDateFormat object in the method that needs it. Note that this is
only done after it has been determined that a message *will* be output,
which in most cases means that there will be disk io that will have far
more impact on the system than creating a simple object. Optimising the
path in commons-logging that determines *if* a message is to be logged
is important; optimising the actual logging operation is much less
critical.
Of course I'm open to persuasion on this (eg performance stats)..
Cheers,
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
RE: Logging: SimpleLog not thread-safe
Posted by Kenneth Xu <ke...@gmail.com>.
Yes, it'll be GC'ed when thread is. And initialized when first use in a new
thread. Here is the test code:
public class TestMe {
private static String pattern = "yyyy-mm-dd";
private static ThreadLocal formatter = new ThreadLocal() {
protected Object initialValue() {
return new SimpleDateFormat(pattern);
}
};
public static void main(String[] args) throws Exception{
test2();
}
public static void test2() {
Date date = new Date();
int size=1000000;
long l;
l = System.currentTimeMillis();
for (int i=0; i<size; i++) {
((DateFormat)formatter.get()).format(date);
}
System.out.println(System.currentTimeMillis()-l);
l = System.currentTimeMillis();
for (int i=0; i<size; i++) {
(new SimpleDateFormat(pattern)).format(date);
}
System.out.println(System.currentTimeMillis()-l);
}
}
-----Original Message-----
From: James Carman [mailto:james@carmanconsulting.com]
Sent: Thursday, October 12, 2006 10:26 AM
To: 'Jakarta Commons Developers List'
Subject: RE: Logging: SimpleLog not thread-safe
I guess it will clean itself up when the thread is garbage collected. I'd
play around with it just to be sure, though. Application servers typically
use a thread pool, so they won't be garbage collected. But, that's okay,
because you want to be able to reuse them if you can to avoid having to
reconstruct it from scratch.
-----Original Message-----
From: James Carman [mailto:james@carmanconsulting.com]
Sent: Thursday, October 12, 2006 6:23 AM
To: Jakarta Commons Developers List
Subject: RE: Logging: SimpleLog not thread-safe
How do you know when to clean it up? Just instantiate a new one each time.
> It might do; not sure what the performance of ThreadLocal is. However
> the price is extra memory usage: a DateFormatter for every thread in the
> system that ever logs a message.
>
> On Wed, 2006-10-11 at 17:42 -0400, Kenneth Xu wrote:
>> Hi,
>>
>> I think a thread local formatter will give us better performance than
>> creating one new in every log invocation.
>>
>> Just my 2 cents,
>>
>> Thanks,
>> Ken
>>
>> -----Original Message-----
>> From: Simon Kitching [mailto:skitching@apache.org]
>> Sent: Wednesday, October 11, 2006 5:05 AM
>> To: Jakarta Commons Developers List
>> Subject: Re: Logging: SimpleLog not thread-safe
>>
>> Hi Martin,
>>
>> Thanks very much for letting us know about this. I think you're right
>> about there being a thread-safety issue.
>>
>> SimpleLog is definitely in use, but obviously this bug will only be
>> triggered under pretty rare conditions, and I expect would usually just
>> result in a malformed date string which may not be noticed anyway.
>>
>> But it should be fixed; I'll try to do that this weekend.
>>
>> Regards,
>>
>> Simon
>>
>> On Fri, 2006-10-06 at 17:15 +0100, Martin Wilson wrote:
>> > Hi,
>> >
>> > I'm not sure if anyone else uses the SimpleLog class - anyway I've
>> > noticed that SimpleLog.log is not thread-safe. The following code
>> > (starting on line 282):
>> >
>> > if(showDateTime) {
>> > buf.append(dateFormatter.format(new Date()));
>> > buf.append(" ");
>> > }
>> >
>> > makes an unsynchronized call to dateFormatter.format. As dateFormatter
>> > is an instance variable, and DateFormat.format is not thread-safe,
>> this
>> > will cause problems if more than one thread tried to log at the same
>> > time.
>> >
>> > Solution: remove the dateFormatter instance variable and instantiate a
>> > new DateFormat each time in the log method, e.g.
>> >
>> > DateFormat dateFormatter = null;
>> > try {
>> > dateFormatter = new
>> > SimpleDateFormat(dateTimeFormat);
>> > }
>> > catch(IllegalArgumentException e) {
>> > dateFormatter = new
>> > SimpleDateFormat(DEFAULT_DATE_TIME_FORMAT);
>> > }
>> >
>> > Is anyone available who could make this change?
>> >
>> > Thanks,
>> > Martin
>>
>> > ___________
>> >
>> > Martin Wilson
>> > martinw@bright-interactive.com
>> >
>> > Bright Interactive: successfully delivering interactive websites and
>> > business applications
>> > <http://www.bright-interactive.com/>
>> http://www.bright-interactive.com
>> > 0870 240 6520
>> >
>>
>>
>> ---------------------------------------------------------------------
>> 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
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
James Carman, President
Carman Consulting, Inc.
---------------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
RE: Logging: SimpleLog not thread-safe
Posted by James Carman <ja...@carmanconsulting.com>.
I guess it will clean itself up when the thread is garbage collected. I'd
play around with it just to be sure, though. Application servers typically
use a thread pool, so they won't be garbage collected. But, that's okay,
because you want to be able to reuse them if you can to avoid having to
reconstruct it from scratch.
-----Original Message-----
From: James Carman [mailto:james@carmanconsulting.com]
Sent: Thursday, October 12, 2006 6:23 AM
To: Jakarta Commons Developers List
Subject: RE: Logging: SimpleLog not thread-safe
How do you know when to clean it up? Just instantiate a new one each time.
> It might do; not sure what the performance of ThreadLocal is. However
> the price is extra memory usage: a DateFormatter for every thread in the
> system that ever logs a message.
>
> On Wed, 2006-10-11 at 17:42 -0400, Kenneth Xu wrote:
>> Hi,
>>
>> I think a thread local formatter will give us better performance than
>> creating one new in every log invocation.
>>
>> Just my 2 cents,
>>
>> Thanks,
>> Ken
>>
>> -----Original Message-----
>> From: Simon Kitching [mailto:skitching@apache.org]
>> Sent: Wednesday, October 11, 2006 5:05 AM
>> To: Jakarta Commons Developers List
>> Subject: Re: Logging: SimpleLog not thread-safe
>>
>> Hi Martin,
>>
>> Thanks very much for letting us know about this. I think you're right
>> about there being a thread-safety issue.
>>
>> SimpleLog is definitely in use, but obviously this bug will only be
>> triggered under pretty rare conditions, and I expect would usually just
>> result in a malformed date string which may not be noticed anyway.
>>
>> But it should be fixed; I'll try to do that this weekend.
>>
>> Regards,
>>
>> Simon
>>
>> On Fri, 2006-10-06 at 17:15 +0100, Martin Wilson wrote:
>> > Hi,
>> >
>> > I'm not sure if anyone else uses the SimpleLog class - anyway I've
>> > noticed that SimpleLog.log is not thread-safe. The following code
>> > (starting on line 282):
>> >
>> > if(showDateTime) {
>> > buf.append(dateFormatter.format(new Date()));
>> > buf.append(" ");
>> > }
>> >
>> > makes an unsynchronized call to dateFormatter.format. As dateFormatter
>> > is an instance variable, and DateFormat.format is not thread-safe,
>> this
>> > will cause problems if more than one thread tried to log at the same
>> > time.
>> >
>> > Solution: remove the dateFormatter instance variable and instantiate a
>> > new DateFormat each time in the log method, e.g.
>> >
>> > DateFormat dateFormatter = null;
>> > try {
>> > dateFormatter = new
>> > SimpleDateFormat(dateTimeFormat);
>> > }
>> > catch(IllegalArgumentException e) {
>> > dateFormatter = new
>> > SimpleDateFormat(DEFAULT_DATE_TIME_FORMAT);
>> > }
>> >
>> > Is anyone available who could make this change?
>> >
>> > Thanks,
>> > Martin
>>
>> > ___________
>> >
>> > Martin Wilson
>> > martinw@bright-interactive.com
>> >
>> > Bright Interactive: successfully delivering interactive websites and
>> > business applications
>> > <http://www.bright-interactive.com/>
>> http://www.bright-interactive.com
>> > 0870 240 6520
>> >
>>
>>
>> ---------------------------------------------------------------------
>> 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
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
James Carman, President
Carman Consulting, Inc.
---------------------------------------------------------------------
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
RE: Logging: SimpleLog not thread-safe
Posted by James Carman <ja...@carmanconsulting.com>.
How do you know when to clean it up? Just instantiate a new one each time.
> It might do; not sure what the performance of ThreadLocal is. However
> the price is extra memory usage: a DateFormatter for every thread in the
> system that ever logs a message.
>
> On Wed, 2006-10-11 at 17:42 -0400, Kenneth Xu wrote:
>> Hi,
>>
>> I think a thread local formatter will give us better performance than
>> creating one new in every log invocation.
>>
>> Just my 2 cents,
>>
>> Thanks,
>> Ken
>>
>> -----Original Message-----
>> From: Simon Kitching [mailto:skitching@apache.org]
>> Sent: Wednesday, October 11, 2006 5:05 AM
>> To: Jakarta Commons Developers List
>> Subject: Re: Logging: SimpleLog not thread-safe
>>
>> Hi Martin,
>>
>> Thanks very much for letting us know about this. I think you're right
>> about there being a thread-safety issue.
>>
>> SimpleLog is definitely in use, but obviously this bug will only be
>> triggered under pretty rare conditions, and I expect would usually just
>> result in a malformed date string which may not be noticed anyway.
>>
>> But it should be fixed; I'll try to do that this weekend.
>>
>> Regards,
>>
>> Simon
>>
>> On Fri, 2006-10-06 at 17:15 +0100, Martin Wilson wrote:
>> > Hi,
>> >
>> > I'm not sure if anyone else uses the SimpleLog class - anyway I've
>> > noticed that SimpleLog.log is not thread-safe. The following code
>> > (starting on line 282):
>> >
>> > if(showDateTime) {
>> > buf.append(dateFormatter.format(new Date()));
>> > buf.append(" ");
>> > }
>> >
>> > makes an unsynchronized call to dateFormatter.format. As dateFormatter
>> > is an instance variable, and DateFormat.format is not thread-safe,
>> this
>> > will cause problems if more than one thread tried to log at the same
>> > time.
>> >
>> > Solution: remove the dateFormatter instance variable and instantiate a
>> > new DateFormat each time in the log method, e.g.
>> >
>> > DateFormat dateFormatter = null;
>> > try {
>> > dateFormatter = new
>> > SimpleDateFormat(dateTimeFormat);
>> > }
>> > catch(IllegalArgumentException e) {
>> > dateFormatter = new
>> > SimpleDateFormat(DEFAULT_DATE_TIME_FORMAT);
>> > }
>> >
>> > Is anyone available who could make this change?
>> >
>> > Thanks,
>> > Martin
>>
>> > ___________
>> >
>> > Martin Wilson
>> > martinw@bright-interactive.com
>> >
>> > Bright Interactive: successfully delivering interactive websites and
>> > business applications
>> > <http://www.bright-interactive.com/>
>> http://www.bright-interactive.com
>> > 0870 240 6520
>> >
>>
>>
>> ---------------------------------------------------------------------
>> 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
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
James Carman, President
Carman Consulting, Inc.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
RE: Logging: SimpleLog not thread-safe
Posted by Simon Kitching <sk...@apache.org>.
It might do; not sure what the performance of ThreadLocal is. However
the price is extra memory usage: a DateFormatter for every thread in the
system that ever logs a message.
On Wed, 2006-10-11 at 17:42 -0400, Kenneth Xu wrote:
> Hi,
>
> I think a thread local formatter will give us better performance than
> creating one new in every log invocation.
>
> Just my 2 cents,
>
> Thanks,
> Ken
>
> -----Original Message-----
> From: Simon Kitching [mailto:skitching@apache.org]
> Sent: Wednesday, October 11, 2006 5:05 AM
> To: Jakarta Commons Developers List
> Subject: Re: Logging: SimpleLog not thread-safe
>
> Hi Martin,
>
> Thanks very much for letting us know about this. I think you're right
> about there being a thread-safety issue.
>
> SimpleLog is definitely in use, but obviously this bug will only be
> triggered under pretty rare conditions, and I expect would usually just
> result in a malformed date string which may not be noticed anyway.
>
> But it should be fixed; I'll try to do that this weekend.
>
> Regards,
>
> Simon
>
> On Fri, 2006-10-06 at 17:15 +0100, Martin Wilson wrote:
> > Hi,
> >
> > I'm not sure if anyone else uses the SimpleLog class - anyway I've
> > noticed that SimpleLog.log is not thread-safe. The following code
> > (starting on line 282):
> >
> > if(showDateTime) {
> > buf.append(dateFormatter.format(new Date()));
> > buf.append(" ");
> > }
> >
> > makes an unsynchronized call to dateFormatter.format. As dateFormatter
> > is an instance variable, and DateFormat.format is not thread-safe, this
> > will cause problems if more than one thread tried to log at the same
> > time.
> >
> > Solution: remove the dateFormatter instance variable and instantiate a
> > new DateFormat each time in the log method, e.g.
> >
> > DateFormat dateFormatter = null;
> > try {
> > dateFormatter = new
> > SimpleDateFormat(dateTimeFormat);
> > }
> > catch(IllegalArgumentException e) {
> > dateFormatter = new
> > SimpleDateFormat(DEFAULT_DATE_TIME_FORMAT);
> > }
> >
> > Is anyone available who could make this change?
> >
> > Thanks,
> > Martin
>
> > ___________
> >
> > Martin Wilson
> > martinw@bright-interactive.com
> >
> > Bright Interactive: successfully delivering interactive websites and
> > business applications
> > <http://www.bright-interactive.com/> http://www.bright-interactive.com
> > 0870 240 6520
> >
>
>
> ---------------------------------------------------------------------
> 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
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
RE: Logging: SimpleLog not thread-safe
Posted by Kenneth Xu <ke...@gmail.com>.
Hi,
I think a thread local formatter will give us better performance than
creating one new in every log invocation.
Just my 2 cents,
Thanks,
Ken
-----Original Message-----
From: Simon Kitching [mailto:skitching@apache.org]
Sent: Wednesday, October 11, 2006 5:05 AM
To: Jakarta Commons Developers List
Subject: Re: Logging: SimpleLog not thread-safe
Hi Martin,
Thanks very much for letting us know about this. I think you're right
about there being a thread-safety issue.
SimpleLog is definitely in use, but obviously this bug will only be
triggered under pretty rare conditions, and I expect would usually just
result in a malformed date string which may not be noticed anyway.
But it should be fixed; I'll try to do that this weekend.
Regards,
Simon
On Fri, 2006-10-06 at 17:15 +0100, Martin Wilson wrote:
> Hi,
>
> I'm not sure if anyone else uses the SimpleLog class - anyway I've
> noticed that SimpleLog.log is not thread-safe. The following code
> (starting on line 282):
>
> if(showDateTime) {
> buf.append(dateFormatter.format(new Date()));
> buf.append(" ");
> }
>
> makes an unsynchronized call to dateFormatter.format. As dateFormatter
> is an instance variable, and DateFormat.format is not thread-safe, this
> will cause problems if more than one thread tried to log at the same
> time.
>
> Solution: remove the dateFormatter instance variable and instantiate a
> new DateFormat each time in the log method, e.g.
>
> DateFormat dateFormatter = null;
> try {
> dateFormatter = new
> SimpleDateFormat(dateTimeFormat);
> }
> catch(IllegalArgumentException e) {
> dateFormatter = new
> SimpleDateFormat(DEFAULT_DATE_TIME_FORMAT);
> }
>
> Is anyone available who could make this change?
>
> Thanks,
> Martin
> ___________
>
> Martin Wilson
> martinw@bright-interactive.com
>
> Bright Interactive: successfully delivering interactive websites and
> business applications
> <http://www.bright-interactive.com/> http://www.bright-interactive.com
> 0870 240 6520
>
---------------------------------------------------------------------
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
Re: Logging: SimpleLog not thread-safe
Posted by Simon Kitching <sk...@apache.org>.
Hi Martin,
Thanks very much for letting us know about this. I think you're right
about there being a thread-safety issue.
SimpleLog is definitely in use, but obviously this bug will only be
triggered under pretty rare conditions, and I expect would usually just
result in a malformed date string which may not be noticed anyway.
But it should be fixed; I'll try to do that this weekend.
Regards,
Simon
On Fri, 2006-10-06 at 17:15 +0100, Martin Wilson wrote:
> Hi,
>
> I'm not sure if anyone else uses the SimpleLog class - anyway I've
> noticed that SimpleLog.log is not thread-safe. The following code
> (starting on line 282):
>
> if(showDateTime) {
> buf.append(dateFormatter.format(new Date()));
> buf.append(" ");
> }
>
> makes an unsynchronized call to dateFormatter.format. As dateFormatter
> is an instance variable, and DateFormat.format is not thread-safe, this
> will cause problems if more than one thread tried to log at the same
> time.
>
> Solution: remove the dateFormatter instance variable and instantiate a
> new DateFormat each time in the log method, e.g.
>
> DateFormat dateFormatter = null;
> try {
> dateFormatter = new
> SimpleDateFormat(dateTimeFormat);
> }
> catch(IllegalArgumentException e) {
> dateFormatter = new
> SimpleDateFormat(DEFAULT_DATE_TIME_FORMAT);
> }
>
> Is anyone available who could make this change?
>
> Thanks,
> Martin
> ___________
>
> Martin Wilson
> martinw@bright-interactive.com
>
> Bright Interactive: successfully delivering interactive websites and
> business applications
> <http://www.bright-interactive.com/> http://www.bright-interactive.com
> 0870 240 6520
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
RE: Logging: SimpleLog not thread-safe
Posted by Kenneth Xu <ke...@gmail.com>.
> Solution: remove the dateFormatter instance variable and instantiate a
> new DateFormat each time in the log method, e.g.
How about a ThreadLocal variable?
Thanks,
Ken
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org