Return-Path: X-Original-To: apmail-logging-log4j-dev-archive@www.apache.org Delivered-To: apmail-logging-log4j-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 7E0F617801 for ; Tue, 7 Apr 2015 09:17:56 +0000 (UTC) Received: (qmail 50229 invoked by uid 500); 7 Apr 2015 09:17:56 -0000 Delivered-To: apmail-logging-log4j-dev-archive@logging.apache.org Received: (qmail 50179 invoked by uid 500); 7 Apr 2015 09:17:56 -0000 Mailing-List: contact log4j-dev-help@logging.apache.org; run by ezmlm Precedence: bulk List-Unsubscribe: List-Help: List-Post: List-Id: "Log4J Developers List" Reply-To: "Log4J Developers List" Delivered-To: mailing list log4j-dev@logging.apache.org Received: (qmail 50169 invoked by uid 99); 7 Apr 2015 09:17:56 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 07 Apr 2015 09:17:56 +0000 X-ASF-Spam-Status: No, hits=1.5 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (nike.apache.org: domain of remko.popma@gmail.com designates 209.85.214.175 as permitted sender) Received: from [209.85.214.175] (HELO mail-ob0-f175.google.com) (209.85.214.175) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 07 Apr 2015 09:17:30 +0000 Received: by obbeb7 with SMTP id eb7so7302769obb.3 for ; Tue, 07 Apr 2015 02:15:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=eE7NcYkF33DnOC6+jIWhYgb7czHobLHpFUQPoP9G0+M=; b=wdqKiguD9rgYi2efe+pd27FVPcgWoyCbWzkaHNhXXnMzSjRtsqIg348bO8QnvE3ZRh QBjcQISeE743gZ1n7L9SuO7Nye++iYMymCDLKJpOna8YMy1Skq6YIAe4W2kBlziD92Ag 6EDg+lSaSdt+lNeqPWWnqgXcbTiUzFu+Hn1VoMCb62Eqb+RWkZ+wI0mTz+J9fBOFmvm2 maMFTJs+2k84VqHdcEccOV4EL3Wb3apr5PmgzygaRrUF/UFIvZ1DKZLXkaYydZLDTK7q LLKy0TDsqERhyjbmSmw3lGVlb+eJBuEBehCSdIdFXEvkRagnDet/PzNJiuaHwgtY5PgK wDkw== MIME-Version: 1.0 X-Received: by 10.60.123.100 with SMTP id lz4mr23735588oeb.86.1428398158919; Tue, 07 Apr 2015 02:15:58 -0700 (PDT) Received: by 10.182.79.38 with HTTP; Tue, 7 Apr 2015 02:15:58 -0700 (PDT) In-Reply-To: References: <7262AEF6-CBE1-43F6-A983-10628E0F587B@dslextreme.com> Date: Tue, 7 Apr 2015 18:15:58 +0900 Message-ID: Subject: Re: LoggerContext locking From: Remko Popma To: Log4J Developers List Content-Type: multipart/alternative; boundary=047d7b5d3972dc3dcb05131edcbb X-Virus-Checked: Checked by ClamAV on apache.org --047d7b5d3972dc3dcb05131edcbb Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 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 tho= se > 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 > 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 > wrote: > >> > >> I=E2=80=99ve been looking at the deadlock that is documented in LOG4J-= 982. It > seems to me that the problem is that get/setConfigLocation and reconfigur= e > are locking the whole LoggerContext. First, the history shows that I add= ed > get & setConfigLocation as part of LOG4J2-207. I think Remko actually wro= te > the code. I am not sure why these methods need to be synchronized. > Wouldn=E2=80=99t it be enough for configLocation to be volatile? > >> > >> Second, I don=E2=80=99t think locking the LoggerContext for the entire= ty a > reconfigure is a good idea. reconfigure calls setConfiguration, which is > also synchronized. It seems to me that while setConfiguration does need t= o > 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 > > > --047d7b5d3972dc3dcb05131edcbb Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable For testing, just a thought: what about creating a (test) plugin that durin= g its=C2=A0initialization triggers a reconfiguration in another thread?
The other thread should not be able to complete reconfigura= tion until the first configuration completes.=C2=A0Not sure if this can be = turned into a=C2=A0useful test...

On Tuesday, April 7, = 2015, Ralph Goers <ralph.g= oers@dslextreme.com> wrote:
I agre= e 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 whil= e not synchronized. What needs to be locked is the setConfigurationMethod, = which can I believe can be locked using the configLock, rather than synchro= nizing the method.

The problem with this is I am not sure I know of a good way to verify all o= f this. The error that is occurring happens because the FlumeAppender is tr= ying to shutdown its writer threads during a reconfigure and one of those t= hreads is trying to flush data which apparently is causing=C2=A0 a new Logg= er 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 tric= ky.

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 syn= chronized methods getConfigLocation and setConfigLocation were added. The p= atch 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 at= omic.
>
> 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 setConfigLoc= ation
> 3. Add a new private method reconfigure(URI configLocation). This does= all the work of the current reconfigure() method, but uses the specified c= onfigLocation 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 bec= omes a one-liner.)
>
> This is a cleaner way to preserve the atomicity of the setConfiguratio= n 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=E2=80=99ve been looking at the deadlock that is documented in LO= G4J-982.=C2=A0 It seems to me that the problem is that get/setConfigLocatio= n and reconfigure are locking the whole LoggerContext.=C2=A0 First, the his= tory shows that I added get & setConfigLocation as part of LOG4J2-207. = I think Remko actually wrote the code.=C2=A0 I am not sure why these method= s need to be synchronized. Wouldn=E2=80=99t it be enough for configLocation= to be volatile?
>>
>> Second, I don=E2=80=99t think locking the LoggerContext for the en= tirety a reconfigure is a good idea. reconfigure calls setConfiguration, wh= ich is also synchronized. It seems to me that while setConfiguration does n= eed to be locked, it should be using a lock that specifically only prevents= setConfiguration from being executed simultaneously from more than one thr= ead.
>>
>> Thoughts?
>>
>> Ralph
>> ------------------------------------------------------------------= ---
>> To unsubscribe, e-mail: lo= g4j-dev-unsubscribe@logging.apache.org
>> For additional commands, e-mail: log4j-dev-help@logging.apache.org
>>
>
> ---------------------------------------------------------------------<= br> > To unsubscribe, e-mail: log4j-= dev-unsubscribe@logging.apache.org
> For additional commands, e-mail: log4= j-dev-help@logging.apache.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-u= nsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev= -help@logging.apache.org

--047d7b5d3972dc3dcb05131edcbb--