Actually, I believe there was a case where I did that because the method in the subclass used java.util.concurrent instead of requiring method synchronization.
Ralph On Apr 30, 2014, at 7:19 AM, Remko Popma <remko.po...@gmail.com> wrote: > Yes, overriding a synchronized method with an unsynchronized method sounds > dangerous... Warnings would probably be helpful. > > > On Wed, Apr 30, 2014 at 11:13 PM, Gary Gregory <garydgreg...@gmail.com> wrote: > I'm pretty sure Eclipse does that. > > Gary > > > On Wed, Apr 30, 2014 at 9:50 AM, Matt Sicker <boa...@gmail.com> wrote: > 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 > 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 <boa...@gmail.com> > > > > > -- > Matt Sicker <boa...@gmail.com> > > > > -- > 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 >
