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

[Mandy Chung]

> On May 2, 2016, at 7:13 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
> 
> 
>> 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)
> 

OK.  thanks for checking.

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

I think that’d be cleaner to inspect java.util.logging.LoggingMXBean instead as Logging class now implements it.
  
> 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.
> 

Good.

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

I missed it inspected implementation class rather than LoggingMXBean.  Once you change it to find methods on LoggingMXBean, it should expect all methods can be found.
 
> 
>> 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?
> 

Sounds good.

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

Good.

> Thanks a lot for the feedback!
> 
> New webrev here:
> http://cr.openjdk.java.net/~dfuchs/8139982_webrev/webrev.07/index.html

Nit: in the class spec for LoggingMXBean.java - wrap type names with {@code…} such as MBeanServer, PlatformLoggingMXBean.

ManagementFactoryHelper.java
line 333: extra line to be removed?

Mandy