Fix leak in the events listener storage

Author: arjantijms

nucleus/core/kernel/src/main/java/org/glassfish/kernel/event/EventsImpl.java

@@ -17,15 +17,14 @@
 
 package org.glassfish.kernel.event;
 
-import java.lang.reflect.Method;
 import java.util.Arrays;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentSkipListMap;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.atomic.AtomicLong;
-import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import org.glassfish.api.event.EventListener;
@@ -37,6 +36,9 @@
 import org.glassfish.hk2.api.PostConstruct;
 import org.glassfish.kernel.KernelLoggerInfo;
 
+import static java.util.logging.Level.SEVERE;
+import static java.util.logging.Level.WARNING;
+
 /**
  * Simple implementation of the events dispatching facility.
  *
@@ -51,7 +53,8 @@ public class EventsImpl implements Events, PostConstruct {
     /**
      * Use skip list based map to preserve listeners in registration order.
      */
-    final Map<Listener, EventMatcher> listeners = new ConcurrentSkipListMap<>();
+    final Map<Listener, EventMatcher> listenersBySequence = new ConcurrentSkipListMap<>();
+    final Map<EventListener, Listener> listenersByIdentity = new ConcurrentHashMap<>();
 
     private ExecutorService executor;
 
@@ -70,22 +73,43 @@ public void postConstruct() {
     @Override
     public void register(EventListener listener) {
         try {
-            Method eventMethod = listener.getClass().getMethod("event", Event.class);
-            RestrictTo[] restrictTo = eventMethod.getParameters()[0].getAnnotationsByType(RestrictTo.class);
-            EventTypes<?>[] eventTypes = Arrays.stream(restrictTo)
-                .map(restrict -> EventTypes.create(restrict.value())).toArray(EventTypes[]::new);
-            listeners.putIfAbsent(new Listener(listener, sequenceGenerator.getAndIncrement()), new EventMatcher(eventTypes));
+            RestrictTo[] restrictTo =
+                    listener.getClass()
+                            .getMethod("event", Event.class)
+                            .getParameters()[0]
+                            .getAnnotationsByType(RestrictTo.class);
+
+            listenersByIdentity.computeIfAbsent(listener, e -> {
+
+                Listener listenerWrapperWithSeq = new Listener(listener, sequenceGenerator.getAndIncrement());
+
+                EventMatcher eventMatcher = new EventMatcher(
+                    Arrays.stream(restrictTo)
+                          .map(restrict -> EventTypes.create(restrict.value()))
+                          .toArray(EventTypes[]::new));
+
+                listenersBySequence.put(
+                    listenerWrapperWithSeq,
+                    eventMatcher);
+
+                return listenerWrapperWithSeq;
+            });
+
+
         } catch (Throwable t) {
-            // We need to catch Throwable, otherwise we can server not to
-            // shutdown when the following happens:
+            // We need to catch Throwable, otherwise we can not
+            // shutdown the server when the following happens:
+            //
             // Assume a bundle which has registered an event listener
             // has been uninstalled without unregistering the listener.
+            //
             // listener.getClass() refers to a class of such an uninstalled
-            // bundle. If framework has been refreshed, then the
+            // bundle. If the framework has been refreshed, then the
             // classloader can't be used further to load any classes.
+            //
             // As a result, an exception like NoClassDefFoundError is thrown
             // from getMethod.
-            LOG.log(Level.SEVERE, KernelLoggerInfo.exceptionRegisterEventListener, t);
+            LOG.log(SEVERE, KernelLoggerInfo.exceptionRegisterEventListener, t);
         }
     }
 
@@ -96,28 +120,32 @@ public void send(final Event<?> event) {
 
     @Override
     public void send(final Event<?> event, boolean asynchronously) {
-        for (Map.Entry<Listener, EventMatcher> entry : listeners.entrySet()) {
-            EventMatcher matcher = entry.getValue();
+        for (var entry : listenersBySequence.entrySet()) {
+
             // Check if the listener is interested with his event.
-            if (matcher.matches(event)) {
+            if (entry.getValue().matches(event)) {
+
                 Listener listener = entry.getKey();
+
                 if (asynchronously) {
+                    // Process Async
                     executor.submit(() -> {
                         try {
                             listener.event(event);
                         } catch (Throwable t) {
-                            LOG.log(Level.WARNING, KernelLoggerInfo.exceptionDispatchEvent, t);
+                            LOG.log(WARNING, KernelLoggerInfo.exceptionDispatchEvent, t);
                         }
                     });
                 } else {
+                    // Process sync
                     try {
                         listener.event(event);
                     } catch (DeploymentException e) {
                         // When synchronous listener throws DeploymentException
                         // we re-throw the exception to abort the deployment
                         throw e;
                     } catch (Throwable t) {
-                        LOG.log(Level.WARNING, KernelLoggerInfo.exceptionDispatchEvent, t);
+                        LOG.log(WARNING, KernelLoggerInfo.exceptionDispatchEvent, t);
                     }
                 }
             }
@@ -126,7 +154,12 @@ public void send(final Event<?> event, boolean asynchronously) {
 
     @Override
     public boolean unregister(EventListener listener) {
-        return listeners.remove(new Listener(listener)) != null;
+        Listener listenerWrapperWithSeq = listenersByIdentity.remove(listener);
+        if (listenerWrapperWithSeq == null) {
+            return false;
+        }
+
+        return listenersBySequence.remove(listenerWrapperWithSeq) != null;
     }
 
     /**
@@ -158,23 +191,26 @@ EventListener unwrap() {
 
         @Override
         public boolean equals(Object obj) {
-            if (obj == this) {
-                return true;
-            }
-            if (!(obj instanceof Listener)) {
-                return false;
-            }
-            return eventListener.equals(((Listener) obj).eventListener);
+            return
+                obj instanceof Listener listener &&
+                this.eventListener == listener.eventListener;
         }
 
         @Override
         public int hashCode() {
-            return eventListener.hashCode();
+            return System.identityHashCode(eventListener);
         }
 
         @Override
         public int compareTo(Listener listener) {
-            return Long.compare(sequenceNumber, listener.sequenceNumber);
+            // Treat the same underlying listener as the SAME KEY
+            if (this.eventListener == listener.eventListener) {
+                // Identity
+                return 0;
+            }
+
+            // otherwise keep registration order via sequence
+            return Long.compare(this.sequenceNumber, listener.sequenceNumber);
         }
     }
 

nucleus/core/kernel/src/main/java/org/glassfish/kernel/event/OSGiAwareEventsImpl.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2024 Contributors to the Eclipse Foundation.
+ * Copyright (c) 2024, 2025 Contributors to the Eclipse Foundation.
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Public License v. 2.0, which is available at
@@ -23,7 +23,9 @@
 import org.osgi.framework.BundleContext;
 import org.osgi.framework.FrameworkEvent;
 import org.osgi.framework.FrameworkListener;
-import org.osgi.framework.FrameworkUtil;
+
+import static org.osgi.framework.FrameworkEvent.PACKAGES_REFRESHED;
+import static org.osgi.framework.FrameworkUtil.getBundle;
 
 /**
  * OSGi-aware implementation of the events dispatching facility.
@@ -48,25 +50,33 @@ public void removeFrameworkListener() {
 
     @Override
     public void frameworkEvent(FrameworkEvent frameworkEvent) {
-        if (frameworkEvent.getType() == FrameworkEvent.PACKAGES_REFRESHED) {
+        if (frameworkEvent.getType() == PACKAGES_REFRESHED) {
+
             // Get System Bundle context
             BundleContext bundleContext = frameworkEvent.getBundle().getBundleContext();
+
             // Uninstalled bundles has been removed when framework has been refreshed.
             // Remove listeners registered by this uninstalled bundles.
-            listeners.keySet().removeIf(listener -> {
+            listenersBySequence.keySet().removeIf(listener -> {
+
                 // Get bundle for event listener
-                Bundle bundle = FrameworkUtil.getBundle(listener.unwrap().getClass());
-                if (bundle != null) {
-                    // Even if Bundle has been removed, it must preserve bundle_id.
-                    return bundleContext.getBundle(bundle.getBundleId()) == null;
+                Bundle bundle = getBundle(listener.unwrap().getClass());
+                if (bundle == null) {
+                    return true;
                 }
-                return true;
+
+                // Even if Bundle has been removed, it must preserve bundle_id.
+                return bundleContext.getBundle(bundle.getBundleId()) == null;
             });
         }
     }
 
     private BundleContext getBundleContext() {
-        Bundle bundle = FrameworkUtil.getBundle(getClass());
-        return bundle != null ? bundle.getBundleContext() : null;
+        Bundle bundle = getBundle(getClass());
+        if (bundle == null) {
+            return null;
+        }
+
+        return bundle.getBundleContext();
     }
 }