RFR: JDK-7184195 - java.util.logging.Logger.getGlobal().info() doesn't log without configuration

[Mandy Chung]

Hi Daniel,

On 6/19/2013 8:31 AM, Daniel Fuchs wrote:
> The fix proposed is simple. In getGlobal() we check whether
> the 'manager' variable is null - and if it is, we initialize it
> by calling LogManager.getLogManager().
> This is a pattern which is already present at other places in
> the Logger.java class file.
>

This fix is much better.  I am happy that you found this simple approach 
to avoid calling Logger.getLogger from Logger.getGlobal() as the 
LogManager class initialization is fragile and its circular dependency 
with Logger class caused several deadlock issues in the past.

I reviewed the new webrev.01.
> <http://cr.openjdk.java.net/~dfuchs/JDK-7184195/webrev.01/>

Looks good in general.  Some comments:

Logger.global is deprecated.  In LogManager static initializer, we 
should have @SuppressWarnings("deprecation") on Logger.global (probably 
need to define a local variable) to suppress the warning and together 
with your comment it will make it clear that accessing this deprecated 
field is intentional.

As you pointed out, the same pattern is used in the checkPermission() 
method.  Might be worth having a private getManager() method to replace 
direct access to the manager field in the Logger class - just a thought 
to prevent similar bug from happening again.

>
> However - initialization of LogManager has proved to be fragile
> in the past - in particular wrt deadlocks between Logger and
> LogManager caused by circular class initialization.
>

Yes indeed and it's hard to diagnose if something goes wrong.  In the 
readPrimordialConfiguration() method, it silently ignores any exception:

       } catch (Exception ex) {
            // System.err.println("Can't read logging configuration:");
            // ex.printStackTrace();
       }

I have a patch at one point that turns this into an assert so that we 
will diagnose any logging initialization issue easier.  You may want to 
consider adding this in the future.

Great to see the regression tests covering various configurations that 
looks fine. Some suggestions.

I think TestGetGlobal and TestGetGlobal2 are almost the same except 
verifyingLogger.getGlobal()vs
Logger.getLogger(Logger.GLOBAL_LOGGER_NAME)- btw they have the same 
@summary.

Have you considered merging them into one single file and having the 
argument to control which method the test will call?  That will avoid 
the duplicated code.  The downside the number of @run will be double. 
One idea is to move the @run -Djava.util.logging.manager to the 
corresponding LogManager subclass (i.e. TestLogManagerImpl.java will be 
a jtreg test that runs the tests that set itself as the global 
LogManager) that might make it easier to understand what each LogManager 
subclass is about.

You're improving the test coverage for logging which is great. It may be 
good to adopt the current convention e.g. put these tests under 
jdk/test/java/util/logging/Logger/getGlobal directory as they are for 
Logger.getGlobal method.

TestHandles and TestLogManagerImpl are in testgetglobal package and I 
suggest to move them to "testgetglobal" subdirectory.

Thanks
Mandy