RFR: 8139982 Re-examine java.management dependency on java.util.logging.LoggingMXBean

[Daniel Fuchs]

Hi Mandy,

Answers inline, and new webrev at the end.

On 29/04/16 21:55, Mandy Chung wrote:
> Hi Daniel,
>
>> On Apr 29, 2016, at 8:08 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> Hi,
>>
>> Please find below a patch [2] that eliminates a static
>> dependency of java.lang.management on java.util.logging.LoggingMXBean.
>>
>> When JDK-6876135 was fixed, it introduced the PlatformLoggingMXBean
>> interface, and recommended using PlatformLoggingMXBean over
>> LoggingMXBean.
>
> Adding to this, JDK-6876135 prepared the JDK for modularization and PlatformLoggingMXBean was introduced that can be replaced with existing usage of LoggingMXBean.
>
> With this change, java.management dependency on java.logging will become implementation details for logging purpose and can be eliminated completely in the future.
>
> About the deprecation, to be specific, LoggingMXBean will no longer be a platform MXBean and an instance of LoggingMXBean will not register in the platform MBeanServer.
>
> This is a revised wording for the deprecation note for LoggingMXBean:
>
> @deprecated {@code LoggingMXBean} is no longer a {@link java.lang.management.PlatformManagedObject platform MXBean} and replaced with {@link java.lang.management.PlatformLoggingMXBean}.
> It will not register in the platform MBeanServer. Use
> {@code ManagementFactory.getPlatformMXBean(PlatformLoggingMXBean.class)} instead.

Done.

> One question about: ManagementFactory::getPlatformMXBean(MBeanServerConnection, PlatformLoggingMXBean.class)
> - what would happen if this method is called from an image with java.logging module present and connect to
> a VM with no java.logging module?
> Should the ManagementFactory::getPlatformMXBean spec or PlatformLoggingMXBean spec be clarified?

I don't think there's anything to clarify: it will throw
an IllegalArgumentException as specified in the spec.
(it will get an InstanceNotFoundException from the
remote VM, which will be wrapped and thrown as the
cause of an IllegalArgumentException)


> ManagementFactoryHelper.java
>
>  191                 if (result != null) {
>  192                     LoggingMXBeanSupport.class.getModule().addReads(m);
>  193                 }
>
> Reflection access assumes readability now:
>   http://openjdk.java.net/projects/jigsaw/spec/issues/#ReflectionWithoutReadability
 >
 > So this addReads is not needed.

Oh - OK thanks for the pointer!

>
>  252         final Map<String, Method> methods = AccessController.doPrivileged(
>  253             new PrivilegedAction<>() {
>  254                 @Override
>  255                 public Map<String, Method> run() {
>  256                     return initMethodMap(impl);
>  257                 }
>  258             });
>
> I believe this doPrivileged is not necessary and so do the other getMethod calls.
> Probably you are thinking ahead what if java.management may one day be defined
> by a child loader of the defining loader of java.logging.
> Then you can move doPrivileged to initMethodMap.

The doPrivileged around initMethod is necessary because
the implementation class from which the methods are looked
up is package protected, so unfortunately Method.setAccessible
needs to be called. Another possibility would be to lookup
the method on java.util.logging.LoggingMXBean instead of
looking them up on the concrete implementation class.
In that case setAccessible should no longer be needed.

Do you think I should modify the code to do that?

However your remark made me review the doPrivs and I believe
the one in getMXBeanImplementation() is not needed, since
it invokes a public static method on public exported class.
I removed that.


>  217                     // skip
> - better to throw InternalError since it expects all methods are used?

No. getObjectName() will fall in this bucket - it's not
defined on LoggingMXBean.

>  273                 throw new SecurityException(ex);
> - should not reach here.  SecurityException may cause confusion since this will be thrown even without security manager.  could simply throw InternalException

OK - Wrapped in UnsupportedOperationException instead.


>  296         private PlatformLoggingImpl(LoggingMXBeanSupport support) {
> - perhaps renaming LoggingMXBeanSupport to LoggingProxy to better represent it.

It's not really a proxy - it only implemements "invoke". Maybe it should
be called LoggingMXBeanAccess?


>> Backward Compatibility considerations:
>> --------------------------------------
>>
>> 1. Local clients which obtain an instance of the logging
>> MXBean by calling ManagementFactory.getPlatformMXBean(
>>            "java.util.logging:type=Logging",
>>             PlatformLoggingMXBean.class)
>> will no longer be able to cast the result on
>> java.util.logging.LoggingMXBean.
>> [There should be few, given that PlatformLoggingMXBean
>>  already has all the methods defined in LoggingMXBean]
>>
>
> I expect this would be really rare too.
>
>> 2. ManagementFactory.getPlatformMBeanServer().isInstanceOf(
>>     ObjectName, "java.util.logging.LoggingMXBean")
>> will now return 'false' instead of 'true'.
>>
>> 3. The Logging MXBean MBeanInfo will now report that its
>> management interface is "java.lang.management.PlatformLoggingMXBean"
>> instead of "sun.management.ManagementFactoryHelper$LoggingMXBean”.
>
> Any impact to permission confiugred in security policy? Should also document that.

The permission are checked against the concrete implementation
class name "sun.management.ManagementFactoryHelper$PlatformLoggingImpl"
and we're not changing that - so there should be no incompatibility
here.

Thanks a lot for the feedback!

New webrev here:
http://cr.openjdk.java.net/~dfuchs/8139982_webrev/webrev.07/index.html


-- daniel

>> 4. Calls to ManagementFactory.newPlatformMXBeanProxy(
>>    MBeanServerConnection, ObjectName,
>>    java.util.logging.LoggingMXBean.class); and
>> JMX.newMXBeanProxy(MBeanServerConnection, ObjectName,
>>    java.util.logging.LoggingMXBean.class)
>> will continue to work as before.
>>
>
>> 5. Remote clients running previous version of the JDK
>>    should see no changes, except for the interface
>>    name in MBeanInfo, and the change in isInstanceOf
>>    reported in 2.
>
> This is good.
>
> The incompatibility risk for this change is rather low.
>
> Mandy
>