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> 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> 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 | 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
