Part 3 for issue eclipse-ee4j#24900 More documentation and more unit …

…test coverage.

appserver/connectors/connectors-internal-api/src/main/java/com/sun/appserv/connectors/internal/api/TransactedPoolManager.java

@@ -1,4 +1,5 @@
 /*
+ * Copyright (c) 2024 Eclipse Foundation and/or its affiliates. All rights reserved.
  * Copyright (c) 1997, 2020 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
@@ -35,22 +36,24 @@ public interface TransactedPoolManager {
      * pool manager.
      *
      * @param tran Transaction to which the resource is enlisted
-     * @param res Resource that is enlisted
+     * @param resource Resource that is enlisted
      * @throws IllegalStateException when unable to enlist the resource
      */
-    void resourceEnlisted(Transaction tran, ResourceHandle res) throws IllegalStateException;
+    void resourceEnlisted(Transaction tran, ResourceHandle resource) throws IllegalStateException;
 
     /**
-     * registers the provided resource with the component & enlists the resource in the transaction
-     * @param handle resource-handle
+     * Registers the provided resource with the component & enlists the resource in the transaction
+     *
+     * @param resource Resource to be registered.
      * @throws PoolingException when unable to register the resource
      */
-    void registerResource(ResourceHandle handle) throws PoolingException;
+    void registerResource(ResourceHandle resource) throws PoolingException;
 
     /**
-     * unregisters the resource from the component and delists the resource from the transaction
-     * @param resource resource-handle
-     * @param xaresFlag
+     * Unregisters the resource from the component and delists the resource from the transaction
+     *
+     * @param resource Resource to be unregistered.
+     * @param xaresFlag flag indicating transaction success. This can be XAResource.TMSUCCESS or XAResource.TMFAIL
      */
     void unregisterResource(ResourceHandle resource, int xaresFlag);
 }

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

@@ -34,8 +34,17 @@ public final class ConnectionPoolReconfigHelper {
         LogDomains.RSR_LOGGER);
 
     public enum ReconfigAction {
+        /**
+         * Recreate connection pool
+         */
         RECREATE_POOL,
+        /**
+         * Update ManagedConnectionFactory and attributes
+         */
         UPDATE_MCF_AND_ATTRIBUTES,
+        /**
+         * No operation
+         */
         NO_OP
     }
 

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

