A quick note that this was fixed for 2.3. Gary
On Tue, Apr 7, 2015 at 6:36 AM, Remko Popma <remko.po...@gmail.com> wrote: > 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 >>> >>> > -- 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
