Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixed cyclic dependency between api and impl forbidden by JPMS #56

Merged
merged 4 commits into from
Feb 25, 2025

Conversation

dmatej
Copy link
Contributor

@dmatej dmatej commented Feb 24, 2025

  • Fixed cyclic dependency between api and impl using ServiceLoader instead of reflection and hardcoded class name
    • GenericConstructor used reflection to get impl from the api module.
    • It is possible to use alternatives now, however I don't believe somebody would try that.
    • Caused failures in jdbc_group3 tests of GlassFish in combination with swallowing exceptions; fortunately they were logged as FINEST at least.
  • Deleted unused field in AMXClientTest - initialization broke tests

Note: I will restage 4.1.0 after merge.

EDIT: I have found also the stacktrace from GlassFish tests:

[2025-02-23T22:11:43.549488+01:00] [GF 7.0.23-SNAPSHOT] [FINE] [] [org.glassfish.gmbal.util] [tid: _ThreadID=126 _ThreadName=RMI TCP Connection(4)-127.0.0.1] [levelValue: 500] [[
  Failure in getConstructor
java.lang.ClassNotFoundException: org.glassfish.gmbal.impl.ManagedObjectManagerImpl not found by org.glassfish.gmbal.gmbal-api-only [38]
        at org.apache.felix.framework.BundleWiringImpl.findClassOrResourceByDelegation(BundleWiringImpl.java:1591)
        at org.apache.felix.framework.BundleWiringImpl.access$300(BundleWiringImpl.java:79)
        at org.apache.felix.framework.BundleWiringImpl$BundleClassLoader.loadClass(BundleWiringImpl.java:1976)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
        at java.base/java.lang.Class.forName0(Native Method)
        at java.base/java.lang.Class.forName(Class.java:375)
        at org.glassfish.gmbal.util.GenericConstructor.getConstructor(GenericConstructor.java:58)
        at org.glassfish.gmbal.util.GenericConstructor.create(GenericConstructor.java:90)
        at org.glassfish.gmbal.ManagedObjectManagerFactory.createFederated(ManagedObjectManagerFactory.java:97)
        at org.glassfish.admin.monitor.StatsProviderManagerDelegateImpl.registerGmbal(StatsProviderManagerDelegateImpl.java:719)
        at org.glassfish.admin.monitor.StatsProviderManagerDelegateImpl.mbeanRegistered(StatsProviderManagerDelegateImpl.java:703)
        at org.glassfish.external.amx.MBeanListener.handleNotification(MBeanListener.java:303)
        at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor$ListenerWrapper.handleNotification(DefaultMBeanServerInterceptor.java:1716)
        at java.management/javax.management.NotificationBroadcasterSupport.handleNotification(NotificationBroadcasterSupport.java:275)
        at java.management/javax.management.NotificationBroadcasterSupport$SendNotifJob.run(NotificationBroadcasterSupport.java:352)
        at java.management/javax.management.NotificationBroadcasterSupport$1.execute(NotificationBroadcasterSupport.java:337)
        at java.management/javax.management.NotificationBroadcasterSupport.sendNotification(NotificationBroadcasterSupport.java:248)
        at java.management/javax.management.MBeanServerDelegate.sendNotification(MBeanServerDelegate.java:211)
        at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.sendNotification(DefaultMBeanServerInterceptor.java:1478)
        at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.registerWithRepository(DefaultMBeanServerInterceptor.java:1877)
        at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.registerDynamicMBean(DefaultMBeanServerInterceptor.java:960)
        at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.registerObject(DefaultMBeanServerInterceptor.java:895)
        at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.registerMBean(DefaultMBeanServerInterceptor.java:320)
        at java.management/com.sun.jmx.mbeanserver.JmxMBeanServer.registerMBean(JmxMBeanServer.java:523)
        at com.sun.enterprise.v3.admin.DynamicInterceptor.registerMBean(DynamicInterceptor.java:396)
        at org.glassfish.admin.amx.impl.AMXStartupService.loadDomainRoot(AMXStartupService.java:235)
        at org.glassfish.admin.amx.impl.AMXStartupService._loadAMXMBeans(AMXStartupService.java:318)
        at org.glassfish.admin.amx.impl.AMXStartupService.loadAMXMBeans(AMXStartupService.java:215)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:569)
        at sun.reflect.misc.Trampoline.invoke(MethodUtil.java:72)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:569)
        at java.base/sun.reflect.misc.MethodUtil.invoke(MethodUtil.java:262)
        at java.management/com.sun.jmx.mbeanserver.StandardMBeanIntrospector.invokeM2(StandardMBeanIntrospector.java:112)
        at java.management/com.sun.jmx.mbeanserver.StandardMBeanIntrospector.invokeM2(StandardMBeanIntrospector.java:46)
        at java.management/com.sun.jmx.mbeanserver.MBeanIntrospector.invokeM(MBeanIntrospector.java:237)
        at java.management/com.sun.jmx.mbeanserver.PerInterface.invoke(PerInterface.java:138)
        at java.management/com.sun.jmx.mbeanserver.MBeanSupport.invoke(MBeanSupport.java:252)
        at java.management/javax.management.StandardMBean.invoke(StandardMBean.java:405)
        at java.management/com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.invoke(DefaultMBeanServerInterceptor.java:814)
        at java.management/com.sun.jmx.mbeanserver.JmxMBeanServer.invoke(JmxMBeanServer.java:802)
        at com.sun.enterprise.v3.admin.DynamicInterceptor.invoke(DynamicInterceptor.java:298)
        at org.glassfish.admin.mbeanserver.BootAMX.bootAMX(BootAMX.java:124)
        at org.glassfish.admin.mbeanserver.BootAMXListener.handleNotification(BootAMXListener.java:64)
        at java.management/javax.management.NotificationBroadcasterSupport.handleNotification(NotificationBroadcasterSupport.java:275)
        at java.management/javax.management.NotificationBroadcasterSupport$SendNotifJob.run(NotificationBroadcasterSupport.java:352)
        at java.management/javax.management.NotificationBroadcasterSupport$1.execute(NotificationBroadcasterSupport.java:337)
        at java.management/javax.management.NotificationBroadcasterSupport.sendNotification(NotificationBroadcasterSupport.java:248)
        at java.management/javax.management.remote.JMXConnectorServer.sendNotification(JMXConnectorServer.java:322)
        at java.management/javax.management.remote.JMXConnectorServer.connectionOpened(JMXConnectorServer.java:239)
        at java.management.rmi/javax.management.remote.rmi.RMIConnectorServer.connectionOpened(RMIConnectorServer.java:658)
        at java.management.rmi/javax.management.remote.rmi.RMIServerImpl.doNewClient(RMIServerImpl.java:257)
        at java.management.rmi/javax.management.remote.rmi.RMIServerImpl.newClient(RMIServerImpl.java:198)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:569)
        at java.rmi/sun.rmi.server.UnicastServerRef.dispatch(UnicastServerRef.java:360)
        at java.rmi/sun.rmi.transport.Transport$1.run(Transport.java:200)
        at java.rmi/sun.rmi.transport.Transport$1.run(Transport.java:197)
        at java.base/java.security.AccessController.doPrivileged(AccessController.java:712)
        at java.rmi/sun.rmi.transport.Transport.serviceCall(Transport.java:196)
        at java.rmi/sun.rmi.transport.tcp.TCPTransport.handleMessages(TCPTransport.java:587)
        at java.rmi/sun.rmi.transport.tcp.TCPTransport$ConnectionHandler.run0(TCPTransport.java:828)
        at java.rmi/sun.rmi.transport.tcp.TCPTransport$ConnectionHandler.lambda$run$0(TCPTransport.java:705)
        at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
        at java.rmi/sun.rmi.transport.tcp.TCPTransport$ConnectionHandler.run(TCPTransport.java:704)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.base/java.lang.Thread.run(Thread.java:840)
]]

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- GenericConstructor used reflection to get impl from the api module.
- It is possible to use alternatives now, however I don't believe somebody
  would try that.

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
@dmatej dmatej added the bug Something isn't working label Feb 24, 2025
@dmatej dmatej added this to the 4.1.0 milestone Feb 24, 2025
@dmatej dmatej requested review from pzygielo and a team February 24, 2025 22:09
@dmatej dmatej self-assigned this Feb 24, 2025
@@ -46,8 +46,6 @@
<configuration>
<instructions>
<Bundle-SymbolicName>${project.groupId}.${project.artifactId}</Bundle-SymbolicName>
<Export-Package>org.glassfish.gmbal,org.glassfish.pfl.tf.timer.spi</Export-Package>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed any more, felix see it very clear now

