Agreed that unless the synchronization is part of the public API it is better to lock on an internal object (e.g. private Object lock = new Object(); ).
On the other hand, OutputStreamManager and subclasses have some synchronized public/protected methods and in this case the synchronization on "this" is part of the public API. On Wed, Apr 30, 2014 at 1:24 PM, Matt Sicker <boa...@gmail.com> wrote: > If you want more than one condition monitor, it's also better to use Lock. > I'll definitely agree on the simplicity of synchronizing on an internal > Object instance, though. Locking on this, however, is generally a bad idea > because any other code can synchronize on the object as well by accident > and cause weird problems. > > > On 29 April 2014 20:51, Gary Gregory <garydgreg...@gmail.com> wrote: > >> I like the KISS approach for now as well. >> >> Gary >> >> >> On Tue, Apr 29, 2014 at 9:45 PM, Remko Popma <remko.po...@gmail.com>wrote: >> >>> This should be a separate thread, but anyway... >>> >>> I would oppose using fair locks. Not only will throughput be much less, >>> the benefits are debatable, since fairness of locks does not guarantee >>> fairness of thread scheduling. So we would always pay a price in >>> throughput without any guarantee of upside on the latency variance. Not a >>> good trade-off. >>> >>> The solution here is not using different locking mechanisms but not >>> using locking at all. That is, anyone who is concerned about latency >>> variance (of which starvation is an extreme case) should be using >>> asynchronous loggers. The numbers show that with the disruptor (Async >>> Loggers) you get 3 to 5 *orders of magnitude* less latency. (Yes that means >>> 1000x - 100,000x less latency.) And this is compared to AsyncAppender. >>> Synchronous logging is not even in the picture here. >>> >>> I do agree that synchronizing on a String should be avoided, because >>> Strings can be intern()-ed (and in the ConfigurationFactory case it was a >>> String literal which is intern()ed by default), which means that our lock >>> object is globally visible and other parts of the system could potentially >>> synchronize on it and cause deadlock. That was a nice catch. >>> >>> However, I'm not convinced that java.util.concurrent.Lock is always >>> better than the synchronized keyword. It is a more powerful API, and gives >>> more low-level control, but it comes with more complexity and >>> responsibility, the most obvious one being you need to manually unlock() >>> it. With the synchronized keyword I don't need to worry if I or anyone >>> refactoring the code still correctly unlock()s the lock in a finally >>> clause. It just works. Nice and simple. It is only when I really need the >>> more powerful API that I consider using Locks. For example when using >>> separate read and write locks. Or if I need to use tryLock(). Things that I >>> can't do with the synchronized keyword. Otherwise I prefer to keep it >>> simple. >>> >>> My 2 cents. >>> >>> Remko >>> >>> >>> On Wed, Apr 30, 2014 at 5:45 AM, Matt Sicker <boa...@gmail.com> wrote: >>> >>>> Should we be using a fair lock? That's usually a lot slower than a >>>> typical one, but if it's more proper behavior, then it would make sense to >>>> go that route. >>>> >>>> >>>> On 29 April 2014 14:49, Jörn Huxhorn <jhuxh...@googlemail.com> wrote: >>>> >>>>> Please keep in mind that synchronized is not fair. >>>>> >>>>> http://sourceforge.net/apps/trac/lilith/wiki/SynchronizedVsFairLock >>>>> >>>>> Yes, a fair ReentrantLock is way slower than an unfair one… but if >>>>> starvation is caused by a logging framework then this is a serious issue >>>>> in >>>>> my opinion. >>>>> >>>>> Joern >>>>> >>>>> On 29. April 2014 at 01:05:26, Matt Sicker (boa...@gmail.com) wrote: >>>>> > I'll delegate my arguments to the SO post about it: >>>>> > >>>>> http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java >>>>> > >>>>> > In short, it's defensive programming. It's safer to synchronize on an >>>>> > internal object than on this. Plus, it aids in concurrency throughput >>>>> > instead of just using a synchronized method. >>>>> > >>>>> > >>>>> > On 28 April 2014 12:45, Ralph Goers wrote: >>>>> > >>>>> > > Why are they not appropriate lock objects? Start a discussion >>>>> before just >>>>> > > changing them. >>>>> > > >>>>> > > Ralph >>>>> > > >>>>> > > >>>>> > > >>>>> > > On Apr 28, 2014, at 10:40 AM, Matt Sicker wrote: >>>>> > > >>>>> > > In that case, Item 69: prefer concurrency utilities to wait and >>>>> notify. >>>>> > > Sounds like we can just use a plain Object instance to lock (which >>>>> is >>>>> > > pretty much equivalent to using ReentrantLock) when doing normal >>>>> locks, but >>>>> > > instead of using .notifyAll() and .wait(), we should use the >>>>> Condition >>>>> > > interface (which would require using Lock as well). >>>>> > > >>>>> > > I agree that using synchronized(object) makes sense when it's all >>>>> that's >>>>> > > being done. However, I've been changing instances of >>>>> synchronized(this) and >>>>> > > synchronized(foo) where foo is not an appropriate lock object >>>>> (e.g., a >>>>> > > string, or a non-final object, things like that). >>>>> > > >>>>> > > >>>>> > > On 28 April 2014 12:28, Ralph Goers wrote: >>>>> > > >>>>> > >> Yes, guidelines called out by Effective Java are appropriate when >>>>> they >>>>> > >> apply. >>>>> > >> >>>>> > >> As for concurrency, “New” isn’t always better than old. In a few >>>>> places >>>>> > >> you changed synchronized(object) to use a Lock instead. There is >>>>> little to >>>>> > >> no value in doing that and makes the code look a little more >>>>> cluttered. >>>>> > >> However, if a ReadWriteLock can be used in place of synchronized >>>>> that is a >>>>> > >> real benefit. >>>>> > >> >>>>> > >> The point of the guidelines are that when it comes to stuff like >>>>> this, >>>>> > >> unless there is a guideline written down that says the current >>>>> code is >>>>> > >> wrong discuss it on the list before making a change. >>>>> > >> >>>>> > >> Ralph >>>>> > >> >>>>> > >> On Apr 28, 2014, at 9:35 AM, Matt Sicker wrote: >>>>> > >> >>>>> > >> What about style things covered by Effective Java? These are >>>>> pretty much >>>>> > >> all good ideas. >>>>> > >> >>>>> > >> Also, how about some guidelines on concurrency? I'd recommend not >>>>> using >>>>> > >> the old concurrency stuff and instead using >>>>> java.util.concurrent.* for more >>>>> > >> effective concurrency. This doesn't include the Thread vs >>>>> Runnable thing, >>>>> > >> so that's still debatable. >>>>> > >> >>>>> > >> >>>>> > >> On 28 April 2014 08:46, Gary Gregory wrote: >>>>> > >> >>>>> > >>> Perhaps if one breaks the build, it should be polite to revert >>>>> that last >>>>> > >>> commit... >>>>> > >>> >>>>> > >>> Gary >>>>> > >>> >>>>> > >>> >>>>> > >>> On Mon, Apr 28, 2014 at 3:07 AM, Ralph Goers > >>> > wrote: >>>>> > >>> >>>>> > >>>> I think we need to develop and post some development >>>>> “guidelines”, >>>>> > >>>> “best practices” or whatever you want to call it for Log4j 2. >>>>> Here are >>>>> > >>>> some of the things I would definitely want on it. >>>>> > >>>> >>>>> > >>>> 1. Error on the side of caution. If you don’t understand it, >>>>> don’t >>>>> > >>>> touch it and ask on the list. If you think you understand it >>>>> read it again >>>>> > >>>> or ask until you are sure you do. Nobody will blame you for >>>>> asking >>>>> > >>>> questions. >>>>> > >>>> 2. Don’t break the build - if there is the slightest chance the >>>>> change >>>>> > >>>> you are making could cause unit test failures, run all unit >>>>> tests. Better >>>>> > >>>> yet, get in the habit of always running the unit tests before >>>>> doing the >>>>> > >>>> commit. >>>>> > >>>> 3. If the build breaks and you have made recent changes then >>>>> assume you >>>>> > >>>> broke it and try to fix it. Although it might not have been >>>>> something you >>>>> > >>>> did it will make others feel a lot better than having to fix >>>>> the mistake >>>>> > >>>> for you. Everyone makes mistakes. Taking responsibility for >>>>> them is a good >>>>> > >>>> thing. >>>>> > >>>> 4. Don’t change things to match your personal preference - the >>>>> project >>>>> > >>>> has style guidelines that are validated with checkstyle, PMD, >>>>> and other >>>>> > >>>> tools. If you aren’t fixing a bug, fixing a problem identified >>>>> by the >>>>> > >>>> tools, or fixing something specifically called out in these >>>>> guidelines then >>>>> > >>>> start a discussion to see if the change is something the >>>>> project wants >>>>> > >>>> before starting to work on it. We try to discuss things first >>>>> and then >>>>> > >>>> implement the consensus reached in the discussion. >>>>> > >>>> >>>>> > >>>> Of course, the actual coding conventions we follow should also >>>>> be >>>>> > >>>> spelled out, such as indentation, braces style, import ordering >>>>> and where >>>>> > >>>> to use the final keyword. >>>>> > >>>> >>>>> > >>>> Ralph >>>>> > >>>> >>>>> --------------------------------------------------------------------- >>>>> > >>>> To unsubscribe, e-mail: >>>>> log4j-dev-unsubscr...@logging.apache.org >>>>> > >>>> For additional commands, e-mail: >>>>> log4j-dev-h...@logging.apache.org >>>>> > >>>> >>>>> > >>>> >>>>> > >>> >>>>> > >>> >>>>> > >>> -- >>>>> > >>> E-Mail: garydgreg...@gmail.com | ggreg...@apache.org >>>>> > >>> Java Persistence with Hibernate, Second Edition >>>>> > >>> JUnit in Action, Second Edition >>>>> > >>> Spring Batch in Action >>>>> > >>> Blog: http://garygregory.wordpress.com >>>>> > >>> Home: http://garygregory.com/ >>>>> > >>> Tweet! http://twitter.com/GaryGregory >>>>> > >>> >>>>> > >> >>>>> > >> >>>>> > >> >>>>> > >> -- >>>>> > >> Matt Sicker >>>>> > >> >>>>> > >> >>>>> > >> >>>>> > > >>>>> > > >>>>> > > -- >>>>> > > Matt Sicker >>>>> > > >>>>> > > >>>>> > > >>>>> > >>>>> > >>>>> > -- >>>>> > Matt Sicker >>>>> > >>>>> >>>>> >>>>> --------------------------------------------------------------------- >>>>> To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org >>>>> For additional commands, e-mail: log4j-dev-h...@logging.apache.org >>>>> >>>>> >>>> >>>> >>>> -- >>>> Matt Sicker <boa...@gmail.com> >>>> >>> >>> >> >> >> -- >> E-Mail: garydgreg...@gmail.com | ggreg...@apache.org >> Java Persistence with Hibernate, Second >> Edition<http://www.manning.com/bauer3/> >> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> >> Spring Batch in Action <http://www.manning.com/templier/> >> Blog: http://garygregory.wordpress.com >> Home: http://garygregory.com/ >> Tweet! http://twitter.com/GaryGregory >> > > > > -- > Matt Sicker <boa...@gmail.com> >
