RFR 8026499: Root Logger level can be reset unexpectedly

[Daniel Fuchs]

On 10/17/13 7:16 PM, Mandy Chung wrote:
> 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. I will do that before pushing.

-- daniel

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