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 >
