RFR 8026499: Root Logger level can be reset unexpectedly

[Daniel Fuchs]

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