RFR 8026499: Root Logger level can be reset unexpectedly

[Mandy Chung]

On 10/16/13 5:35 AM, Daniel Fuchs wrote:
> Hi,
>
> Please find below the new revision:
>
> <http://cr.openjdk.java.net/~dfuchs/webrev_8026499/webrev.01/>
>

Looks good.

As for the test, LoggingSupport.getLogger is just a layer to support the 
PlatformLogger be called in the absence of java.util.logging for 
modularization and not intended to be used by anyone.  I suggest to take 
out line 99-100 to create a platform logger via LoggingSupport.  Calling 
PlatformLogger.getLogger in the test is adequate to cause the root 
logger be added in the system context.

thanks
Mandy

> - added comment in LogManager
> - renamed hasLocalLevel() into isLevelInitialized()
> - fixed the test, including Logger.global, with and
>    without SecurityManager
>
> -- daniel
>
> On 10/16/13 11:47 AM, Daniel Fuchs wrote:
>> Hi Mandy,
>>
>> On 10/16/13 12:58 AM, Mandy Chung wrote:
>>>
>>> On 10/15/2013 9:48 AM, Daniel Fuchs wrote:
>>>> Hi,
>>>>
>>>> Please find below a fix for:
>>>>
>>>>     8026499: Root Logger level can be reset unexpectedly
>>>>     https://bugs.openjdk.java.net/browse/JDK-8026499
>>>>
>>>> <http://cr.openjdk.java.net/~dfuchs/webrev_8026499/webrev.00/>
>>>>
>>>> The issue here is that calling a method like e.g. URL.openConnection()
>>>> will trigger the initialization of some platform logger, which could
>>>> in turn cause the root logger to be lazilly added to the system 
>>>> context,
>>>> which in turn causes the root logger's level to be reset with the
>>>> value found in the configuration file (INFO).
>>>>
>>>> The fix is to not reinitialize the value of the logger's level if
>>>> it already has been initialized.
>>>
>>> The change looks okay.  When the root logger is added to the system
>>> context, it considers it as a newly created logger and thus resetting
>>> its level.   The global logger should have the same issue as there is
>>> only a single instance.   I believe the logger's handler doesn't get
>>> reset and level is the only issue.
>>
>> That was my analysis too :-)
>>
>>>
>>> Logger.hasLocalLevel() doesn't seem to be necessary and it seems to be
>>> more explicit to replace !logger.hasLocalLevel() with
>>>      logger.getLevel() == null
>>
>> I considered that but rejected it because getLevel() can be overridden,
>> and what I really want to know is whether setLevel() has been called.
>>
>> The alternative would have been to declare a package
>> 'setLevelIfAbsent()' method (but it would need to be invoked
>> in doPrivileged) or to add a package getLevelObject() method,
>> (which looked weird) or to expose levelObject - which I didn't want
>> to do.
>>
>> Hence the hasLocalLevel() which I don't like very much but like better
>> than the alternatives ;-)
>>
>>
>>> It would also help to add a comment to explain the check.
>>
>> Right.
>>
>> // Do not reset the logger level if it has already been initialized
>>
>>
>>> TestRootLoggerLevel.java - it may be good to also test the global 
>>> logger.
>>
>> Good point.
>>
>>> line 70: I believe this line is not necessary (do you mean to do any
>>> other checking and that's why you call LoggingSupport.getLogger?)
>>
>> In fact either line 70-71 or line 73-74 are enough to trigger the issue.
>> What really triggers the issue however is the underlying call to
>> LogManager.demandSystemLogger(). I just put both for good measure ;-)
>>
>>
>>> I think the comment on the behavior before the fix (line 77-78) can be
>>> removed and this may become not as relevant after this bug fix. Line 
>>> 104
>>> - this permission may be needed in your early version of this test?  
>>> can
>>> be removed now?  btw - should this test also run with no security
>>> manager?
>>
>> Yes and yes.
>>
>> Thanks for the review!
>>
>>>
>>> Mandy
>>>
>>>
>>
>