Move expensive operations outside lock

Author: rPraml

ebean-datasource/src/main/java/io/ebean/datasource/pool/ConnectionBuffer.java

@@ -239,29 +239,44 @@ PooledConnection removeFree(Object affinityId) {
     return pc;
   }
 
+
   /**
-   * Close all free connections in this buffer.
+   * Clears the freelist and return the connections.
    */
-  void closeAllFree(boolean logErrors) {
+  List<PooledConnection> clearFreeList() {
     List<PooledConnection> tempList = new ArrayList<>();
 
     freeList.forEach(pc -> {
       pc.unlink();
       tempList.add(pc);
     });
+    return tempList;
+  }
 
-    if (Log.isLoggable(System.Logger.Level.TRACE)) {
-      Log.trace("... closing all {0} connections from the free list with logErrors: {1}", tempList.size(), logErrors);
-    }
-    for (PooledConnection connection : tempList) {
-      connection.closeConnectionFully(logErrors);
-    }
+
+  /**
+   * Clears the busy list and return the connections that should be considered leaked.
+   */
+  List<PooledConnection> cleanupBusyList(long leakTimeMinutes) {
+    long olderThanTime = System.currentTimeMillis() - (leakTimeMinutes * 60000);
+    List<PooledConnection> tempList = new ArrayList<>();
+
+    busyList.forEach(pc -> {
+      if (pc.lastUsedTime() > olderThanTime) {
+        // PooledConnection has been used recently or
+        // expected to be longRunning so not closing...
+      } else {
+        pc.unlink();
+        tempList.add(pc);
+      }
+    });
+    return tempList;
   }
 
   /**
    * Trim any inactive connections that have not been used since usedSince.
    */
-  int trim(int minSize, long usedSince, long createdSince) {
+  List<PooledConnection> trim(int minSize, long usedSince, long createdSince) {
     int toTrim = freeSize() - minSize;
 
     List<PooledConnection> ret = new ArrayList<>(toTrim);
@@ -274,35 +289,7 @@ int trim(int minSize, long usedSince, long createdSince) {
         ret.add(pc);
       }
     }
-    ret.forEach(pc -> pc.closeConnectionFully(true));
-    return ret.size();
-  }
-
-  /**
-   * Close connections that should be considered leaked.
-   */
-  void closeBusyConnections(long leakTimeMinutes) {
-    long olderThanTime = System.currentTimeMillis() - (leakTimeMinutes * 60000);
-    Log.debug("Closing busy connections using leakTimeMinutes {0}", leakTimeMinutes);
-    busyList.forEach(pc -> {
-      if (pc.lastUsedTime() > olderThanTime) {
-        // PooledConnection has been used recently or
-        // expected to be longRunning so not closing...
-      } else {
-        pc.unlink();
-        closeBusyConnection(pc);
-      }
-    });
-  }
-
-  private void closeBusyConnection(PooledConnection pc) {
-    try {
-      Log.warn("DataSource closing busy connection? {0}", pc.fullDescription());
-      System.out.println("CLOSING busy connection: " + pc.fullDescription());
-      pc.closeConnectionFully(false);
-    } catch (Exception ex) {
-      Log.error("Error when closing potentially leaked connection " + pc.description(), ex);
-    }
+    return ret;
   }
 
   /**

ebean-datasource/src/main/java/io/ebean/datasource/pool/PooledConnectionQueue.java

@@ -5,6 +5,8 @@
 import io.ebean.datasource.pool.ConnectionPool.Status;
 
 import java.sql.SQLException;
+import java.util.Collections;
+import java.util.List;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.locks.Condition;
 import java.util.concurrent.locks.ReentrantLock;
@@ -107,15 +109,10 @@ PoolStatus status(boolean reset) {
   }
 
   void setMaxSize(int maxSize) {
-    lock.lock();
-    try {
-      if (maxSize < this.minSize) {
-        throw new IllegalArgumentException("maxSize " + maxSize + " < minSize " + this.minSize);
-      }
-      this.maxSize = maxSize;
-    } finally {
-      lock.unlock();
+    if (maxSize < this.minSize) {
+      throw new IllegalArgumentException("maxSize " + maxSize + " < minSize " + this.minSize);
     }
+    this.maxSize = maxSize;
   }
 
   private int totalConnections() {
@@ -141,20 +138,24 @@ void ensureMinimumConnections() throws SQLException {
    * Return a PooledConnection.
    */
   void returnPooledConnection(PooledConnection c, boolean forceClose) {
+    boolean closeConnection = false;
     lock.lock();
     try {
       if (!buffer.removeBusy(c)) {
         Log.error("Connection [{0}] not found in BusyList?", c);
       }
       if (forceClose || c.shouldTrimOnReturn(lastResetTime, maxAgeMillis)) {
-        c.closeConnectionFully(false);
+        closeConnection = true;
       } else {
         buffer.addFree(c);
         notEmpty.signal();
       }
     } finally {
       lock.unlock();
     }
+    if (closeConnection) {
+      c.closeConnectionFully(false);
+    }
   }
 
   private PooledConnection extractFromFreeList(Object affinitiyId) {
@@ -356,17 +357,12 @@ void reset(long leakTimeMinutes) {
   }
 
   void trim(long maxInactiveMillis, long maxAgeMillis) {
-    lock.lock();
-    try {
-      if (trimInactiveConnections(maxInactiveMillis, maxAgeMillis)) {
-        try {
-          ensureMinimumConnections();
-        } catch (SQLException e) {
-          Log.error("Error trying to ensure minimum connections", e);
-        }
+    if (trimInactiveConnections(maxInactiveMillis, maxAgeMillis)) {
+      try {
+        ensureMinimumConnections();
+      } catch (SQLException e) {
+        Log.error("Error trying to ensure minimum connections", e);
       }
-    } finally {
-      lock.unlock();
     }
   }
 
@@ -375,33 +371,50 @@ void trim(long maxInactiveMillis, long maxAgeMillis) {
    */
   private boolean trimInactiveConnections(long maxInactiveMillis, long maxAgeMillis) {
     final long createdSince = (maxAgeMillis == 0) ? 0 : System.currentTimeMillis() - maxAgeMillis;
-    final int trimmedCount;
-    if (buffer.freeSize() > minSize) {
-      // trim on maxInactive and maxAge
-      long usedSince = System.currentTimeMillis() - maxInactiveMillis;
-      trimmedCount = buffer.trim(minSize, usedSince, createdSince);
-    } else if (createdSince > 0) {
-      // trim only on maxAge
-      trimmedCount = buffer.trim(0, createdSince, createdSince);
-    } else {
-      trimmedCount = 0;
+    final List<PooledConnection> toTrim;
+    lock.lock();
+    try {
+      if (buffer.freeSize() > minSize) {
+        // trim on maxInactive and maxAge
+        long usedSince = System.currentTimeMillis() - maxInactiveMillis;
+        toTrim = buffer.trim(minSize, usedSince, createdSince);
+      } else if (createdSince > 0) {
+        // trim only on maxAge
+        toTrim = buffer.trim(0, createdSince, createdSince);
+      } else {
+        toTrim = Collections.emptyList();
+      }
+    } finally {
+      lock.unlock();
+    }
+    if (toTrim.isEmpty()) {
+      return false;
     }
-    if (trimmedCount > 0 && Log.isLoggable(DEBUG)) {
-      Log.debug("DataSource [{0}] trimmed [{1}] inactive connections. New size[{2}]", name, trimmedCount, totalConnections());
+    toTrim.forEach(pc -> pc.closeConnectionFully(true));
+    if (Log.isLoggable(DEBUG)) {
+      Log.debug("DataSource [{0}] trimmed [{1}] inactive connections. New size[{2}]", name, toTrim.size(), totalConnections());
     }
-    return trimmedCount > 0 && buffer.freeSize() < minSize;
+    return buffer.freeSize() < minSize;
   }
 
   /**
    * Close all the connections that are in the free list.
    */
   private void closeFreeConnections(boolean logErrors) {
+    List<PooledConnection> tempList;
     lock.lock();
     try {
-      buffer.closeAllFree(logErrors);
+      tempList = buffer.clearFreeList();
     } finally {
       lock.unlock();
     }
+    // closing the connections is done outside lock
+    if (Log.isLoggable(System.Logger.Level.TRACE)) {
+      Log.trace("... closing all {0} connections from the free list with logErrors: {1}", tempList.size(), logErrors);
+    }
+    for (PooledConnection connection : tempList) {
+      connection.closeConnectionFully(logErrors);
+    }
   }
 
   /**
@@ -415,12 +428,22 @@ private void closeFreeConnections(boolean logErrors) {
    * closed and put back into the pool.
    */
   void closeBusyConnections(long leakTimeMinutes) {
+    List<PooledConnection> tempList;
     lock.lock();
     try {
-      buffer.closeBusyConnections(leakTimeMinutes);
+      tempList = buffer.cleanupBusyList(leakTimeMinutes);
     } finally {
       lock.unlock();
     }
+    for (PooledConnection pc : tempList) {
+      try {
+        Log.warn("DataSource closing busy connection? {0}", pc.fullDescription());
+        System.out.println("CLOSING busy connection: " + pc.fullDescription());
+        pc.closeConnectionFully(false);
+      } catch (Exception ex) {
+        Log.error("Error when closing potentially leaked connection " + pc.description(), ex);
+      }
+    }
   }
 
   String getBusyConnectionInformation() {