logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Remko Popma <remko.po...@gmail.com>
Subject Re: LoggerContext locking
Date Tue, 07 Apr 2015 13:36:57 GMT
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.popma@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.goers@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.popma@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.goers@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-unsubscribe@logging.apache.org
>> >> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>> >>
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
>> > For additional commands, e-mail: log4j-dev-help@logging.apache.org
>> >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
>> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>>
>>

Mime
View raw message