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> 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> 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 >> For additional commands, e-mail: log4j-dev-h...@logging.apache.org >> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org > For additional commands, e-mail: log4j-dev-h...@logging.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: log4j-dev-unsubscr...@logging.apache.org For additional commands, e-mail: log4j-dev-h...@logging.apache.org