commons-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Wilson, Chris (SAHQ I/S)" <chris.wil...@valero.com>
Subject RE: Log4JLogger Not Thread Safe?
Date Tue, 29 Nov 2005 19:31:03 GMT
To preface, let me state I am certainly not a JVM, compiler, or
threading expert!  The link you provided
(http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html)
and this article by Doug Lea (http://gee.cs.oswego.edu/dl/cpj/jmm.html)
talk about the compiler or memory system reordering statements as long
as "as-if-serial" semantics are preserved.

Since getLogger() is not synchronized, I suppose the only danger could
be the following scenario:

Thread A enters getLogger() and determines that the logger instance is
null (because the class has been recently deserialized).  Thread A
proceeds to get an instance of logger from the underlying logging
system.  Because getLogger() is not synchronized, a reference to the new
logger instance may be set in the local logger variable before the
object is fully constructed.  At this point Thread A gets swapped out
for another thread to run.

Thread B enters getLogger() and sees that the logger instance is not
null and happily returns it for use.  The code using the logger instance
calls some methods on it before it has been completely setup (still
waiting to happen in Thread A which is waiting to run again).

At this point, what happens?  The behavior of the logger instance is
undefined as I understand it.  It may not fail, but could possibly act
in a strange way?

Chris

-----Original Message-----
From: robert burrell donkin
[mailto:robertburrelldonkin@blueyonder.co.uk] 
Sent: Tuesday, November 29, 2005 1:09 PM
To: Jakarta Commons Users List
Subject: Re: Log4JLogger Not Thread Safe?

On Mon, 2005-11-28 at 10:15 -0600, Wilson, Chris (SAHQ I/S) wrote:
> Hello,
> 
> I was looking at the source code of Log4JLogger and it seems to me
that
> it may not be thread safe after deserialization.
> 
> The implementation holds a transient variable called logger which is
> initialized at construction time.  There is no synchronized access to
> the logger variable anywhere in the class.  The classes encapsulates
all
> access to the logger variable via the getLogger() method which
> optionally initializes it if it's null.
> 
> Since the logger variable is transient, when a Log4JLogger instance is
> deserialized, the logger variable will be null and thus reinitialized
> the next time getLogger() is called.  If a Log4JLogger instance is
held
> as a static variable in a containing class (a standard commons logging
> pattern), it would be possible for two threads to call getLogger() at
> the same time which could lead to a thread unsafe scenario since
> getLogger() is not synchronized.
> 
> Even if the underlying log implementation is thread safe (which Log4J
> is), this class accesses and stores a reference to its returned logger
> in a thread unsafe way.
> 
> Unless I'm missing something, it seems this class should protect
access
> to the logger variable if it is to be dynamically created via
> getLogger() or override the readObject() method so that it's created
> during deserialization (which I'm assuming would be thread safe as the
> JVM would release the instance for use until it had been completely
> deserialized).

i've take a look and here's how i see things...

since the class is unsynchronized, it is likely that concurrent use
cases could be developed without the need for serialization. (there are
a lot of oddities in java. for example see
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html.)

JCL is (for better or worse) one of the most widely used java libraries
and is now approaching maturity after many years of use. though it has
it's deficiencies, i would have expected any synchronization issues to
have emerged by now.

for me, the key question is: what are the possible scenarios when two
threads concurrently enter getLogger() and are any of these unsafe?

luckily, the method is very simple. the worst case scenario (i can see)
results in two threads both setting the logger to a non-null value. this
would be safe but a little inefficient. 

but if you think i'm wrong or my arguments seem unconvincing, please
take this as a challenge to create a unit test that proves log4j logger
is thread unsafe :)

- robert


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


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


Mime
View raw message