...and of course, setConfiguration(URI newLocation) should call reconfigure( newLocation) with the specified parameter, instead of the public reconfigure() method.
On Tuesday, April 7, 2015, Remko Popma <remko.po...@gmail.com> wrote: > Looking at the patch I submitted for LOG4J2-207, this is where the > synchronized methods getConfigLocation and setConfigLocation were added. > The patch also made the configLocation field mutable (it was final before). > > I think my reasoning for making them synchronized was that setting the > configLocation field and the subsequent call to reconfigure() should be > atomic. > > I agree that there may be better ways of doing this. One idea: > 1. Make configLocation volatile as Ralph suggested > 2. Remove synchronized keyword from getConfigLocation and setConfigLocation > 3. Add a new private method reconfigure(URI configLocation). This does all > the work of the current reconfigure() method, but uses the specified > configLocation method parameter instead of the field. > 4. Modify the public synchronized method reconfigure() to delegate to the > private method with the configLocation field value. (So this method becomes > a one-liner.) > > This is a cleaner way to preserve the atomicity of the setConfiguration > method without any additional locks. > > Thoughts? > > Sent from my iPhone > > > On 2015/04/07, at 1:02, Ralph Goers <ralph.go...@dslextreme.com > <javascript:;>> wrote: > > > > I’ve been looking at the deadlock that is documented in LOG4J-982. It > seems to me that the problem is that get/setConfigLocation and reconfigure > are locking the whole LoggerContext. First, the history shows that I added > get & setConfigLocation as part of LOG4J2-207. I think Remko actually wrote > the code. I am not sure why these methods need to be synchronized. > Wouldn’t it be enough for configLocation to be volatile? > > > > Second, I don’t think locking the LoggerContext for the entirety a > reconfigure is a good idea. reconfigure calls setConfiguration, which is > also synchronized. It seems to me that while setConfiguration does need to > be locked, it should be using a lock that specifically only prevents > setConfiguration from being executed simultaneously from more than one > thread. > > > > Thoughts? > > > > Ralph > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org > <javascript:;> > > For additional commands, e-mail: log4j-dev-h...@logging.apache.org > <javascript:;> > > >
