VELOCITY-989 Fixed multiple connection leaks in DataSourceResourceLoader.

velocity-engine-core/pom.xml

@@ -252,6 +252,23 @@
             <classifier>${test.jdbc.driver.classifier}</classifier>
         </dependency>
         <dependency>
+            <groupId>org.apache.commons</groupId>
+            <artifactId>commons-dbcp2</artifactId>
+            <version>2.13.0</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.apache.tomcat</groupId>
+            <artifactId>tomcat-jdbc</artifactId>
+            <version>9.0.104</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>com.zaxxer</groupId>
+            <artifactId>HikariCP</artifactId>
+            <version>4.0.3</version>
+            <scope>test</scope>
+        </dependency>        <dependency>
             <groupId>org.slf4j</groupId>
             <artifactId>slf4j-simple</artifactId>
             <scope>test</scope>

velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java

@@ -167,7 +167,9 @@ public void close() throws IOException
             {
                 PreparedStatement statement = (PreparedStatement) resultSet.getStatement();
                 try {
-                    resultSet.close();
+                    if (!resultSet.isClosed()) {
+                        resultSet.close();
+                    }
                 } finally {
                     factory.releaseStatement(templateSQL, statement);
                 }
@@ -417,7 +419,9 @@ private void closeResultSet(final ResultSet rs)
         {
             try
             {
-                rs.close();
+                if (!rs.isClosed()) {
+                    rs.close();
+                }
             }
             catch (RuntimeException re)
             {

velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DefaultDatabaseObjectsFactory.java

@@ -35,7 +35,7 @@ public void init(DataSource dataSource, ExtProperties properties)
     public PreparedStatement prepareStatement(String sql) throws SQLException
     {
         Connection connection = dataSource.getConnection();
-        return connection.prepareStatement(sql);
+        return PreparedStatementProxy.newInstance(connection.prepareStatement(sql), connection);
     }
 
     /**
@@ -49,11 +49,15 @@ public void releaseStatement(String sql, PreparedStatement stmt) throws SQLExcep
         Connection connection = stmt.getConnection();
         try
         {
-            stmt.close();
+            if (!stmt.isClosed()) {
+                stmt.close();
+            }
         }
         finally
         {
-            connection.close();
+            if (!connection.isClosed()) {
+                connection.close();
+            }
         }
     }
 }

velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/PreparedStatementProxy.java

@@ -0,0 +1,64 @@
+package org.apache.velocity.runtime.resource.loader;
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+
+public class PreparedStatementProxy implements InvocationHandler {
+    private final PreparedStatement wrappedPreparedStatement;
+    private PreparedStatement proxyPreparedStatement;
+    private final Connection wrappedConnection;
+
+    public PreparedStatementProxy(PreparedStatement wrappedPreparedStatement, Connection wrappedConnection) {
+        this.wrappedPreparedStatement = wrappedPreparedStatement;
+        this.wrappedConnection = wrappedConnection;
+    }
+
+    @Override
+    public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+        Object result;
+        if (method.getName().equals("getConnection")) {
+            result = wrappedConnection;
+        } else if (method.getName().equals("executeQuery")) {
+            return ResultSetProxy.newInstance((ResultSet) method.invoke(wrappedPreparedStatement, args), proxyPreparedStatement);
+        } else {
+            result = method.invoke(wrappedPreparedStatement, args);
+        }
+        return result;
+    }
+
+    public static PreparedStatement newInstance(PreparedStatement ps, Connection con) throws SQLException {
+        PreparedStatementProxy invocationHandler = new PreparedStatementProxy(ps, con);
+        PreparedStatement proxyPreparedStatement = (PreparedStatement) Proxy.newProxyInstance(ps.getClass().getClassLoader(),
+                ps.getClass().getInterfaces(),
+                invocationHandler);
+        invocationHandler.setProxyPreparedStatement(proxyPreparedStatement);
+        return proxyPreparedStatement;
+    }
+
+    public void setProxyPreparedStatement(PreparedStatement proxyPreparedStatement) {
+        this.proxyPreparedStatement = proxyPreparedStatement;
+    }
+}

velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/ResultSetProxy.java

@@ -0,0 +1,54 @@
+package org.apache.velocity.runtime.resource.loader;
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+
+public class ResultSetProxy implements InvocationHandler {
+    private final ResultSet wrappedResultSet;
+    private final PreparedStatement wrappedPreparedStatement;
+
+    public ResultSetProxy(ResultSet wrappedResultSet, PreparedStatement wrappedPreparedStatement) {
+        this.wrappedResultSet = wrappedResultSet;
+        this.wrappedPreparedStatement = wrappedPreparedStatement;
+    }
+
+    @Override
+    public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+        Object result;
+        if (method.getName().equals("getStatement")) {
+            result = wrappedPreparedStatement;
+        } else {
+            result = method.invoke(wrappedResultSet, args);
+        }
+        return result;
+    }
+
+    public static ResultSet newInstance(ResultSet rs, PreparedStatement ps) throws SQLException {
+        return (ResultSet) Proxy.newProxyInstance(rs.getClass().getClassLoader(),
+                rs.getClass().getInterfaces(),
+                new ResultSetProxy(rs, ps));
+    }
+
+}

velocity-engine-core/src/test/java/org/apache/velocity/test/sql/DataSourceResourceLoaderTestCase.java

@@ -19,8 +19,10 @@
  * under the License.
  */
 
+import com.zaxxer.hikari.HikariDataSource;
 import junit.framework.Test;
 import junit.framework.TestSuite;
+import org.apache.commons.dbcp2.BasicDataSource;
 import org.apache.velocity.Template;
 import org.apache.velocity.VelocityContext;
 import org.apache.velocity.app.Velocity;
@@ -70,8 +72,17 @@ public class DataSourceResourceLoaderTestCase
     /* engine with VARCHAR templates data source */
     private RuntimeInstance varcharTemplatesEngine = null;
 
-    /* engine with VARCHAR templates data source for testing connection counts*/
-    private RuntimeInstance varcharTemplatesConnectionCountTestEngine = null;
+    /* engine with VARCHAR templates data source for testing connection counts with DBCP2 data source*/
+    private RuntimeInstance varcharTemplatesDBCP2ConnectionCountTestEngine = null;
+    private BasicDataSource dbcp2ConnectionCountDataSource = null;
+
+    /* engine with VARCHAR templates data source for testing connection counts with Tomcat JDBC data source*/
+    private RuntimeInstance varcharTemplatesTomcatJDBCConnectionCountTestEngine = null;
+    private org.apache.tomcat.jdbc.pool.DataSource tomcatJDBCConnectionCountDataSource = null;
+
+    /* engine with VARCHAR templates data source for testing connection counts with Tomcat JDBC data source*/
+    private RuntimeInstance varcharTemplatesHikariConnectionCountTestEngine = null;
+    private HikariDataSource hikariConnectionCountDataSource = null;
 
     /* engine with CLOB templates data source */
     private RuntimeInstance clobTemplatesEngine = null;
@@ -97,37 +108,71 @@ public void setUp()
         DataSource ds1 = new TestDataSource(TEST_JDBC_DRIVER_CLASS, TEST_JDBC_URI, TEST_JDBC_LOGIN, TEST_JDBC_PASSWORD);
         DataSourceResourceLoader rl1 = new DataSourceResourceLoader();
         rl1.setDataSource(ds1);
-
-        DataSource ds2 = new TestDataSource(TEST_JDBC_DRIVER_CLASS, TEST_JDBC_URI, TEST_JDBC_LOGIN, TEST_JDBC_PASSWORD);
-        DataSourceResourceLoader rl2 = new DataSourceResourceLoader();
-        rl2.setDataSource(ds2);
-
-        DataSource ds3 = new TestDataSource(TEST_JDBC_DRIVER_CLASS, TEST_JDBC_URI, TEST_JDBC_LOGIN, TEST_JDBC_PASSWORD);
-        DataSourceResourceLoader rl3 = new DataSourceResourceLoader();
-        rl3.setDataSource(ds3);
-
         ExtProperties props = getResourceLoaderProperties();
         props.setProperty( "ds.resource.loader.instance", rl1);
         props.setProperty( "ds.resource.loader.resource.table", "velocity_template_varchar");
-
         varcharTemplatesEngine = new RuntimeInstance();
         varcharTemplatesEngine.setConfiguration(props);
         varcharTemplatesEngine.init();
 
+        DataSource ds2 = new TestDataSource(TEST_JDBC_DRIVER_CLASS, TEST_JDBC_URI, TEST_JDBC_LOGIN, TEST_JDBC_PASSWORD);
+        DataSourceResourceLoader rl2 = new DataSourceResourceLoader();
+        rl2.setDataSource(ds2);
         ExtProperties props2 = (ExtProperties)props.clone();
         props2.setProperty( "ds.resource.loader.instance", rl2);
         props2.setProperty( "ds.resource.loader.resource.table",  "velocity_template_clob");
         clobTemplatesEngine = new RuntimeInstance();
         clobTemplatesEngine.setConfiguration(props2);
         clobTemplatesEngine.init();
 
+        BasicDataSource ds3 = new BasicDataSource();
+        ds3.setDriverClassName(TEST_JDBC_DRIVER_CLASS);
+        ds3.setUrl(TEST_JDBC_URI);
+        ds3.setUsername(TEST_JDBC_LOGIN);
+        ds3.setPassword(TEST_JDBC_PASSWORD);
+        ds3.setMaxTotal(10);
+        DataSourceResourceLoader rl3 = new DataSourceResourceLoader();
+        rl3.setDataSource(ds3);
         ExtProperties props3 = getResourceLoaderProperties();
         props3.setProperty( "ds.resource.loader.instance", rl3);
         props3.setProperty( "ds.resource.loader.resource.table", "velocity_template_varchar");
-        props3.setProperty( "ds.resource.loader.database_objects_factory.class", "org.apache.velocity.test.sql.TestDefaultDatabaseObjectsFactory");
-        varcharTemplatesConnectionCountTestEngine = new RuntimeInstance();
-        varcharTemplatesConnectionCountTestEngine.setConfiguration(props3);
-        varcharTemplatesConnectionCountTestEngine.init();
+        varcharTemplatesDBCP2ConnectionCountTestEngine = new RuntimeInstance();
+        varcharTemplatesDBCP2ConnectionCountTestEngine.setConfiguration(props3);
+        varcharTemplatesDBCP2ConnectionCountTestEngine.init();
+        dbcp2ConnectionCountDataSource = ds3;
+
+        org.apache.tomcat.jdbc.pool.DataSource ds4 = new org.apache.tomcat.jdbc.pool.DataSource();
+        ds4.setDriverClassName(TEST_JDBC_DRIVER_CLASS);
+        ds4.setUrl(TEST_JDBC_URI);
+        ds4.setUsername(TEST_JDBC_LOGIN);
+        ds4.setPassword(TEST_JDBC_PASSWORD);
+        ds4.setMaxActive(10);
+        DataSourceResourceLoader rl4 = new DataSourceResourceLoader();
+        rl4.setDataSource(ds4);
+        ExtProperties props4 = getResourceLoaderProperties();
+        props4.setProperty( "ds.resource.loader.instance", rl4);
+        props4.setProperty( "ds.resource.loader.resource.table", "velocity_template_varchar");
+        varcharTemplatesTomcatJDBCConnectionCountTestEngine = new RuntimeInstance();
+        varcharTemplatesTomcatJDBCConnectionCountTestEngine.setConfiguration(props4);
+        varcharTemplatesTomcatJDBCConnectionCountTestEngine.init();
+        tomcatJDBCConnectionCountDataSource = ds4;
+
+        HikariDataSource ds5 = new HikariDataSource();
+        ds5.setDriverClassName(TEST_JDBC_DRIVER_CLASS);
+        ds5.setJdbcUrl(TEST_JDBC_URI);
+        ds5.setUsername(TEST_JDBC_LOGIN);
+        ds5.setPassword(TEST_JDBC_PASSWORD);
+        ds5.setMaximumPoolSize(10);
+        DataSourceResourceLoader rl5 = new DataSourceResourceLoader();
+        rl5.setDataSource(ds5);
+        ExtProperties props5 = getResourceLoaderProperties();
+        props5.setProperty( "ds.resource.loader.instance", rl5);
+        props5.setProperty( "ds.resource.loader.resource.table", "velocity_template_varchar");
+        varcharTemplatesHikariConnectionCountTestEngine = new RuntimeInstance();
+        varcharTemplatesHikariConnectionCountTestEngine.setConfiguration(props5);
+        varcharTemplatesHikariConnectionCountTestEngine.init();
+        hikariConnectionCountDataSource = ds5;
+
     }
 
     protected ExtProperties getResourceLoaderProperties()
@@ -155,19 +200,51 @@ public void testSimpleTemplate()
         assertFalse("Timestamp is 0", 0 == t.getLastModified());
     }
     /**
-     * Tests loading and rendering of a simple template and checks that there are no connection leaks.
+     * Tests loading and rendering of a simple template and checks that there are no connection leaks. Uses DBCP2 Data data source
+     */
+    public void testDBCP2DataSourceForConnectionLeaks()
+            throws Exception
+    {
+        executeTest("testTemplate1", varcharTemplatesDBCP2ConnectionCountTestEngine);
+        try {
+            varcharTemplatesDBCP2ConnectionCountTestEngine.getTemplate("fakeTemplate");
+            fail("Should have thrown exception ResourceNotFoundException");
+        } catch (ResourceNotFoundException e) {
+            //continue
+        }
+        assertEquals("Open connection count is greater then 0", 0, this.dbcp2ConnectionCountDataSource.getConnectionPool().getNumActive());
+    }
+
+    /**
+     * Tests loading and rendering of a simple template and checks that there are no connection leaks. Uses Tomcat JDBC data source
+     */
+    public void testTomcatJDBCDataSourceForConnectionLeaks()
+            throws Exception
+    {
+        executeTest("testTemplate1", varcharTemplatesTomcatJDBCConnectionCountTestEngine);
+        try {
+            varcharTemplatesTomcatJDBCConnectionCountTestEngine.getTemplate("fakeTemplate");
+            fail("Should have thrown exception ResourceNotFoundException");
+        } catch (ResourceNotFoundException e) {
+            //continue
+        }
+        assertEquals("Open connection count is greater then 0", 0, this.tomcatJDBCConnectionCountDataSource.getActive());
+    }
+
+    /**
+     * Tests loading and rendering of a simple template and checks that there are no connection leaks. Uses Tomcat JDBC data source
      */
-    public void testForConnectionLeaks()
+    public void testHikariCPDataSourceForConnectionLeaks()
             throws Exception
     {
-        executeTest("testTemplate1", varcharTemplatesConnectionCountTestEngine);
+        executeTest("testTemplate1", varcharTemplatesHikariConnectionCountTestEngine);
         try {
-            varcharTemplatesConnectionCountTestEngine.getTemplate("fakeTemplate");
+            varcharTemplatesHikariConnectionCountTestEngine.getTemplate("fakeTemplate");
             fail("Should have thrown exception ResourceNotFoundException");
         } catch (ResourceNotFoundException e) {
             //continue
         }
-        assertEquals("Open connection count is greater then 0", 0, TestDefaultDatabaseObjectsFactory.getConnectionCount());
+        assertEquals("Open connection count is greater then 0", 0, this.hikariConnectionCountDataSource.getHikariPoolMXBean().getActiveConnections());
     }
 
     public void testUnicode(RuntimeInstance engine)