@@ -11,8 +11,12 @@

module org.glassfish.gmbal.api {
requires java.logging;
requires transitive org.glassfish.external.management.api;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transitive is hiding information, it is better to have explicit list of my own dependencies.

requires org.glassfish.pfl.tf;

exports org.glassfish.gmbal;

uses org.glassfish.gmbal.ManagedObjectManagerService;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declaration of the service extension point; with JPMS then you can provide its implementation. Using the jar without JPMS we still need the service file as specified in older Java versions.

*/
public final class ManagedObjectManagerFactory {
private ManagedObjectManagerFactory() {}

private static GenericConstructor<ManagedObjectManager> objectNameCons =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GenericConstructor class was very toxic hack how to mask out the cyclic dependency between API and IMPL. Api still required the impl, which was wrong, but the compiler did not see it through reflection.

}
}
} ) ;
} catch (Exception exc) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When GenericConstructor failed, it just logged the failure, yet just as FINE log, so in GlassFish you had no idea what happened, just that ORB doesn't work.


requires org.glassfish.gmbal.api;
requires org.glassfish.pfl.basic;
requires org.glassfish.pfl.tf;

exports org.glassfish.gmbal.impl;
exports org.glassfish.gmbal.typelib;

provides org.glassfish.gmbal.ManagedObjectManagerService with org.glassfish.gmbal.impl.ManagedObjectManagerServiceImpl;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now IMPL provides the service for the API.

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
@dmatej dmatej merged commit 53ece0d into eclipse-ee4j:master Feb 25, 2025
2 checks passed
@dmatej dmatej deleted the jpms branch February 25, 2025 14:32
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants