Question about locking in LoggerContext.start()

[Matt Sicker <bo...@gmail.com>]

Alright, I'm back to working on my proposed refactoring for LoggerContext
initialization and I've come across an issue. What I wanted to do was
extract a method object from the bodies of the start() and stop() methods.
However, calling out to foreign code like that in a synchronized context is
a great way to cause concurrency bugs.

Basically, what I'd like to know is if it's necessary to add the shutdown
hook thread during a configLock. This doesn't really seem to require
locking the configuration since a configuration change won't have an effect
on said shutdown thread.

In a related issue, why is setConfiguration locking on this instead of
configLock? Or really any of the synchronized methods in LoggerContext.

And with the status state switching logic, in start(), the reconfigure()
call is done before status switches to STARTED. However, in
start(Configuration), the call to setConfiguration() happens _after_ it's
in the STARTED state (and outside the configLock section).

What I'd like to propose here is that we separate out the shutdown hook
logic into an unsynchronized context as it doesn't really require
synchronization. I also think we should take a closer look at the use of
locking in LoggerContext. With the configLock object, do we really need to
use synchronized methods?

-- 
Matt Sicker <bo...@gmail.com>

Re: Question about locking in LoggerContext.start()

[Ralph Goers <rg...@apache.org>]

I am afraid you are going to have to wait a couple of days until I have time to look at the code again.  I do know that particular part of the code was modified a few times and is fairly sensitive so I'm guessing there are good reasons for everything.

Ralph

> On Apr 2, 2014, at 6:49 PM, Matt Sicker <bo...@gmail.com> wrote:
> 
> Alright, I'm back to working on my proposed refactoring for LoggerContext initialization and I've come across an issue. What I wanted to do was extract a method object from the bodies of the start() and stop() methods. However, calling out to foreign code like that in a synchronized context is a great way to cause concurrency bugs.
> 
> Basically, what I'd like to know is if it's necessary to add the shutdown hook thread during a configLock. This doesn't really seem to require locking the configuration since a configuration change won't have an effect on said shutdown thread.
> 
> In a related issue, why is setConfiguration locking on this instead of configLock? Or really any of the synchronized methods in LoggerContext.
> 
> And with the status state switching logic, in start(), the reconfigure() call is done before status switches to STARTED. However, in start(Configuration), the call to setConfiguration() happens _after_ it's in the STARTED state (and outside the configLock section).
> 
> What I'd like to propose here is that we separate out the shutdown hook logic into an unsynchronized context as it doesn't really require synchronization. I also think we should take a closer look at the use of locking in LoggerContext. With the configLock object, do we really need to use synchronized methods?
> 
> -- 
> Matt Sicker <bo...@gmail.com>