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
