RFR 8026499: Root Logger level can be reset unexpectedly

[Daniel Fuchs]

Hi,

Please find below the new revision:

<http://cr.openjdk.java.net/~dfuchs/webrev_8026499/webrev.01/>

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