Thanks! Closed. Sent from my iPhone
> On 2015/05/21, at 4:28, Gary Gregory <garydgreg...@gmail.com> wrote: > > 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 > JUnit in Action, Second Edition > Spring Batch in Action > Blog: http://garygregory.wordpress.com > Home: http://garygregory.com/ > Tweet! http://twitter.com/GaryGregory
