Note that I haven't addressed Ralph's concern on what the best locking mechanism is for reconfigure/setConfiguration...
On Tuesday, April 7, 2015, Gary Gregory <garydgreg...@gmail.com> wrote: > I think you should give it a go. Some more tests too perhaps? > > Gary > > On Mon, Apr 6, 2015 at 7:02 PM, Remko Popma <remko.po...@gmail.com > <javascript:_e(%7B%7D,'cvml','remko.po...@gmail.com');>> wrote: > >> ...and of course, setConfiguration(URI newLocation) should call >> reconfigure(newLocation) with the specified parameter, instead of the >> public reconfigure() method. >> >> >> On Tuesday, April 7, 2015, Remko Popma <remko.po...@gmail.com >> <javascript:_e(%7B%7D,'cvml','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 >>> > >>> >> > > > -- > E-Mail: garydgreg...@gmail.com > <javascript:_e(%7B%7D,'cvml','garydgreg...@gmail.com');> | ggreg...@apache.org > <javascript:_e(%7B%7D,'cvml','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 >
