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

[Daniel Fuchs]

On 6/28/13 9:37 PM, Mandy Chung wrote:
> 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/>
>

Hi Mandy, thanks for the review!

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

Right. I can do that. But I don't see any warnings when compiling
(with make) - maybe they're turned out by default?

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

Well - hmm - I'd prefer to do that in a later cleanup.
I'd want first to analyze all the places where manager is used.
Plus there is Peter's suggestion about changing the dynamics
between LogManager & Logger.
Both are excellent suggestions but I think I'd like to dedicate
a whole changeset for this kind of cleanup, without mixing it
with bug fix.

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

Good catch - I will update the 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.

Well - yes that's partly why I put two tests:

One to verify calling Logger.getGlobal(), one to verify calling
Logger.getLogger(Logger.GLOBAL_LOGGER_NAME).

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

Hmmm... Not sure that's a good idea. I'd prefer leave
TestLogManagerImpl* as simple subclasses of LogManager - but maybe
I should remove the Test prefix - and leave that for actual
test classes.

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

OK

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

OK.

-- daniel
>
> Thanks
> Mandy