Fixes issue #24849 make relevant methods synchronized in LocalTxConnectionEventListener

appserver/connectors/connectors-runtime/src/main/java/com/sun/enterprise/resource/ConnectorXAResource.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, 2022 Contributors to the Eclipse Foundation
+ * Copyright (c) 2021, 2024 Contributors to the Eclipse Foundation
  * Copyright (c) 1997, 2020 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
@@ -21,7 +21,7 @@
 import static java.util.logging.Level.SEVERE;
 
 import java.util.Map;
-import java.util.Set;
+import java.util.Map.Entry;
 import java.util.logging.Logger;
 
 import javax.transaction.xa.XAException;
@@ -46,8 +46,12 @@
  */
 public class ConnectorXAResource implements XAResource {
 
-    static Logger _logger = LogDomains.getLogger(ConnectorXAResource.class, LogDomains.RSR_LOGGER);
+    private static Logger _logger = LogDomains.getLogger(ConnectorXAResource.class, LogDomains.RSR_LOGGER);
 
+    /**
+     * userHandle meaning: an object representing the "connection handle for the underlying physical connection". In some
+     * code also named connectionHandle.
+     */
     private Object userHandle;
     private ResourceSpec spec;
     private ClientSecurityInfo info;
@@ -120,7 +124,9 @@ public void end(Xid xid, int flags) throws XAException {
                 if (handle != null) { // not needed, just to be sure.
                     ManagedConnection associatedConnection = (ManagedConnection) handle.getResource();
                     associatedConnection.associateConnection(userHandle);
-                    _logger.log(FINE, "connection_sharing_reset_association", userHandle);
+                    if (_logger.isLoggable(FINE)) {
+                        _logger.log(FINE, "connection_sharing_reset_association", userHandle);
+                    }
                 }
             }
         } catch (Exception e) {
@@ -227,25 +233,18 @@ private JavaEETransaction getCurrentTransaction() throws SystemException {
     private void resetAssociation() throws XAException {
         try {
             ResourceHandle handle = getResourceHandle();
-
-            LocalTxConnectionEventListener listerner = (LocalTxConnectionEventListener) handle.getListener();
-            // Get all associated Handles and reset their ManagedConnection association.
-            Map associatedHandles = listerner.getAssociatedHandles();
-            if (associatedHandles != null) {
-                Set<Map.Entry> userHandles = associatedHandles.entrySet();
-                for (Map.Entry userHandleEntry : userHandles) {
-                    ResourceHandle associatedHandle = (ResourceHandle) userHandleEntry.getValue();
-                    ManagedConnection associatedConnection = (ManagedConnection) associatedHandle.getResource();
-                    associatedConnection.associateConnection(userHandleEntry.getKey());
-                    if (_logger.isLoggable(FINE)) {
-                        _logger.log(FINE, "connection_sharing_reset_association", userHandleEntry.getKey());
-                    }
+            LocalTxConnectionEventListener listener = (LocalTxConnectionEventListener) handle.getListener();
+
+            // Clear the associations and Map all associated handles back to their actual Managed Connection.
+            Map<Object, ResourceHandle> associatedHandles = listener.getAssociatedHandlesAndClearMap();
+            for (Entry<Object, ResourceHandle> userHandleEntry : associatedHandles.entrySet()) {
+                ResourceHandle associatedHandle = (ResourceHandle) userHandleEntry.getValue();
+                ManagedConnection associatedConnection = (ManagedConnection) associatedHandle.getResource();
+                associatedConnection.associateConnection(userHandleEntry.getKey());
+                if (_logger.isLoggable(FINE)) {
+                    _logger.log(FINE, "connection_sharing_reset_association", userHandleEntry.getKey());
                 }
-
-                // All associated handles are mapped back to their actual Managed Connection. Clear the associations.
-                associatedHandles.clear();
             }
-
         } catch (Exception ex) {
             handleResourceException(ex);
         }

appserver/connectors/connectors-runtime/src/main/java/com/sun/enterprise/resource/ResourceHandle.java

@@ -52,7 +52,7 @@ public class ResourceHandle implements com.sun.appserv.connectors.internal.api.R
 
     private final long id;
     private final ClientSecurityInfo info;
-    private final Object resource; // represents MC
+    private final Object resource; // represents ManagedConnection
     private ResourceSpec spec;
     private XAResource xaRes;
     private Object userConnection; // represents connection-handle to user

appserver/connectors/connectors-runtime/src/main/java/com/sun/enterprise/resource/listener/LocalTxConnectionEventListener.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022 Contributors to the Eclipse Foundation
+ * Copyright (c) 2022, 2024 Contributors to the Eclipse Foundation
  * Copyright (c) 1997, 2020 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
@@ -32,37 +32,51 @@
  */
 public class LocalTxConnectionEventListener extends ConnectionEventListener {
 
-    private PoolManager poolManager;
+    /**
+     * A shortcut to the singleton PoolManager instance. Field could also be removed.
+     */
+    private final PoolManager poolManager = ConnectorRuntime.getRuntime().getPoolManager();
 
-    // connectionHandle -> ResourceHandle
-    // Whenever a connection is associated with a ManagedConnection,
-    // that connection and the resourcehandle associated with its
-    // original ManagedConnection will be put in this table.
-    private IdentityHashMap associatedHandles;
+    /**
+     * Map to store the relation: "userHandle/connectionHandle -> ResourceHandle" using reference-equality. Whenever a
+     * connection is associated with a ManagedConnection, that connection and the resourceHandle associated with its
+     * original ManagedConnection will be put in this table.
+     * <p>
+     * userHandle meaning: an object representing the "connection handle for the underlying physical connection". In some
+     * code also named connectionHandle.
+     * <p>
+     * All code altering associatedHandles must be synchronized.
+     */
+    private final IdentityHashMap<Object, ResourceHandle> associatedHandles = new IdentityHashMap<>(10);
 
-    private ResourceHandle resource;
+    /**
+     * The original resource for which this listener is created.
+     */
+    private final ResourceHandle resource;
 
     public LocalTxConnectionEventListener(ResourceHandle resource) {
         this.resource = resource;
-        this.associatedHandles = new IdentityHashMap(10);
-        this.poolManager = ConnectorRuntime.getRuntime().getPoolManager();
     }
 
     @Override
-    public void connectionClosed(ConnectionEvent evt) {
+    public synchronized void connectionClosed(ConnectionEvent evt) {
         Object connectionHandle = evt.getConnectionHandle();
         ResourceHandle handle = resource;
         if (associatedHandles.containsKey(connectionHandle)) {
-            handle = (ResourceHandle) associatedHandles.get(connectionHandle);
+            handle = associatedHandles.get(connectionHandle);
         }
+        // ManagedConnection instance is still valid and put back in the pool: do not remove the event listener.
         poolManager.resourceClosed(handle);
     }
 
     @Override
-    public void connectionErrorOccurred(ConnectionEvent evt) {
+    public synchronized void connectionErrorOccurred(ConnectionEvent evt) {
         resource.setConnectionErrorOccurred();
+
+        // ManagedConnection instance is now invalid and unusable. Remove this event listener.
         ManagedConnection mc = (ManagedConnection) evt.getSource();
         mc.removeConnectionEventListener(this);
+
         poolManager.resourceErrorOccurred(resource);
     }
 
@@ -72,12 +86,15 @@ public void connectionErrorOccurred(ConnectionEvent evt) {
      * @param evt ConnectionEvent
      */
     @Override
-    public void badConnectionClosed(ConnectionEvent evt) {
+    public synchronized void badConnectionClosed(ConnectionEvent evt) {
         Object connectionHandle = evt.getConnectionHandle();
         ResourceHandle handle = resource;
         if (associatedHandles.containsKey(connectionHandle)) {
-            handle = (ResourceHandle) associatedHandles.get(connectionHandle);
+            handle = associatedHandles.get(connectionHandle);
         }
+
+        // TODO: Explain why event listener needs to be removed.
+        // There is no documentation mentioning: ManagedConnection instance is now invalid and unusable.
         ManagedConnection mc = (ManagedConnection) evt.getSource();
         mc.removeConnectionEventListener(this);
 
@@ -99,16 +116,38 @@ public void localTransactionRolledback(ConnectionEvent evt) {
         // no-op
     }
 
-    public void associateHandle(Object c, ResourceHandle h) {
-        associatedHandles.put(c, h);
+    /**
+     * Associate the given userHandle to the resourceHandle.
+     *
+     * @param userHandle the userHandle object to be associated with the new handle
+     * @param resourceHandle the original Handle
+     */
+    public synchronized void associateHandle(Object userHandle, ResourceHandle resourceHandle) {
+        associatedHandles.put(userHandle, resourceHandle);
     }
 
-    public ResourceHandle removeAssociation(Object c) {
-        return (ResourceHandle) associatedHandles.remove(c);
+    /**
+     * Removes the Map entory for the given userHandle key.
+     *
+     * @param userHandle The userHandle key to be removed from the map.
+     * @return the associated ResourceHandle that is removed from the map or null if no association was found. A null return
+     * can also indicate that the map previously associated null with userHandle.
+     */
+    public synchronized ResourceHandle removeAssociation(Object userHandle) {
+        return associatedHandles.remove(userHandle);
     }
 
-    public Map getAssociatedHandles() {
-        return associatedHandles;
-    }
+    /**
+     * Returns a clone of the whole associatedHandles map and clears the map in the listener.
+     * @return The clone of the associatedHandles map.
+     */
+    public synchronized Map<Object, ResourceHandle> getAssociatedHandlesAndClearMap() {
+        // Clone the associatedHandles, because we will clear the list in this method
+        IdentityHashMap<Object, ResourceHandle> result = (IdentityHashMap<Object, ResourceHandle>) associatedHandles.clone();
 
+        // Clear the associatedHandles
+        associatedHandles.clear();
+
+        return result;
+    }
 }

appserver/connectors/connectors-runtime/src/test/java/com/sun/enterprise/resource/listener/LocalTxConnectionEventListenerTest.java

@@ -0,0 +1,87 @@
+/*
+ * Copyright (c) 2024 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
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the
+ * Eclipse Public License v. 2.0 are satisfied: GNU General Public License,
+ * version 2 with the GNU Classpath Exception, which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ */
+
+package com.sun.enterprise.resource.listener;
+
+import static org.easymock.EasyMock.createNiceMock;
+import static org.easymock.EasyMock.replay;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import java.util.Map;
+
+import org.glassfish.api.naming.SimpleJndiName;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import com.sun.appserv.connectors.internal.api.PoolingException;
+import com.sun.enterprise.connectors.ConnectorRuntime;
+import com.sun.enterprise.resource.ResourceHandle;
+import com.sun.enterprise.resource.ResourceSpec;
+
+import jakarta.resource.ResourceException;
+import jakarta.resource.spi.ManagedConnection;
+
+public class LocalTxConnectionEventListenerTest {
+
+    @BeforeEach
+    public void setup() throws PoolingException, ResourceException {
+        // Make sure ConnectorRuntime singleton is initialized
+        new ConnectorRuntime();
+    }
+
+    @Test
+    public void associateHandleTest() throws ResourceException {
+        ResourceHandle mainResourceHandle = createResourceHandle(1);
+        LocalTxConnectionEventListener localTxConnectionEventListener = new LocalTxConnectionEventListener(mainResourceHandle);
+
+        // Associate a new handle association
+        ResourceHandle associatedResourceHandle = createResourceHandle(2);
+        final Object userHandle = new Object();
+        localTxConnectionEventListener.associateHandle(userHandle, associatedResourceHandle);
+
+        // Remove the new handle association
+        ResourceHandle removeAssociation = localTxConnectionEventListener.removeAssociation(userHandle);
+        assertEquals(associatedResourceHandle, removeAssociation);
+
+        // Check the remove did work in the previous call
+        removeAssociation = localTxConnectionEventListener.removeAssociation(userHandle);
+        assertNull(removeAssociation);
+    }
+
+    @Test
+    public void getAssociatedHandlesAndClearMapTest() throws ResourceException {
+        ResourceHandle mainResourceHandle = createResourceHandle(1);
+        LocalTxConnectionEventListener localTxConnectionEventListener = new LocalTxConnectionEventListener(mainResourceHandle);
+
+        localTxConnectionEventListener.associateHandle(new Object(), createResourceHandle(2));
+        localTxConnectionEventListener.associateHandle(new Object(), createResourceHandle(3));
+
+        // Check the clone works
+        Map<Object, ResourceHandle> associatedHandlesAndClearMap = localTxConnectionEventListener.getAssociatedHandlesAndClearMap();
+        assertEquals(2, associatedHandlesAndClearMap.size());
+
+        // Check the clear did work in the previous call
+        associatedHandlesAndClearMap = localTxConnectionEventListener.getAssociatedHandlesAndClearMap();
+        assertEquals(0, associatedHandlesAndClearMap.size());
+    }
+
+    private ResourceHandle createResourceHandle(int i) throws ResourceException {
+        ManagedConnection managedConnection = createNiceMock(ManagedConnection.class);
+        replay();
+        return new ResourceHandle(managedConnection, new ResourceSpec(new SimpleJndiName("testResource" + i), 0), null, null);
+    }
+}