exceptions handling for create object is thrown directly, and minor additions to pool stats methods (#4)

Author: venkata-kishore

pom.xml

@@ -7,11 +7,11 @@
     <parent>
         <groupId>today.bonfire.oss</groupId>
         <artifactId>bonfire-oss-parent</artifactId>
-        <version>1.1.4</version>
+        <version>1.1.5</version>
     </parent>
 
     <artifactId>simple-object-pool</artifactId>
-    <version>2.1.0</version>
+    <version>2.2.0</version>
     <packaging>jar</packaging>
 
     <properties>

src/main/java/today/bonfire/oss/sop/PooledObject.java

@@ -18,7 +18,7 @@ public class PooledObject<T extends PoolObject> {
   private          long timesBorrowed;
 
   /**
-   * Creates a new PooledEntity wrapping the given object with a specified ID.
+   * Creates a new PooledObject wrapping the given object with a specified ID.
    * Initializes the object in an idle, non-broken state with current timestamp.
    *
    * @param object The object to be pooled

src/main/java/today/bonfire/oss/sop/SimpleObjectPool.java

@@ -41,6 +41,7 @@ public class SimpleObjectPool<T extends PoolObject> implements AutoCloseable {
   private final Condition                              retryCreationWait;
   private final AtomicLong                             objectCreateCount = new AtomicLong(0);
   private final AtomicInteger                          currentPoolSize   = new AtomicInteger(0);
+  private final AtomicLong timesBorrowed = new AtomicLong(0);
 
   public SimpleObjectPool(SimpleObjectPoolConfig config, PooledObjectFactory<T> factory) {
     this.config       = config;
@@ -172,7 +173,7 @@ private void evictionRun() {
 
     if (!objectsToDestroy.isEmpty()) {
       log.debug("Evicted {} idle objects. Current pool size: {}, Idle: {}, Borrowed: {}",
-               objectsToDestroy.size(), currentPoolSize(), idleObjectCount(), borrowedObjectsCount());
+                objectsToDestroy.size(), currentPoolSize(), idleObjectCount(), borrowedObjectsCount());
       objectsToDestroy.clear();
     }
   }
@@ -256,7 +257,8 @@ private PooledObject<T> createObject() {
     try {
       pooledObject = new PooledObject<>(factory.createObject(), objectCreateCount.incrementAndGet());
     } catch (Exception e) {
-      throw new PoolObjectException("Failed to create object", e);
+      log.error("Failed to create object for the pool: {}", e.getMessage());
+      throw e;
     }
     if (config.testOnCreate()) {
       if (!factory.isObjectValid(pooledObject.object())) {
@@ -319,9 +321,15 @@ public T borrowObject() throws PoolException {
               pooledObject  = createObject();
               createdObject = true;
               retriesLeft--;
-            } catch (PoolObjectException | PoolObjectValidationException e) {
-              log.error("Failed to create objects when borrowing", e);
+            } catch (Exception e) {
+              if (e instanceof PoolObjectValidationException) {
+                log.error("", e);
+              }
               retriesLeft--;
+              if (retriesLeft < 0) {
+                // we have exhausted retries and throw the actual exception so that the caller can handle it
+                throw e;
+              }
               continue;
             }
           }
@@ -343,11 +351,13 @@ public T borrowObject() throws PoolException {
           continue;
         }
 
+
         // Object is valid, prepare for borrowing
         pooledObject.borrow();
         factory.activateObject(object);
         borrowedObjects.put(pooledObject.id(), pooledObject);
         if (createdObject) currentPoolSize.incrementAndGet();
+        timesBorrowed.incrementAndGet();
         notEmpty.signal();
         log.trace("Resource borrowed - id: {}, current pool size: {}",
                   pooledObject.id(), currentPoolSize.get());
@@ -380,43 +390,43 @@ public void returnObject(T obj, boolean broken) throws PoolObjectException {
 
     try {
       lock.lock();
-      var pooledEntity = borrowedObjects.get(obj.getEntityId());
-      if (pooledEntity == null) {
+      var pooledObject = borrowedObjects.get(obj.getEntityId());
+      if (pooledObject == null) {
         log.warn("Attempted returning object that is not in borrowed objects list. id: {}", obj.getEntityId());
         return;
       }
       if (broken) {
-        pooledEntity.broken(true);
+        pooledObject.broken(true);
       }
       // First check if object is broken
-      boolean isValid = !pooledEntity.isBroken();
+      boolean isValid = !pooledObject.isBroken();
 
       // Then perform testOnReturn validation if configured and object isn't already invalid
       if (isValid && config.testOnReturn()) {
         try {
           isValid = factory.isObjectValid(obj);
         } catch (Exception e) {
           log.error("Error validating object on return", e);
+          // exception is swallowed since object is already invalid and will be destroyed.
           isValid = false;
         }
       }
 
       if (!isValid) {
-        removeAndDestroyBorrowedObjects(pooledEntity);
-        log.warn("Returned broken or invalid entity with id {} and destroyed it.", pooledEntity.id());
+        log.warn("Returned broken or invalid entity with id {} and destroying it.", pooledObject.id());
+        removeAndDestroyBorrowedObjects(pooledObject);
       } else {
 
         factory.passivateObject(obj);
-        borrowedObjects.remove(pooledEntity.id());
-        pooledEntity.markIdle();
-        idleObjects.add(pooledEntity);
+        borrowedObjects.remove(pooledObject.id());
+        pooledObject.markIdle();
+        idleObjects.add(pooledObject);
         log.trace("Object returned - id: {}, current pool size: {}",
-                  pooledEntity.id(), currentPoolSize.get());
+                  pooledObject.id(), currentPoolSize.get());
         notEmpty.signal();
       }
     } catch (Exception e) {
-      log.error("Error returning object to pool", e);
-      throw new PoolObjectException("Failed to return object to pool", e);
+      throw new PoolObjectException("Unable to properly return object back to pool", e);
     } finally {
       lock.unlock();
     }
@@ -530,4 +540,43 @@ public long numOfObjectsCreated() {
   public int waitingCount() {
     return lock.getQueueLength();
   }
+
+
+  /**
+   * Returns the total number of times objects have been borrowed from this pool
+   * since its creation.
+   *
+   * @return the total number of times objects have been borrowed
+   */
+  public long numOfTimesBorrowedFromPool() {
+    return timesBorrowed.get();
+  }
+
+
+  /**
+   * Returns the number of times a specific object has been borrowed from the pool
+   * since its creation.
+   * <p>
+   * Note that this method will only return statistics for objects that are currently
+   * borrowed from the pool. For objects that are idle this will have to iterate and will have a delay.
+   * If you need to check, check only for borrowed objects.
+   *
+   * @param objectId the id of the object to query, may be null
+   * @return the number of times the object has been borrowed
+   */
+  public long numOfTimesBorrowed(Long objectId) {
+    if (objectId == null) return 0;
+    var pooledObject = borrowedObjects.get(objectId);
+    if (pooledObject == null) {
+      log.warn("Object with id {} not found in borrowed objects", objectId);
+      pooledObject = idleObjects.stream()
+                                .filter(pooledObject1 -> pooledObject1.id().equals(objectId))
+                                .findAny().orElse(null);
+    }
+    if (pooledObject == null) {
+      log.warn("Object with id {} not found in idle objects", objectId);
+      return 0;
+    }
+    return pooledObject.timesBorrowed();
+  }
 }

src/test/java/today/bonfire/oss/sop/SimpleObjectPoolConfigTest.java

@@ -5,7 +5,6 @@
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.time.Duration;
@@ -16,7 +15,7 @@
 
 class SimpleObjectPoolConfigTest {
 
-  private static final Logger                      log = LoggerFactory.getLogger(SimpleObjectPoolConfig.class);
+  private static final org.slf4j.Logger log = LoggerFactory.getLogger(SimpleObjectPoolConfig.class);
   private              ListAppender<ILoggingEvent> listAppender;
 
   @BeforeEach

src/test/java/today/bonfire/oss/sop/SimpleObjectPoolPerformanceTest.java

@@ -1,5 +1,6 @@
 package today.bonfire.oss.sop;
 
+import lombok.extern.slf4j.Slf4j;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.junit.jupiter.MockitoExtension;
@@ -15,14 +16,14 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
-@ExtendWith(MockitoExtension.class)
+@Slf4j @ExtendWith(MockitoExtension.class)
 class SimpleObjectPoolPerformanceTest {
 
   private static final int  MAX_POOL_SIZE       = 10;
   private static final int  MIN_POOL_SIZE       = 5;
-  private static final long IDLE_TIMEOUT        = 1000L;
-  private static final long ABANDONED_TIMEOUT   = 2000L;
-  private static final int  NUM_BORROW_REQUESTS = 20000;
+  private static final long IDLE_TIMEOUT        = 10000L;
+  private static final long ABANDONED_TIMEOUT   = 20000L;
+  private static final int  NUM_BORROW_REQUESTS = 10000;
 
 
   @Test
@@ -31,7 +32,8 @@ void testHighConcurrencyBorrowAndReturn() throws Exception {
     var pool = new SimpleObjectPool<>(SimpleObjectPoolConfig.builder()
                                                             .maxPoolSize(MAX_POOL_SIZE)
                                                             .minPoolSize(MIN_POOL_SIZE)
-                                                            .testWhileIdle(true)
+                                                            .testWhileIdle(false)
+                                                            .waitingForObjectTimeout(Duration.ofSeconds(20))
                                                             .objEvictionTimeout(Duration.ofMillis(IDLE_TIMEOUT))
                                                             .abandonedTimeout(Duration.ofMillis(ABANDONED_TIMEOUT))
                                                             .build(), factory);
@@ -45,16 +47,26 @@ void testHighConcurrencyBorrowAndReturn() throws Exception {
         try {
           var entity = pool.borrowObject();
           assertThat(entity).isNotNull();
+          Thread.sleep(10);
           borrowCounter.incrementAndGet();
+          // log.debug("Borrowed object with id {}", borrowCounter.incrementAndGet());
           pool.returnObject(entity);
         } catch (Exception e) {
-          throw new RuntimeException("Unexpected exception: " + e.getMessage());
+          throw new RuntimeException(e);
         }
       }, executor));
     }
     assertThat(factory.getIdCounter().get()).isEqualTo(MAX_POOL_SIZE);
-    CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).get(10, TimeUnit.SECONDS);
+    CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).get(30, TimeUnit.SECONDS);
+    assertThat(pool.numOfTimesBorrowedFromPool()).isEqualTo(NUM_BORROW_REQUESTS);
+    log.info("pool size: {}, times borrowed: {}", pool.currentPoolSize(), pool.numOfTimesBorrowedFromPool());
+    for (var i = 0; i < MAX_POOL_SIZE; i++) {
+      var       entity        = pool.borrowObject();
+      final var timesBorrowed = pool.numOfTimesBorrowed(entity.getEntityId());
+      log.debug("Borrowed object's count {} with id {}", timesBorrowed, entity.getEntityId());
+    }
     assertThat(borrowCounter.get()).isEqualTo(NUM_BORROW_REQUESTS);
+    pool.close();
     executor.shutdown();
     assertThat(executor.awaitTermination(5, TimeUnit.SECONDS)).isTrue();
   }