Author: fschumacher
Date: Sat Nov 21 13:29:18 2015
New Revision: 1715521

URL: http://svn.apache.org/viewvc?rev=1715521&view=rev
Log:
Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=34319

It is not only inefficient memory wise to load all sessions in processExpire, 
it will
put more pressure on the lock for JDBCStore, also.

Modified:
    tomcat/trunk/java/org/apache/catalina/session/JDBCStore.java
    tomcat/trunk/java/org/apache/catalina/session/StoreBase.java
    tomcat/trunk/webapps/docs/changelog.xml

Modified: tomcat/trunk/java/org/apache/catalina/session/JDBCStore.java
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/session/JDBCStore.java?rev=1715521&r1=1715520&r2=1715521&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/session/JDBCStore.java (original)
+++ tomcat/trunk/java/org/apache/catalina/session/JDBCStore.java Sat Nov 21 
13:29:18 2015
@@ -156,11 +156,6 @@ public class JDBCStore extends StoreBase
     protected PreparedStatement preparedSizeSql = null;
 
     /**
-     * Variable to hold the <code>keys()</code> prepared statement.
-     */
-    protected PreparedStatement preparedKeysSql = null;
-
-    /**
      * Variable to hold the <code>save()</code> prepared statement.
      */
     protected PreparedStatement preparedSaveSql = null;
@@ -458,17 +453,28 @@ public class JDBCStore extends StoreBase
 
     // --------------------------------------------------------- Public Methods
 
+    @Override
+    public String[] expiredKeys() throws IOException {
+        return keys(true);
+    }
+
+    @Override
+    public String[] keys() throws IOException {
+        return keys(false);
+    }
+
     /**
      * Return an array containing the session identifiers of all Sessions
      * currently saved in this Store.  If there are no such Sessions, a
      * zero-length array is returned.
      *
+     * @param expiredOnly flag, whether only keys of expired sessions should
+     *        be returned
      * @return array containing the list of session IDs
      *
      * @exception IOException if an input/output error occurred
      */
-    @Override
-    public String[] keys() throws IOException {
+    private String[] keys(boolean expiredOnly) throws IOException {
         String keys[] = null;
         synchronized (this) {
             int numberOfTries = 2;
@@ -479,24 +485,29 @@ public class JDBCStore extends StoreBase
                     return new String[0];
                 }
                 try {
-                    if (preparedKeysSql == null) {
-                        String keysSql = "SELECT " + sessionIdCol + " FROM "
-                                + sessionTable + " WHERE " + sessionAppCol
-                                + " = ?";
-                        preparedKeysSql = _conn.prepareStatement(keysSql);
-                    }
 
-                    preparedKeysSql.setString(1, getName());
-                    try (ResultSet rst = preparedKeysSql.executeQuery()) {
-                        ArrayList<String> tmpkeys = new ArrayList<>();
-                        if (rst != null) {
-                            while (rst.next()) {
-                                tmpkeys.add(rst.getString(1));
+                    String keysSql = "SELECT " + sessionIdCol + " FROM "
+                            + sessionTable + " WHERE " + sessionAppCol + " = 
?";
+                    if (expiredOnly) {
+                        keysSql += " AND (" + sessionLastAccessedCol + " + "
+                                + sessionMaxInactiveCol + " * 1000 < ?)";
+                    }
+                    try (PreparedStatement preparedKeysSql = 
_conn.prepareStatement(keysSql)) {
+                        preparedKeysSql.setString(1, getName());
+                        if (expiredOnly) {
+                            preparedKeysSql.setLong(2, 
System.currentTimeMillis());
+                        }
+                        try (ResultSet rst = preparedKeysSql.executeQuery()) {
+                            ArrayList<String> tmpkeys = new ArrayList<>();
+                            if (rst != null) {
+                                while (rst.next()) {
+                                    tmpkeys.add(rst.getString(1));
+                                }
                             }
+                            keys = tmpkeys.toArray(new String[tmpkeys.size()]);
+                            // Break out after the finally block
+                            numberOfTries = 0;
                         }
-                        keys = tmpkeys.toArray(new String[tmpkeys.size()]);
-                        // Break out after the finally block
-                        numberOfTries = 0;
                     }
                 } catch (SQLException e) {
                     
manager.getContext().getLogger().error(sm.getString(getStoreName() + 
".SQLException", e));
@@ -934,13 +945,6 @@ public class JDBCStore extends StoreBase
         this.preparedSizeSql = null;
 
         try {
-            preparedKeysSql.close();
-        } catch (Throwable f) {
-            ExceptionUtils.handleThrowable(f);
-        }
-        this.preparedKeysSql = null;
-
-        try {
             preparedSaveSql.close();
         } catch (Throwable f) {
             ExceptionUtils.handleThrowable(f);

Modified: tomcat/trunk/java/org/apache/catalina/session/StoreBase.java
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/session/StoreBase.java?rev=1715521&r1=1715520&r2=1715521&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/session/StoreBase.java (original)
+++ tomcat/trunk/java/org/apache/catalina/session/StoreBase.java Sat Nov 21 
13:29:18 2015
@@ -113,6 +113,18 @@ public abstract class StoreBase extends
     }
 
     /**
+     * Get only those keys of sessions, that are saved in the Store and are to
+     * be expired.
+     *
+     * @return list of session keys, that are to be expired
+     * @throws IOException
+     *             if an input-/output error occurred
+     */
+    public String[] expiredKeys() throws IOException {
+        return keys();
+    }
+
+    /**
      * Called by our background reaper thread to check if Sessions
      * saved in our store are subject of being expired. If so expire
      * the Session and remove it from the Store.
@@ -126,7 +138,7 @@ public abstract class StoreBase extends
         }
 
         try {
-            keys = keys();
+            keys = expiredKeys();
         } catch (IOException e) {
             manager.getContext().getLogger().error("Error getting keys", e);
             return;

Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1715521&r1=1715520&r2=1715521&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Sat Nov 21 13:29:18 2015
@@ -67,6 +67,11 @@
         <code>Engine</code> rather than a <code>Container</code>. (markt)
       </scode>
       <fix>
+        <bug>34319</bug>: Only load those keys in 
<code>StoreBase.processExpire</code>
+        from JDBCStore, that are old enough, to be expired. Based on a patch
+        by Tom Anderson. (fschumacher)
+      </fix>
+      <fix>
         <bug>58629</bug>: Allow an embedded Tomcat instance to start when the
         <code>Service</code> has no <code>Engine</code> configured. (markt)
       </fix>



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to