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

Mandy Chung mandy.chung at oracle.com
Fri Jun 28 19:37:39 UTC 2013

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

More information about the core-libs-dev mailing list