There are legitimate reasons to do that as long as you maintain thread-safety such as the case Ralph mentioned. Though that would probably require full control of the API from top to bottom-ish.
On 30 April 2014 10:22, Ralph Goers <ralph.go...@dslextreme.com> wrote: > 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<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> >>> >> >> >> >> -- >> 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>
