RFR: 8023168 - Cleanup LogManager class initialization and LogManager/LoggerContext relationship

[David M. Lloyd]

On 09/05/2013 02:45 AM, Daniel Fuchs wrote:
> Hi Mandy,
>
> On 9/5/13 2:46 AM, Mandy Chung wrote:
>>
>> On 9/4/2013 12:56 PM, Daniel Fuchs wrote:
>>> Hi,
>>>
>>> Please find below a changeset that will fix
>>>
>>> 8023168: Cleanup LogManager class initialization and
>>>          LogManager/LoggerContext relationship.
>>>
>>> <http://cr.openjdk.java.net/~dfuchs/webrev_8023168/webrev.00/>
>>>
>>
>> It is good to see the LogManager/Logger initialization untangled. This
>> helps in the long run.
>>
>> LogManager.java L342: thanks to adding the assert here. L340-341 can
>> be removed.
>> This will be helpful in debugging (rather than swallowing the
>> exception).  The initialization has been so fragile and swallowing the
>> exception makes the problem even worse.  With this cleanup, I think
>> the initialization code will be more robust.
> Will do.
>>
>> The ensureLogManagerInitialized method does the rest of the
>> initialization for the global log manager - it reads the
>> configuration, instantiates and registers the root logger, and also
>> register the global logger.  The comment L289-292 about deadlock
>> between two class initializers needs to be revise.
> Will do.
>>
>> I agree with David that it's worth exploring if lazy holder class
>> idiom would help here - perhaps a holder class with a static
>> rootLogger field.
> Well, I am not too keen on using a lazy holder class idiom here.
> Though it is true that ensureLogManagerInitialized will do nothing if
> called before the 'manager' field
> is set, or if called from additional instances of LogManager, I'd prefer
> not to stick this code in yet
> another static initializer.

I do understand the sentiment - static initializers got us into this 
mess after all! - however I don't think it's a rational one.  Lazy 
holder works well for this because it would only happen once and block 
progress until it is done.  That said, using a volatile+second check 
under lock is also a reasonable approach (deliberately avoiding the 
poisonous "double-checked" phrase).  However I don't think you need 
multiple flags; it seems to me that the lock would only be released once 
initialization was finished, so as long as you only set the flag at the 
end of processing you'd be sure to be consistent (unless you don't want 
second callers to block, which might be a reasonable requirement, but I 
didn't get that out of my reading of the code).

> If that's acceptable - I'd prefer to use a
> synchronized block within the
> method and an additional volatile boolean to mark when initialization is
> finished.
>
> This should make it possible to avoid entering the synchronized block
> once the LogManager
> is initialized.
>
>>
>> Since you touch these lines, it's good to switch to try-with-resource.
>>
>> 1205         final InputStream in = new FileInputStream(fname);
>> 1206         final BufferedInputStream bin = new BufferedInputStream(in);
>> 1207         try {
>> 1208             readConfiguration(bin);
>> 1209         } finally {
>> 1210             in.close();
>> 1211         }
>>
> OK.
>
>> Logger.java - getGlobal() method: would it help if the holder class
>> has static Logger global field which is set during its class
>> initialization.  It should guarantee that global.manager is set
>> and the log manager initialization completes??
>
> Can't do. Logger.global is a public final field.
>
> -- daniel
>
>>
>> I was wondering if this can be simplified.
>>
>> thanks
>> Mandy
>>
>


-- 
- DML