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