Here's the threading issue I see:
public class DatabaseManager
{
private transient Logger logger;
public Database getDatabase() {
Logger logger = getLogger();
logger.debug("Getting database");
// do something here and return database
}
protected Logger getLogger() {
if(logger == null)
logger = LogFactory.getLogger(getClass());
}
}
If two threads call getDatabase() at the same time, there is a chance that you
could wind up allocating two loggers when only one is needed. Since the
logger is thread-safe, we need to make getting a reference to a logger
thread-safe, too.
There are three ways of doing it:
1) Synchronize getLogger() - This introduces synchronization overhead every
time you need a logger.
2) Synchronize all methods that call getLogger() - Ugly.
3) Make the logger a static variable - This moves the initialization of the
logger to when the class is loaded and thus, removes any threading issues.
The penalty for such a refactoring is having one line of duplicate code in
each class:
private static Logger logger = LogFactory.getLogger(Database.class.getName());
That's why I was suggesting using #3.
--Kevin
On Tuesday 19 November 2002 07:52 pm, Kevin Ross wrote:
> >Obtaining loggers might be a bad example. All I'm saying that in my
>
> experience, this kind of resource initialization scheme is better off
> being thread safe from >the start. It eliminates a whole family of bugs
> that you need to worry about down the road.
>
> Agreed, but in the case of logging, I believe you should err on the side
> of performance for the greater good, the 80%+ case. Bothering with
> paranoid initialization (synchronization) makes everyone instance pay
> the price.
>
> Just got your next message. I'll say it one more time, WHAT thread
> safety issues???? The logger is responsible for thread safety. I don't
> implement synchronization when initializing or using a Logger, because
> it is unnecessary for the client to do so. I haven't for years now, and
> I haven't had one problem in organizations with traffic from a few
> thousand hits a day, to a few million.
>
> Duplication of code is one of the fundamentally wrong things to do in OO
> programming, and I agree, therefore I stay away from it.
>
> -Kevin Ross
>
> Kevin A. Smith wrote:
> > I just thought I'd bring it up. Personally, I err on the side of
> > paranoia when I can. I don't enjoy debugging threading issues.
> >
> > If the LogFactory is recycling logger instances, then at most you pay
> > the price of one extra method call. BUT, if the LogFactory is creating
> > a new logger per call, then you're (potentially) paying the price of
> > creating an object that won't be used and an extra method call.
> >
> > Obtaining loggers might be a bad example. All I'm saying that in my
> > experience, this kind of resource initialization scheme is better off
> > being thread safe from the start. It eliminates a whole family of bugs
> > that you need to worry about down the road.
> >
> > --Kevin
> >
> > -----Original Message-----
> > From: Kevin Ross [mailto:[EMAIL PROTECTED]
> > Sent: Tuesday, November 19, 2002 2:30 PM
> > To: [EMAIL PROTECTED]
> > Subject: Re: LogFactory.getLogger()
> >
> > Threading issues?? I knew someone would bring that up. Thread
> > safetyness is the responsibility, gladly accepted by a logger
> > instance, not on the classes using it. There is NO reason why we
> > need paranoid checking here to only get one instance, since it is
> > the responsibility of the logging implementation to even provide
> > the instance in the first place. Do you really think you'll get
> > different instances? Probably not, worst case scenario, you call
> > Logger.getLogger() twice and initialize your instance twice in a
> > race condition. Since it is of zero consequence either way, there
> > is no reason to do it differently. If you are particularly
> > worried, just do:
> >
> > private transient Logger logger = LogFactory.getLogger(getClass());
> >
> > You CANNOT use MyClass.class and have MySubClass.class show up in
> > the log statements, since this is static. You must use dynamic
> > initialization.
> >
> > -Kevin
> >
> > Kevin A. Smith wrote:
> >> That doesn't look thread-safe to me. You'd need to somehow
> >> synchronize access to getLogger() to eliminate a race-condition
> >> on the if(logger == null) test.
> >>
> >> While its a bit more typing, I prefer:
> >>
> >> private static Logger logger = LogFactory.getLogger(MyClass.class);
> >>
> >> or
> >>
> >> private static Logger logger =
> >> LogFactory.getLogger(MyClass.class.getName());
> >>
> >> No threading issues and is pretty easy to read and understand.
> >>
> >> --Kevin
> >>
> >> -----Original Message-----
> >> From: Kevin Ross [mailto:[EMAIL PROTECTED]
> >> Sent: Tuesday, November 19, 2002 2:18 PM
> >> To: Vladimir R. Bossicard; [EMAIL PROTECTED]
> >> Subject: Re: LogFactory.getLogger()
> >>
> >> As I mentioned in my previous message, this is a poor way to
> >> obtain a logger, especially if you have deep inheritance
> >> heirarchies. Since I'm a big fan of OO programming, and my
> >> heirarchies are especially deep, I find the static way of
> >> obtaining a logger invalid.
> >>
> >> Try this:
> >>
> >> public class AbstractLoggable{
> >>
> >> private static transient Logger logger;
> >>
> >> protected Logger getLogger(){
> >>
> >> if( logger == null ){
> >>
> >> logger = Logger.getLogger(getClass());
> >> }
> >>
> >> return logger;
> >> }
> >> }
> >>
> >> whallah! efficient OO programming with an accessor for
> >> subclasses and the proper class name to go with it!
> >>
> >> -Kevin Ross
> >>
> >> Vladimir R. Bossicard wrote:
> >>>>Logger.getLogger(getClass()) at least, whether or not you use the
> >>>> string name is of no consequence.
> >>>
> >>>have you ever tried to call a non-static method within a static
> >>> reference?
> >>>
> >>>if you get this working:
> >>>
> >>> private static Log log = LogFactory.getLog(getClass());
> >>>
> >>>I give you a high-five.
> >>>
> >>>-Vladimir
> >>>
> >>>--
> >>>Vladimir R. Bossicard
> >>>www.bossicard.com