@@ -54,6 +54,12 @@ public interface ResourceAllocator {
 
     boolean isTransactional();
 
+    /**
+     * Forces the cleanup of the ManagedConnection associated to the given resource.
+     *
+     * @param resource the resource referencing a ManagedConnection
+     * @throws PoolingException in case the cleanup failed
+     */
     void cleanup(ResourceHandle resource) throws PoolingException;
 
     boolean shareableWithinComponent();

appserver/connectors/connectors-runtime/src/main/java/com/sun/enterprise/resource/pool/AbstractPoolManager.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, 2018 Oracle and/or its affiliates. All rights reserved.
  *
  * This program and the accompanying materials are made available under the
@@ -17,33 +17,31 @@
 
 package com.sun.enterprise.resource.pool;
 
-import java.util.concurrent.ConcurrentHashMap;
-
-import org.glassfish.resourcebase.resources.api.PoolInfo;
-
-import com.sun.enterprise.resource.ResourceSpec;
-
 /**
  * Abstract Pool manager for unimplemented features. Throws UnsupportedOperationException when invoked.
  */
 public abstract class AbstractPoolManager implements PoolManager {
 
-    @Override
-    public void emptyResourcePool(ResourceSpec spec) {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public void killAllPools() {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public void setSelfManaged(PoolInfo poolInfo, boolean flag) {
-        throw new UnsupportedOperationException();
-    }
-
-    public ConcurrentHashMap getMonitoredPoolTable() {
-        throw new UnsupportedOperationException();
-    }
+// Not used in the code
+//    @Override
+//    public void emptyResourcePool(ResourceSpec spec) {
+//        throw new UnsupportedOperationException();
+//    }
+
+// Not used in the code
+//    @Override
+//    public void killAllPools() {
+//        throw new UnsupportedOperationException();
+//    }
+
+// Not used in the code
+//    @Override
+//    public void setSelfManaged(PoolInfo poolInfo, boolean flag) {
+//        throw new UnsupportedOperationException();
+//    }
+
+// Not used in the code
+//    public ConcurrentHashMap getMonitoredPoolTable() {
+//        throw new UnsupportedOperationException();
+//    }
 }

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

@@ -379,12 +379,12 @@ protected Resizer initializeResizer() {
     }
 
     /**
-     * add a resource with status busy and not enlisted
+     * Add a resource to the pool with status busy and not enlisted.
      *
-     * @param alloc ResourceAllocator
+     * @param alloc the ResourceAllocator to be used
      * @throws PoolingException when unable to add a resource
      */
-    public void addResource(ResourceAllocator alloc) throws PoolingException {
+    private void addResource(ResourceAllocator alloc) throws PoolingException {
         int numResCreated = dataStructure.addResource(alloc, 1);
         if (numResCreated > 0) {
             for (int i = 0; i < numResCreated; i++) {
@@ -568,7 +568,7 @@ public ResourceHandle getResource(ResourceSpec spec, ResourceAllocator alloc, Tr
      * Overridden in AssocWithThreadResourcePool to fetch the resource cached in the ThreadLocal In ConnectionPool this
      * simply returns null.
      *
-     * @param spec ResourceSpec
+     * @param spec the ResourceSpec used to locate the correct resource pool
      * @param alloc ResourceAllocator to create a resource
      * @param tran Transaction
      * @return ResourceHandle resource from ThreadLocal
@@ -613,7 +613,7 @@ protected ResourceHandle internalGetResource(ResourceSpec resourceSpec, Resource
      *
      * @param transaction Current Transaction
      * @param resourceAllocator ResourceAllocator
-     * @param resourceSpec ResourceSpec
+     * @param resourceSpec the ResourceSpec used to locate the correct resource pool
      * @return result ResourceHandle
      */
     private ResourceHandle getResourceFromTransaction(Transaction transaction, ResourceAllocator resourceAllocator, ResourceSpec resourceSpec) {
@@ -706,7 +706,7 @@ private ResourceHandle getResourceFromTransaction(Transaction transaction, Resou
     /**
      * To provide an unenlisted, valid, matched resource from pool.
      *
-     * @param resourceSpec ResourceSpec
+     * @param resourceSpec the ResourceSpec used to locate the correct resource pool
      * @param resourceAllocator ResourceAllocator
      * @param transaction Transaction
      * @return ResourceHandle resource from pool
@@ -793,6 +793,7 @@ protected boolean matchConnection(ResourceHandle resource, ResourceAllocator all
      * return a new resource. returns null if the pool new resources cannot be created. <br>
      *
      * @param resourceAllocator ResourceAllocator
+     * @param resourceSpec the ResourceSpec used to locate the correct resource pool
      * @return ResourceHandle resource from pool
      * @throws PoolingException if unable to create a new resource
      */
@@ -941,7 +942,7 @@ private ResourceHandle getMatchedResourceFromPool(ResourceAllocator alloc) {
     }
 
     /**
-     * Try to purge resources by size <= quantity <br>
+     * Try to purge resources by size <= quantity
      *
      * @param quantity maximum no. of resources to remove. <br>
      * @return resourceCount No. of resources actually removed. <br>
@@ -970,14 +971,16 @@ private int purgeResources(int quantity) {
      * replacement is required since the steadypoolsize might equal maxpoolsize and in that case if we were not to remove a
      * resource from the pool, our resource would be above maxPoolSize
      *
-     * @param resourceAllocator ResourceAllocator to create resource
-     * @param resourceSpec ResourceSpec
+     * @param resourceAllocator the resource allocator to be used to create the new resource
+     * @param resourceSpec the ResourceSpec used to locate the correct resource pool
      * @return newly created resource
      * @throws PoolingException when unable to create a resource
      */
     protected ResourceHandle createSingleResourceAndAdjustPool(ResourceAllocator resourceAllocator, ResourceSpec resourceSpec) throws PoolingException {
+        // TODO document in getResource when a return value can be null
         ResourceHandle handle = dataStructure.getResource();
         if (handle != null) {
+            // TODO document why it is removed / add unit test to show the behavior described in the javadoc of the method
             dataStructure.removeResource(handle);
         }
 
@@ -986,9 +989,9 @@ protected ResourceHandle createSingleResourceAndAdjustPool(ResourceAllocator res
 
     /**
      * Method to be used to create resource, instead of calling ResourceAllocator.createConfigBean(). This method handles
-     * the connection creation retrial in case of failure
+     * the connection creation retry in case of failure.
      *
-     * @param resourceAllocator ResourceAllocator
+     * @param resourceAllocator the resource allocator to be used to create the new resource
      * @return ResourceHandle newly created resource
      * @throws PoolingException when unable create a resource
      */
@@ -1023,9 +1026,9 @@ protected ResourceHandle createSingleResource(ResourceAllocator resourceAllocato
     }
 
     /**
-     * Create specified number of resources.
+     * Create specified number of resources using the given resource allocater.
      *
-     * @param alloc ResourceAllocator
+     * @param alloc the resource allocator to be used to create the new resource
      * @param size number of resources to create.
      * @throws PoolingException When unable to create a resource
      */
@@ -1162,16 +1165,22 @@ protected void freeResource(ResourceHandle resourceHandle) {
 
     }
 
-    protected boolean cleanupResource(ResourceHandle handle) {
+    /**
+     * Asks the resource allocator to cleanup the resource
+     *
+     * @param resource the resource to be cleaned up
+     * @return true if cleanup was successful, otherwise false
+     */
+    protected boolean cleanupResource(ResourceHandle resource) {
         boolean cleanupSuccessful = true;
         // cleanup resource
         try {
-            ResourceAllocator alloc = handle.getResourceAllocator();
-            alloc.cleanup(handle);
+            ResourceAllocator alloc = resource.getResourceAllocator();
+            alloc.cleanup(resource);
         } catch (PoolingException ex) {
             LOG.log(WARNING, "Cleanup of a resource from pool [" + poolInfo.getName() + "] failed.", ex);
             cleanupSuccessful = false;
-            resourceErrorOccurred(handle);
+            resourceErrorOccurred(resource);
         }
         return cleanupSuccessful;
     }
@@ -1245,12 +1254,6 @@ private void doFailAllConnectionsProcessing() {
         scheduleResizerTask();
     }
 
-    /**
-     * This method is called when a resource is enlisted in a transaction.
-     *
-     * @param tran Transaction
-     * @param resource ResourceHandle
-     */
     @Override
     public void resourceEnlisted(Transaction tran, ResourceHandle resource) throws IllegalStateException {
         poolTxHelper.resourceEnlisted(tran, resource);
@@ -1346,6 +1349,7 @@ private void incrementNumConnFailedValidation() {
         }
     }
 
+    // TODO: this method name should be createAndGetNewResource, but it is only used once, to it could be removed
     private ResourceHandle getNewResource(ResourceAllocator alloc) throws PoolingException {
         addResource(alloc);
         return dataStructure.getResource();
@@ -1364,6 +1368,9 @@ public void emptyPool() {
     @Override
     public void emptyFreeConnectionsInPool() {
         LOG.log(FINE, "Emptying free connections in the pool {0}", poolInfo.getName());
+
+        // TODO this is not completely thread safe, between getResource and removeResource
+        // the dataStructure can be altered by other threads
         ResourceHandle h;
         while ((h = dataStructure.getResource()) != null) {
             dataStructure.removeResource(h);
@@ -1408,11 +1415,6 @@ public long getReconfigWaitTime() {
         return reconfigWaitTime;
     }
 
-    /**
-     * Reinitialize connections established in the connection pool and bring the pool to steady pool size.
-     *
-     * @throws com.sun.appserv.connectors.internal.api.PoolingException
-     */
     @Override
     public synchronized boolean flushConnectionPool() throws PoolingException {
         LOG.log(FINE, "Flushing Connection Pool {0}", poolInfo);
@@ -1430,13 +1432,6 @@ public synchronized boolean flushConnectionPool() throws PoolingException {
         return true;
     }
 
-    /**
-     * Reconfigure the Pool's properties. The reconfigConnectorConnectionPool method in the ConnectorRuntime will use this
-     * method (through PoolManager) if it needs to just change pool properties and not recreate the pool
-     *
-     * @param poolResource - the ConnectorConnectionPool JavaBean that holds the new pool properties
-     * @throws PoolingException if the pool resizing fails
-     */
     @Override
     public synchronized void reconfigurePool(ConnectorConnectionPool poolResource) throws PoolingException {
         int _idleTime = Integer.parseInt(poolResource.getIdleTimeoutInSeconds()) * 1000;
@@ -1564,8 +1559,13 @@ private void killExtraResources(int numToKill) {
         scheduleResizerTask();
     }
 
-    /*
-     * Increase the number of steady resources in the pool if we detect that the steadyPoolSize has been increased
+    /**
+     * Increase the number of steady resources in the pool if we detect that the steadyPoolSize has been increased.<br>
+     * Note: if the newSteadyPoolSize is smaller than the current pool size, no changes are made directly. The resizer task
+     * is updated in all cases and will resize the pool in the future.
+     *
+     * @param newSteadyPoolSize The new steady pool size
+     * @throws PoolingException when unable to add new resources to the pool
      */
     private void increaseSteadyPoolSize(int newSteadyPoolSize) throws PoolingException {
         cancelResizerTask();
@@ -1575,27 +1575,16 @@ private void increaseSteadyPoolSize(int newSteadyPoolSize) throws PoolingExcepti
         scheduleResizerTask();
     }
 
-    /**
-     * @param alloc ResourceAllocator
-     * @throws PoolingException when unable to create a resource
-     */
+    // TODO is this private method needed? Why not call addResource directly?
     private void createResourceAndAddToPool(ResourceAllocator alloc) throws PoolingException {
         addResource(alloc);
     }
 
-    /**
-     * Switch on matching of connections in the pool.
-     */
     @Override
     public void switchOnMatching() {
         matchConnections = true;
     }
 
-    /**
-     * query the name of this pool. Required by monitoring
-     *
-     * @return the name of this pool
-     */
     @Override
     public final PoolInfo getPoolInfo() {
         return poolInfo;
@@ -1707,12 +1696,6 @@ public void reclaimConnection(ResourceHandle handle) {
         notifyWaitingThreads();
     }
 
-    /**
-     * Get Connection Pool status by computing the free/used values of the connections in the pool. Computations are based
-     * on whether the pool is initialized or not when this method is invoked.
-     *
-     * @return PoolStatus object
-     */
     @Override
     public PoolStatus getPoolStatus() {
         PoolStatus poolStatus = new PoolStatus(this.poolInfo);