So it's a good idea to set up your IDE to warn if you override a synchronized method with an unsynchronized method? I remember seeing that one somewhere.
On 30 April 2014 00:35, Remko Popma <remko.po...@gmail.com> wrote: > 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> >> > > -- Matt Sicker <boa...@gmail.com>
