I created https://issues.apache.org/jira/browse/LOG4J2-993 for the deadlock issue.
I agree that reconfigure() does not need to be synchronized. Similarly, afaiks onChange() also does not need to be synchronized. As for setConfiguration, locking on a private lock rather than the LoggerContext instance may be better. I noted that the start() methods both use configLock.tryLock(). So if the configLock is already locked, the logic inside these blocks will not execute. For example, if start() is called while a stop() is in progress, the start() method will essentially skip instead of executing after the stop() completes. I am not sure why tryLock() is used and not lock() in the start() methods. I also noted that there is currently a call to setConfiguration from start(Configuration), and this call is outside the configLock block. I am not sure why this is. Is it just to prevent deadlocks (avoid grabbing two locks simultaneously), or should the setConfiguration method execute regardless of whether a stop() is currently in progress? If we use the configLock in setConfiguration, we should probably call configLock.lock() here rather than configLock.tryLock(), would you agree? On Tue, Apr 7, 2015 at 6:15 PM, Remko Popma <remko.po...@gmail.com> wrote: > 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> 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 >> >>
