For testing, just a thought: what about creating a (test) plugin that during its initialization triggers a reconfiguration in another thread?
The other thread should not be able to complete reconfiguration until the first configuration completes. Not sure if this can be turned into a useful test... On Tuesday, April 7, 2015, Ralph Goers <ralph.go...@dslextreme.com> wrote: > I agree with all of this. However, I believe the reconfigure method does > not need to be synchronized. I think it is OK to construct a new > Configuration while not synchronized. What needs to be locked is the > setConfigurationMethod, which can I believe can be locked using the > configLock, rather than synchronizing the method. > > The problem with this is I am not sure I know of a good way to verify all > of this. The error that is occurring happens because the FlumeAppender is > trying to shutdown its writer threads during a reconfigure and one of those > threads is trying to flush data which apparently is causing a new Logger > to be obtained in the process of doing that. > > I can try to create a test case that emulates this but it may be a bit > tricky. > > Ralph > > > On Apr 6, 2015, at 6:55 PM, Remko Popma <remko.po...@gmail.com > <javascript:;>> 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:;> > >> > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org > <javascript:;> > > For additional commands, e-mail: log4j-dev-h...@logging.apache.org > <javascript:;> > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org > <javascript:;> > For additional commands, e-mail: log4j-dev-h...@logging.apache.org > <javascript:;> > >
