-Kevin Ross
Kevin Smith wrote:
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 myexperience, 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
