Part 2 for issue eclipse-ee4j#24900 Start with a new PoolManagertImpl…

… unit test to

understand enlisted versus busy states of Resource handles and the
wiring inside a transaction to keep track of all used resources.

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

@@ -234,9 +234,8 @@ public class ConnectorRuntime implements com.sun.appserv.connectors.internal.api
     @Inject
     private Provider<ResourceManager> resourceManagerProvider;
 
-    /* protected for unit test */
     @Inject
-    protected ProcessEnvironment processEnvironment;
+    private ProcessEnvironment processEnvironment;
 
     @Inject
     private DriverLoader driverLoader;

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

@@ -123,6 +123,7 @@ public class ConnectionPool implements ResourcePool, ConnectionLeakListener, Res
      * Default: true
      */
     protected boolean matchConnections;
+
     /**
      * Represents the "is-connection-validation-required" configuration value.<br>
      * Specifies whether connections have to be validated before being given to the application. If a resource’s validation
@@ -353,8 +354,10 @@ protected synchronized void initPool(ResourceAllocator allocator) throws Pooling
 
     /**
      * Schedules the resizer timer task. If a task is currently scheduled, it would be cancelled and a new one is scheduled.
+     * <p>
+     * package protected for unit tests
      */
-    private void scheduleResizerTask() {
+    protected void scheduleResizerTask() {
         if (resizerTask != null) {
             // cancel the current task
             resizerTask.cancel();
@@ -395,7 +398,7 @@ public void addResource(ResourceAllocator alloc) throws PoolingException {
     }
 
     /**
-     * marks resource as free. This method should be used instead of directly calling
+     * Marks resource as free. This method should be used instead of directly calling
      * resoureHandle.getResourceState().setBusy(false) OR getResourceState(resourceHandle).setBusy(false) as this method
      * handles stopping of connection leak tracing If connection leak tracing is enabled, takes care of stopping connection
      * leak tracing
@@ -408,7 +411,7 @@ protected void setResourceStateToFree(ResourceHandle resourceHandle) {
     }
 
     /**
-     * marks resource as busy. This method should be used instead of directly calling
+     * Marks resource as busy. This method should be used instead of directly calling
      * resoureHandle.getResourceState().setBusy(true) OR getResourceState(resourceHandle).setBusy(true) as this method
      * handles starting of connection leak tracing If connection leak tracing is enabled, takes care of starting connection
      * leak tracing
@@ -421,10 +424,10 @@ protected void setResourceStateToBusy(ResourceHandle resourceHandle) {
     }
 
     /**
-     * returns resource from the pool.
+     * Returns a resource from the connection pool.
      *
      * @return a free pooled resource object matching the ResourceSpec
-     * @throws PoolingException - if any error occurrs - or the pool has reached its max size and the
+     * @throws PoolingException - if any error occurs - or the pool has reached its max size and the
      * max-connection-wait-time-in-millis has expired.
      */
     @Override
@@ -674,6 +677,11 @@ private ResourceHandle getResourceFromTransaction(Transaction transaction, Resou
                                     }
                                 }
 
+                                // TODO: This 'if (state.isFree())' logic suggests the state can already be Busy.
+                                // Document why it can already be busy.
+                                // Is this because you can have a transaction within a transaction and reuse the same resource?
+                                // But in that case: shouldn't the state be already set to 'busy' by the previous code
+                                // in the same thread and still remain busy?
                                 if (state.isFree()) {
                                     setResourceStateToBusy(resourceHandle);
                                 }
@@ -759,6 +767,12 @@ protected boolean isConnectionValid(ResourceHandle resourceHandle, ResourceAlloc
      * @return boolean representing the match status of the connection
      */
     protected boolean matchConnection(ResourceHandle resource, ResourceAllocator alloc) {
+        // TODO: Explain what matching is in detail.
+        // TODO: Explain that if matchConnections is disabled in the connectionpool why 'true' is still returned and not false?!
+        // Old documentation mentions: "match-connections / default: true / If true, enables connection matching. You
+        // can set to false if connections are homogeneous." Jakarta documentation:
+        // jakarta.resource.spi.ManagedConnectionFactory.matchManagedConnections(Set, Subject, ConnectionRequestInfo) mentions:
+        // "criteria used for matching is specific to a resource adapter and is not prescribed by the Connector specification."
         boolean matched = true;
         if (matchConnections) {
             matched = alloc.matchConnection(resource);
@@ -835,9 +849,10 @@ protected ResourceHandle getResourceFromPool(ResourceAllocator resourceAllocator
             }
 
             if (resourceFromPool != null) {
-                // Set correct state
+                // Set state to Busy
                 setResourceStateToBusy(resourceFromPool);
             } else {
+                // Set state to Busy via resizePoolAndGetNewResource call
                 resourceFromPool = resizePoolAndGetNewResource(resourceAllocator);
             }
         } finally {
@@ -905,7 +920,10 @@ private ResourceHandle getMatchedResourceFromPool(ResourceAllocator alloc) {
             while ((handle = dataStructure.getResource()) != null) {
                 if (matchConnection(handle, alloc)) {
                     matchedResourceFromPool = handle;
+                    // TODO: ensure the state is not already isBusy here
                     setResourceStateToBusy(matchedResourceFromPool);
+
+                    // Break from the while loop and do not add the handle to the activeResources list.
                     break;
                 }
                 activeResources.add(handle);
@@ -915,6 +933,7 @@ private ResourceHandle getMatchedResourceFromPool(ResourceAllocator alloc) {
             for (ResourceHandle activeResource : activeResources) {
                 dataStructure.returnResource(activeResource);
             }
+            // No need to clear the list, clear() call is probably here to try to help the garbage collector.
             activeResources.clear();
         }
 
@@ -1057,9 +1076,6 @@ public void deleteResource(ResourceHandle resourceHandle) {
         }
     }
 
-    /**
-     * this method is called to indicate that the resource is not used by a bean/application anymore
-     */
     @Override
     public void resourceClosed(ResourceHandle handle) throws IllegalStateException {
         LOG.log(FINE, "Resource was closed, processing handle: {0}", handle);
@@ -1164,6 +1180,8 @@ protected boolean cleanupResource(ResourceHandle handle) {
     public void resourceErrorOccurred(ResourceHandle resourceHandle) throws IllegalStateException {
         LOG.log(FINE, "Resource error occured: {0}", resourceHandle);
         if (failAllConnections) {
+            // TODO: leakDetector is not updated and isBusy state of this resource is not updated correctly: possible bug.
+            // leakDetector should be updated in the doFailAllConnectionsProcessing method. The resource can be updated here.
             doFailAllConnectionsProcessing();
             return;
         }
@@ -1182,7 +1200,8 @@ public void resourceErrorOccurred(ResourceHandle resourceHandle) throws IllegalS
             throw new IllegalStateException();
         }
 
-        // mark as not busy
+        // Mark as not busy. Even if it is removed from the Pool datastructure,
+        // it is good to clean it up, at least to clean up the leakDetector.
         setResourceStateToFree(resourceHandle);
         state.touchTimestamp();
 
@@ -1213,6 +1232,9 @@ private void doFailAllConnectionsProcessing() {
         }
 
         emptyPool();
+        // TODO: leakDetector might have been used and it is not cleaned up.
+        // should call leakDetector.reset
+
         try {
             createResources(allocator, steadyPoolSize);
             LOG.log(FINE, "Successfully created new resources.");
@@ -1224,7 +1246,7 @@ private void doFailAllConnectionsProcessing() {
     }
 
     /**
-     * this method is called when a resource is enlisted in
+     * This method is called when a resource is enlisted in a transaction.
      *
      * @param tran Transaction
      * @param resource ResourceHandle
@@ -1235,18 +1257,28 @@ public void resourceEnlisted(Transaction tran, ResourceHandle resource) throws I
     }
 
     /**
-     * this method is called when transaction tran is completed
+     * This method is called when transaction tran is completed.
      *
      * @param tran Transaction
      * @param status status of transaction
      */
     @Override
     public void transactionCompleted(Transaction tran, int status) throws IllegalStateException {
+        // transactionCompleted will update all relevant resource handles to be no longer enlisted
         List<ResourceHandle> delistedResources = poolTxHelper.transactionCompleted(tran, status, poolInfo);
+
         for (ResourceHandle resource : delistedResources) {
-            // Application might not have closed the connection.
+            // Application might not have closed the connection, free the resource only if it is not in use anymore.
             if (isResourceUnused(resource)) {
                 freeResource(resource);
+                // Resource is now returned to the pool and another thread can use it, cannot log it anymore.
+            } else {
+                // TODO: Why would the application not close a busy connection if the transaction completed and the resource handle is
+                // delisted from the transaction? Is this done to leave the resource as used in the connection pool and let it
+                // time out and be cleaned up by the timer?
+                // The poolTxHelper.transactionCompleted already altered the enlisted state to be no longer enlisted in any transaction.
+                // So this resource is no longer part of an outer transaction for example.
+                // Would expect a warning here in case the resource handle state still marked as busy.
             }
         }
     }

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

@@ -120,20 +120,21 @@ public void createEmptyConnectionPool(PoolInfo poolInfo, PoolType pooltype, Hash
 
     /**
      * Create and initialize pool if not created already.
+     * <p>
+     * package default for unit test
      *
-     * @param poolInfo Name of the pool to be created
-     * @param poolType - PoolType
-     * @return ResourcePool - newly created pool
+     * @param poolInfo pool identifier of the pool to be created
+     * @param poolType the type of pool to be created
+     * @param env the jndi information to find the ConnectorConnectionPool configuration used to configure the pool
      * @throws PoolingException when unable to create/initialize pool
      */
-    private ResourcePool createAndInitPool(final PoolInfo poolInfo, PoolType poolType, Hashtable env) throws PoolingException {
+    void createAndInitPool(final PoolInfo poolInfo, PoolType poolType, Hashtable env) throws PoolingException {
         ResourcePool pool = getPool(poolInfo);
         if (pool == null) {
             pool = ResourcePoolFactoryImpl.newInstance(poolInfo, poolType, env);
             addPool(pool);
             LOG.log(INFO, "Created connection pool and added it to PoolManager: {0}", pool);
         }
-        return pool;
     }
 
     // invoked by DataSource objects to obtain a connection
@@ -241,7 +242,8 @@ public boolean switchOnMatching(PoolInfo poolInfo) {
         return true;
     }
 
-    private void addPool(ResourcePool pool) {
+    /* package protected for unit test */
+    protected void addPool(ResourcePool pool) {
         LOG.log(FINE, "Adding pool {0} to pooltable", pool.getPoolInfo());
         poolTable.put(pool.getPoolInfo(), pool);
     }
@@ -380,6 +382,7 @@ public void putbackBadResourceToPool(ResourceHandle resourceHandle) {
             ResourcePool pool = poolTable.get(poolInfo);
             if (pool != null) {
                 synchronized (pool) {
+                    // TODO: why is resourceClosed called AND resourceErrorOccurred?
                     pool.resourceClosed(resourceHandle);
                     resourceHandle.setConnectionErrorOccurred();
                     pool.resourceErrorOccurred(resourceHandle);
@@ -396,6 +399,8 @@ public void putbackResourceToPool(ResourceHandle resourceHandle, boolean errorOc
             ResourcePool pool = poolTable.get(poolInfo);
             if (pool != null) {
                 if (errorOccurred) {
+                    // TODO: this code is different from putbackBadResourceToPool logic, explain why
+                    // TODO: why is resourceHandle.setConnectionErrorOccurred(); not called?
                     pool.resourceErrorOccurred(resourceHandle);
                 } else {
                     pool.resourceClosed(resourceHandle);

appserver/connectors/connectors-runtime/src/test/java/com/sun/enterprise/resource/pool/ConnectionPoolTest.java

@@ -68,8 +68,6 @@
 import com.sun.enterprise.resource.pool.datastructure.DataStructure;
 import com.sun.enterprise.transaction.api.JavaEETransaction;
 import com.sun.logging.LogDomains;
-
-import jakarta.resource.ResourceException;
 import jakarta.resource.spi.ManagedConnection;
 import jakarta.resource.spi.ManagedConnectionFactory;
 import jakarta.resource.spi.RetryableUnavailableException;
@@ -94,7 +92,7 @@ public class ConnectionPoolTest {
     // they can probably also be made as a unit test here / or in a similar unit test
 
     @BeforeEach
-    public void createAndPopulateMocks() throws PoolingException, ResourceException {
+    public void createAndPopulateMocks() throws Exception {
         List<Object> mocks = new ArrayList<>();
 
         // Mock ManagedConnection
@@ -124,18 +122,18 @@ public void createAndPopulateMocks() throws PoolingException, ResourceException
 
         // Make sure ConnectorRuntime singleton is initialized
         MyConnectorRuntime connectorRuntime = new MyConnectorRuntime();
-        ProcessEnvironment processEnvironment = new ProcessEnvironment();
-        connectorRuntime.setProcessEnvironment(processEnvironment);
         connectorRuntime.postConstruct();
     }
 
     private void createConnectionPool(int maxPoolSize, int maxWaitTimeInMillis, int poolResizeQuantity) throws PoolingException {
         PoolInfo poolInfo = ConnectionPoolTest.getPoolInfo();
-        MyConnectionPool.myMaxPoolSize = maxPoolSize;
-        MyConnectionPool.maxWaitTimeInMillis = maxWaitTimeInMillis;
-        MyConnectionPool.poolResizeQuantity = poolResizeQuantity;
 
-        connectionPool = new MyConnectionPool(poolInfo);
+        Hashtable<Object, Object> env = new Hashtable<>();
+        env.put("maxPoolSize", Integer.valueOf(maxPoolSize));
+        env.put("maxWaitTimeInMillis", Integer.valueOf(maxWaitTimeInMillis));
+        env.put("poolResizeQuantity", Integer.valueOf(poolResizeQuantity));
+
+        connectionPool = new MyConnectionPool(poolInfo, env);
         assertEquals(0, connectionPool.getSteadyPoolSize());
         assertEquals(maxPoolSize, connectionPool.getMaxPoolSize());
 
@@ -187,7 +185,7 @@ void basicConnectionPoolTest() throws Exception {
 
         // Test issue #24843: make the state of resource1 not busy anymore (it should not happen but it happens in rare cases),
         // resource should still be closed without throwing an exception.
-        resource1.getResourceState().setBusy(false);
+        connectionPool.setResourceStateToFree(resource1);
         connectionPool.resourceClosed(resource1);
         assertResourceIsNotBusy(resource1);
 
@@ -450,12 +448,8 @@ private void cleanupConnectionPool() {
 
     public static class MyConnectionPool extends ConnectionPool {
 
-        public static int myMaxPoolSize;
-        public static int maxWaitTimeInMillis;
-        public static int poolResizeQuantity;
-
-        public MyConnectionPool(PoolInfo poolInfo) throws PoolingException {
-            super(ConnectionPoolTest.getPoolInfo(), new Hashtable<>());
+        public MyConnectionPool(PoolInfo poolInfo, Hashtable env) throws PoolingException {
+            super(ConnectionPoolTest.getPoolInfo(), env);
         }
 
         @Override
@@ -465,6 +459,13 @@ protected ConnectorConnectionPool getPoolConfigurationFromJndi(Hashtable env) th
             ConnectorConnectionPool connectorConnectionPool = ConnectionPoolObjectsUtils
                     .createDefaultConnectorPoolObject(poolInfo, null);
 
+            int myMaxPoolSize = (int) env.get("maxPoolSize");
+            int maxWaitTimeInMillis = (int) env.get("maxWaitTimeInMillis");
+            int poolResizeQuantity = (int) env.get("poolResizeQuantity");
+
+            assertTrue(myMaxPoolSize > 0);
+            assertTrue(poolResizeQuantity > 0);
+
             // Override some defaults
             connectorConnectionPool.setSteadyPoolSize("0");
             connectorConnectionPool.setMaxPoolSize("" + myMaxPoolSize);
@@ -473,12 +474,18 @@ protected ConnectorConnectionPool getPoolConfigurationFromJndi(Hashtable env) th
 
             return connectorConnectionPool;
         }
+
+        protected void scheduleResizerTask() {
+            // Do not schedule any resize tasks
+        }
     }
 
     public class MyConnectorRuntime extends ConnectorRuntime {
+        private ProcessEnvironment processEnvironment = new ProcessEnvironment();
 
-        public void setProcessEnvironment(ProcessEnvironment processEnvironment) {
-            this.processEnvironment = processEnvironment;
+        public MyConnectorRuntime() throws Exception {
+            // Force 'injection' of private field processEnvironment
+            InjectionUtil.injectPrivateField(ConnectorRuntime.class, this, "processEnvironment", processEnvironment);
         }
 
         @Override
@@ -498,7 +505,7 @@ public DelegatingClassLoader getConnectorClassLoader() {
         }
     }
 
-    private static PoolInfo getPoolInfo() {
+    public static PoolInfo getPoolInfo() {
         SimpleJndiName jndiName = new SimpleJndiName("myPool");
         return new PoolInfo(jndiName);
     }

appserver/connectors/connectors-runtime/src/test/java/com/sun/enterprise/resource/pool/InjectionUtil.java

@@ -0,0 +1,37 @@
+/*
+ * Copyright (c) 2024 Eclipse Foundation and/or its affiliates. All rights reserved.
+ *
+ * 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.pool;
+
+import java.lang.reflect.Field;
+
+public class InjectionUtil {
+
+    /**
+     * Use injection to fill in private fields in (final) classes. E.g. fields annotated with jakarta.inject.Inject.
+     *
+     * @param clazz the class to be altered
+     * @param clazzInstance the instance of the class that needs to be altered
+     * @param fieldName the name of the field in the class
+     * @param fieldValue the new value for the field
+     * @throws Exception if the injection of the value failed
+     */
+    public static void injectPrivateField(Class<?> clazz, Object clazzInstance, String fieldName, Object fieldValue) throws Exception {
+        Field declaredField = clazz.getDeclaredField(fieldName);
+        declaredField.setAccessible(true);
+        declaredField.set(clazzInstance, fieldValue);
+    }
+}