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

Reply via email to