Re: [PR] Add a statements pool to DatasourceResourceLoader to fix thread safety [velocity-engine]

[via GitHub]

michael-o commented on code in PR #49:
URL: https://github.com/apache/velocity-engine/pull/49#discussion_r1734400992


##########
velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java:
##########
@@ -19,9 +19,11 @@
  * under the License.
  */
 
+import org.apache.commons.pool2.ObjectPool;

Review Comment:
   Where is this class used here?



##########
velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java:
##########
@@ -209,6 +225,24 @@ else if (dataSourceName != null)
             log.error(msg);
             throw new RuntimeException(msg);
         }
+
+        String factoryClassName = 
configuration.getString("database_objects_factory.class");
+        if (factoryClassName == null)
+        {
+            factoryClassName = DATABASE_OBJECTS_FACTORY_DEFAULT_CLASS;
+        }
+        try
+        {
+            Class<?> factoryClass = ClassUtils.getClass(factoryClassName);
+            factory = (DatabaseObjectsFactory) 
factoryClass.getDeclaredConstructor().newInstance();
+            factory.init(dataSource, 
configuration.subset("database_objects_factory"));
+        }
+        catch (Exception e)
+        {
+            throw new Error("could not find database objects factory class", 
e);

Review Comment:
   Errors are reserved for the VM only, don't abuse them.



##########
velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DefaultDatabaseObjectsFactory.java:
##########
@@ -0,0 +1,53 @@
+package org.apache.velocity.runtime.resource.loader;
+
+import org.apache.velocity.util.ExtProperties;
+
+import javax.sql.DataSource;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+
+/**
+ * Database objects factory which will obtain a new connection from the data 
source and prepare needed statements
+ * at each call
+ */
+
+public class DefaultDatabaseObjectsFactory implements DatabaseObjectsFactory {
+
+    private DataSource dataSource;
+
+    /**
+     * Initialize the factory with the DataSourceResourceLoader properties
+     * @param dataSource data source
+     */
+    @Override
+    public void init(DataSource dataSource, ExtProperties properties)
+    {
+        this.dataSource = dataSource;
+    }
+
+    /**
+     * Prepare a statement
+     * @param sql Statement SQL
+     * @return prepared statement
+     */
+    @Override
+    public PreparedStatement prepareStatement(String sql) throws SQLException
+    {
+        Connection connection = dataSource.getConnection();
+        return connection.prepareStatement(sql);
+    }
+
+    /**
+     * Releases a prepared statement
+     * @param sql original sql query
+     * @param stmt statement
+     */
+    @Override
+    public void releaseStatement(String sql, PreparedStatement stmt) throws 
SQLException
+    {
+        Connection connection = stmt.getConnection();
+        stmt.close();

Review Comment:
   If this fails, the connection is never closed.



##########
velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/CachingDatabaseObjectsFactory.java:
##########
@@ -0,0 +1,161 @@
+package org.apache.velocity.runtime. resource.loader;
+
+import org.apache.commons.pool2.BasePooledObjectFactory;
+import org.apache.commons.pool2.PooledObject;
+import org.apache.commons.pool2.impl.DefaultPooledObject;
+import org.apache.commons.pool2.impl.GenericObjectPool;
+import org.apache.commons.pool2.impl.GenericObjectPoolConfig;
+import org.apache.velocity.util.ExtProperties;
+
+import javax.sql.DataSource;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * <p>Database objects factory which will keep a single connection to be able 
to cache statements preparation, by means
+ * of appropriate pools.</p>
+ * <p>This class requires the following optional dependency (maven syntax):</p>
+ * <pre><code>
+ *         &lt;dependency&gt;
+ *             &lt;groupId&gt;org.apache.commons&lt;/groupId&gt;
+ *             &lt;artifactId&gt;commons-pool2&lt;/artifactId&gt;
+ *             &lt;version&gt;2.12.0&lt;/version&gt;
+ *          &lt;/dependency&gt;
+ * </code></pre>
+ * <p>To use this class, you must add the following property to the example 
configuration described in
+ * @link{org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader}
+ * </p>
+ * <pre><code>
+ * resource.loader.ds.database_objects_factory.class = 
org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader<br>
+ * </code></pre>
+ * <p>The default size of each pool of prepared statements (there is one pool 
per statement) is 50. You can tune it
+ * with:</p>
+ * <pre><code>
+ * resource.loader.ds.database_objects_factory. = 
org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader<br>
+ * </code></pre>
+ * @see org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader
+ */
+
+public class CachingDatabaseObjectsFactory implements DatabaseObjectsFactory {
+
+    private static final String STATEMENTS_POOL_MAX_SIZE = 
"statements_pool_max_size";
+    private static final int STATEMENTS_POOL_MAX_SIZE_DEFAULT = 50;
+
+    private DataSource dataSource;
+    private Connection connection;
+    private int poolsMaxSize;
+    private Map<String, GenericObjectPool<PreparedStatement>> statementsCache 
= new HashMap<>();

Review Comment:
   This map is not thread safe. Why not use the KeyedGenericObjectPool straight 
away? It worked for me for the past ten years. The key will not change at 
runtime, will it?



##########
velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/CachingDatabaseObjectsFactory.java:
##########
@@ -0,0 +1,161 @@
+package org.apache.velocity.runtime. resource.loader;
+
+import org.apache.commons.pool2.BasePooledObjectFactory;
+import org.apache.commons.pool2.PooledObject;
+import org.apache.commons.pool2.impl.DefaultPooledObject;
+import org.apache.commons.pool2.impl.GenericObjectPool;
+import org.apache.commons.pool2.impl.GenericObjectPoolConfig;
+import org.apache.velocity.util.ExtProperties;
+
+import javax.sql.DataSource;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * <p>Database objects factory which will keep a single connection to be able 
to cache statements preparation, by means
+ * of appropriate pools.</p>
+ * <p>This class requires the following optional dependency (maven syntax):</p>
+ * <pre><code>
+ *         &lt;dependency&gt;
+ *             &lt;groupId&gt;org.apache.commons&lt;/groupId&gt;
+ *             &lt;artifactId&gt;commons-pool2&lt;/artifactId&gt;
+ *             &lt;version&gt;2.12.0&lt;/version&gt;
+ *          &lt;/dependency&gt;

Review Comment:
   Add the runtime scope here.



##########
velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DefaultDatabaseObjectsFactory.java:
##########
@@ -0,0 +1,53 @@
+package org.apache.velocity.runtime.resource.loader;
+
+import org.apache.velocity.util.ExtProperties;
+
+import javax.sql.DataSource;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+
+/**
+ * Database objects factory which will obtain a new connection from the data 
source and prepare needed statements
+ * at each call
+ */
+
+public class DefaultDatabaseObjectsFactory implements DatabaseObjectsFactory {
+
+    private DataSource dataSource;
+
+    /**
+     * Initialize the factory with the DataSourceResourceLoader properties
+     * @param dataSource data source
+     */
+    @Override
+    public void init(DataSource dataSource, ExtProperties properties)
+    {
+        this.dataSource = dataSource;
+    }
+
+    /**
+     * Prepare a statement
+     * @param sql Statement SQL
+     * @return prepared statement
+     */
+    @Override
+    public PreparedStatement prepareStatement(String sql) throws SQLException
+    {
+        Connection connection = dataSource.getConnection();
+        return connection.prepareStatement(sql);
+    }
+
+    /**
+     * Releases a prepared statement
+     * @param sql original sql query
+     * @param stmt statement
+     */
+    @Override
+    public void releaseStatement(String sql, PreparedStatement stmt) throws 
SQLException
+    {
+        Connection connection = stmt.getConnection();

Review Comment:
   Is it guaranteed that this is the same connection as in `prepareStatement`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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