Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=51264

Improve fix to return connections to the pool when not in use.
Patch provided by Felix Schumacher.

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1133134 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/java/org/apache/catalina/session/JDBCStore.java b/java/org/apache/catalina/session/JDBCStore.java
index 7580c2b..6c422b1 100644 (file)
--- a/java/org/apache/catalina/session/JDBCStore.java
+++ b/java/org/apache/catalina/session/JDBCStore.java
@@ -711,16 +711,7 @@ public class JDBCStore extends StoreBase {
                 }
 
                 try {
-                    if (preparedRemoveSql == null) {
-                        String removeSql = "DELETE FROM " + sessionTable
-                                + " WHERE " + sessionIdCol + " = ?  AND "
-                                + sessionAppCol + " = ?";
-                        preparedRemoveSql = _conn.prepareStatement(removeSql);
-                    }
-
-                    preparedRemoveSql.setString(1, id);
-                    preparedRemoveSql.setString(2, getName());
-                    preparedRemoveSql.execute();
+                    remove(id, _conn);
                     // Break out after the finally block
                     numberOfTries = 0;
                 } catch (SQLException e) {
@@ -740,6 +731,28 @@ public class JDBCStore extends StoreBase {
     }
 
     /**
+     * Remove the Session with the specified session identifier from
+     * this Store, if present.  If no such Session is present, this method
+     * takes no action.
+     * 
+     * @param id Session identifier of the Session to be removed
+     * @param _conn open connection to be used
+     * @throws SQLException if an error occurs while talking to the database
+     */
+    private void remove(String id, Connection _conn) throws SQLException {
+        if (preparedRemoveSql == null) {
+            String removeSql = "DELETE FROM " + sessionTable
+                    + " WHERE " + sessionIdCol + " = ?  AND "
+                    + sessionAppCol + " = ?";
+            preparedRemoveSql = _conn.prepareStatement(removeSql);
+        }
+
+        preparedRemoveSql.setString(1, id);
+        preparedRemoveSql.setString(2, getName());
+        preparedRemoveSql.execute();
+    }
+
+    /**
      * Remove all of the Sessions in this Store.
      *
      * @exception IOException if an input/output error occurs
@@ -799,12 +812,12 @@ public class JDBCStore extends StoreBase {
                     return;
                 }
 
-                // If sessions already exist in DB, remove and insert again.
-                // TODO:
-                // * Check if ID exists in database and if so use UPDATE.
-                remove(session.getIdInternal());
-
                 try {
+                    // If sessions already exist in DB, remove and insert again.
+                    // TODO:
+                    // * Check if ID exists in database and if so use UPDATE.
+                    remove(session.getIdInternal(), _conn);
+                    
                     bos = new ByteArrayOutputStream();
                     oos = new ObjectOutputStream(new BufferedOutputStream(bos));
 
@@ -874,11 +887,13 @@ public class JDBCStore extends StoreBase {
      * @return <code>Connection</code> if the connection succeeded
      */
     protected Connection getConnection() {
+        Connection conn = null;
         try {
-            if (dbConnection == null || dbConnection.isClosed()) {
+            conn = open();
+            if (conn == null || conn.isClosed()) {
                 manager.getContainer().getLogger().info(sm.getString(getStoreName() + ".checkConnectionDBClosed"));
-                open();
-                if (dbConnection == null || dbConnection.isClosed()) {
+                conn = open();
+                if (conn == null || conn.isClosed()) {
                     manager.getContainer().getLogger().info(sm.getString(getStoreName() + ".checkConnectionDBReOpenFail"));
                 }
             }
@@ -887,7 +902,7 @@ public class JDBCStore extends StoreBase {
                     ex.toString()));
         }
 
-        return dbConnection;
+        return conn;
     }
 
     /**
@@ -916,8 +931,7 @@ public class JDBCStore extends StoreBase {
         }
         
         if (dataSource != null) {
-            dbConnection = dataSource.getConnection();
-            return dbConnection;
+            return dataSource.getConnection();
         }
 
         // Instantiate our database driver if necessary
@@ -1014,13 +1028,15 @@ public class JDBCStore extends StoreBase {
     }
 
     /**
-     * Release the connection, not needed here since the
-     * connection is not associated with a connection pool.
+     * Release the connection, if it
+     * is associated with a connection pool.
      *
      * @param conn The connection to be released
      */
     protected void release(Connection conn) {
-        // NOOP
+        if (dataSource != null) {
+            close(conn); 
+        }
     }
 
     /**
@@ -1033,8 +1049,10 @@ public class JDBCStore extends StoreBase {
     @Override
     protected synchronized void startInternal() throws LifecycleException {
 
-        // Open connection to the database
-        this.dbConnection = getConnection();
+        if (dataSource == null) {
+            // If not using a connection pool, open a connection to the database
+            this.dbConnection = getConnection();
+        }
         
         super.startInternal();
     }

diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 2ebde24..ec7cb86 100644 (file)
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -46,6 +46,11 @@
   <subsection name="Catalina">
     <changelog>
       <fix>
+        <bug>51264</bug>: Improve the previous fix for this issue by returning
+        the connection to the pool when not in use so it does not appear to be
+        an abandoned connection. Patch provided by Felix Schumacher. (markt)
+      </fix>
+      <fix>
         <bug>51324</bug>: Improve handling of exceptions when flushing the
         response buffer to ensure that the doFlush flag does not get stuck in
         the enabled state. Patch provided by Jeremy Norris. (markt)

diff --git a/webapps/docs/config/manager.xml b/webapps/docs/config/manager.xml
index f5b6bdc..2b9835e 100644 (file)
--- a/webapps/docs/config/manager.xml
+++ b/webapps/docs/config/manager.xml
@@ -360,7 +360,10 @@
       <p>Name of the JNDI resource for a JDBC DataSource-factory. If this option
       is given and a valid JDBC resource can be found, it will be used and any
       direct configuration of a JDBC connection via <code>connectionURL</code>
-      and <code>driverName</code> will be ignored.</p>
+      and <code>driverName</code> will be ignored. Since this code uses prepared
+      statements, you might want to configure pooled prepared statements as
+      shown in <a href="../jndi-resources-howto.html">the JNDI resources
+      HOW-TO</a>.</p>
     </attribute>
 
     <attribute name="driverName" required="true">