Am Dienstag, den 07.06.2011, 10:34 +0100 schrieb Mark Thomas:
> On 06/06/2011 21:40, Felix Schumacher wrote:
> > Am Montag, den 06.06.2011, 17:56 +0100 schrieb Mark Thomas:
> >> On 06/06/2011 11:59, Keiichi Fujino wrote:
> >>> When jdbc-pool or DBCP is used as DB connection pool and
> >>> removeAbandoned is "true",
> >>> dbConnection is closed because dbConnection is not returned to the
> >>> connection pool.
> >>> And then, dbConnection is acquired again because dbConnection has been
> >>> already closed.
> >>>
> >>> Should I return the connection to the connection pool?
> >>> Or, is the DB connection cached until removeAbandoned works?
> >>
> >> The connection should be returned to the pool. If you could take care of
> >> that, that would be great. Sorry for missing that when I applied the patch.
> > It was a conscious decision not to return the connection to the pool for
> > the following reasons:
> > 1. the original code didn't do it
>
> The expected usage of pooled connections is that they are obtained from
> the pool, used and then immediately returned. Removing a connection from
> the pool and just keeping it defeats the point of having a pool.
There would be one other usage of jndi-resources, that would be to hide
password and db-user from the context.
>
> > 2. we would have to close the prepared statements in case of pooled
> > connections
>
> Yes, although DBCP can pool those too (if appropriately configured). You
> may wish to add something on that topic to the docs, if only a link the
> the relevant DBCP config docs.
done.
>
> > 3. the connection is not really abandoned, so one could argue, that you
> > should run the pool with removeAbandoned set to 'false'
>
> Yep, but as per 1, it isn't really following the expected conventions
> when using a connection pool.
>
> > But I would gladly provide a patch where connections are returned to the
> > pool.
>
> That would be great. Thanks.
I have attached it, but I could also upload it to the original bugzilla
entry.
I also corrected one of my previous error messages, which contained a
single-tick, which was not so good.
Felix
>
> Mark
>
> >
> > Felix
> >>
> >> Mark
> >>
> >>>
> >>>
> >>> 2011/6/4 <[email protected]>:
> >>>> Author: markt
> >>>> Date: Fri Jun 3 22:13:09 2011
> >>>> New Revision: 1131263
> >>>>
> >>>> URL: http://svn.apache.org/viewvc?rev=1131263&view=rev
> >>>> Log:
> >>>> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=51264
> >>>> Allow the JDBC persistent session store to use a JNDI datasource to
> >>>> define the database in which sessions are persisted.
> >>>> Patch provided by Felix Schumacher.
> >>>>
> >>>> Modified:
> >>>> tomcat/trunk/java/org/apache/catalina/session/JDBCStore.java
> >>>> tomcat/trunk/java/org/apache/catalina/session/LocalStrings.properties
> >>>> tomcat/trunk/webapps/docs/changelog.xml
> >>>> tomcat/trunk/webapps/docs/config/manager.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=1131263&r1=1131262&r2=1131263&view=diff
> >>>> ==============================================================================
> >>>> --- tomcat/trunk/java/org/apache/catalina/session/JDBCStore.java
> >>>> (original)
> >>>> +++ tomcat/trunk/java/org/apache/catalina/session/JDBCStore.java Fri Jun
> >>>> 3 22:13:09 2011
> >>>> @@ -33,6 +33,11 @@ import java.sql.SQLException;
> >>>> import java.util.ArrayList;
> >>>> import java.util.Properties;
> >>>>
> >>>> +import javax.naming.Context;
> >>>> +import javax.naming.InitialContext;
> >>>> +import javax.naming.NamingException;
> >>>> +import javax.sql.DataSource;
> >>>> +
> >>>> import org.apache.catalina.Container;
> >>>> import org.apache.catalina.LifecycleException;
> >>>> import org.apache.catalina.Loader;
> >>>> @@ -102,6 +107,16 @@ public class JDBCStore extends StoreBase
> >>>> */
> >>>> protected String driverName = null;
> >>>>
> >>>> + /**
> >>>> + * name of the JNDI resource
> >>>> + */
> >>>> + protected String dataSourceName = null;
> >>>> +
> >>>> + /**
> >>>> + * DataSource to use
> >>>> + */
> >>>> + protected DataSource dataSource = null;
> >>>> +
> >>>> // -------------------------------------------------------------
> >>>> Table & cols
> >>>>
> >>>> /**
> >>>> @@ -436,6 +451,27 @@ public class JDBCStore extends StoreBase
> >>>> return (this.sessionLastAccessedCol);
> >>>> }
> >>>>
> >>>> + /**
> >>>> + * Set the JNDI name of a DataSource-factory to use for db access
> >>>> + *
> >>>> + * @param dataSourceName The JNDI name of the DataSource-factory
> >>>> + */
> >>>> + public void setDataSourceName(String dataSourceName) {
> >>>> + if (dataSourceName == null || "".equals(dataSourceName.trim()))
> >>>> {
> >>>> + manager.getContainer().getLogger().warn(
> >>>> + sm.getString(getStoreName() +
> >>>> ".missingDataSourceName"));
> >>>> + return;
> >>>> + }
> >>>> + this.dataSourceName = dataSourceName;
> >>>> + }
> >>>> +
> >>>> + /**
> >>>> + * Return the name of the JNDI DataSource-factory
> >>>> + */
> >>>> + public String getDataSourceName() {
> >>>> + return this.dataSourceName;
> >>>> + }
> >>>> +
> >>>> // --------------------------------------------------------- Public
> >>>> Methods
> >>>>
> >>>> /**
> >>>> @@ -866,6 +902,24 @@ public class JDBCStore extends StoreBase
> >>>> if (dbConnection != null)
> >>>> return (dbConnection);
> >>>>
> >>>> + if (dataSourceName != null && dataSource == null) {
> >>>> + Context initCtx;
> >>>> + try {
> >>>> + initCtx = new InitialContext();
> >>>> + Context envCtx = (Context)
> >>>> initCtx.lookup("java:comp/env");
> >>>> + this.dataSource = (DataSource)
> >>>> envCtx.lookup(this.dataSourceName);
> >>>> + } catch (NamingException e) {
> >>>> + manager.getContainer().getLogger().error(
> >>>> + sm.getString(getStoreName() +
> >>>> ".wrongDataSource",
> >>>> + this.dataSourceName), e);
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + if (dataSource != null) {
> >>>> + dbConnection = dataSource.getConnection();
> >>>> + return dbConnection;
> >>>> + }
> >>>> +
> >>>> // Instantiate our database driver if necessary
> >>>> if (driver == null) {
> >>>> try {
> >>>>
> >>>> Modified:
> >>>> tomcat/trunk/java/org/apache/catalina/session/LocalStrings.properties
> >>>> URL:
> >>>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/session/LocalStrings.properties?rev=1131263&r1=1131262&r2=1131263&view=diff
> >>>> ==============================================================================
> >>>> ---
> >>>> tomcat/trunk/java/org/apache/catalina/session/LocalStrings.properties
> >>>> (original)
> >>>> +++
> >>>> tomcat/trunk/java/org/apache/catalina/session/LocalStrings.properties
> >>>> Fri Jun 3 22:13:09 2011
> >>>> @@ -27,6 +27,8 @@ JDBCStore.checkConnectionDBClosed=The da
> >>>> JDBCStore.checkConnectionDBReOpenFail=The re-open on the database
> >>>> failed. The database could be down.
> >>>> JDBCStore.checkConnectionSQLException=A SQL exception occurred {0}
> >>>> JDBCStore.checkConnectionClassNotFoundException=JDBC driver class not
> >>>> found {0}
> >>>> +JDBCStore.wrongDataSource=Can't open JNDI DataSource [{0}]
> >>>> +JDBCStore.missingDataSourceName=No valid JNDI name was given.
> >>>> managerBase.createRandom=Created random number generator for session ID
> >>>> generation in {0}ms.
> >>>> managerBase.createSession.ise=createSession: Too many active sessions
> >>>> managerBase.sessionTimeout=Invalid session timeout setting {0}
> >>>>
> >>>> Modified: tomcat/trunk/webapps/docs/changelog.xml
> >>>> URL:
> >>>> http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1131263&r1=1131262&r2=1131263&view=diff
> >>>> ==============================================================================
> >>>> --- tomcat/trunk/webapps/docs/changelog.xml (original)
> >>>> +++ tomcat/trunk/webapps/docs/changelog.xml Fri Jun 3 22:13:09 2011
> >>>> @@ -69,6 +69,11 @@
> >>>> without error. (markt)
> >>>> </fix>
> >>>> <fix>
> >>>> + <bug>51264</bug>: Allow the JDBC persistent session store to
> >>>> use a
> >>>> + JNDI datasource to define the database in which sessions are
> >>>> persisted.
> >>>> + Patch provided by Felix Schumacher. (markt)
> >>>> + </fix>
> >>>> + <fix>
> >>>> <bug>51274</bug>: Add missing i18n strings in
> >>>> PersistentManagerBase.
> >>>> Patch provided by Eiji Takahashi. (markt)
> >>>> </fix>
> >>>>
> >>>> Modified: tomcat/trunk/webapps/docs/config/manager.xml
> >>>> URL:
> >>>> http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/config/manager.xml?rev=1131263&r1=1131262&r2=1131263&view=diff
> >>>> ==============================================================================
> >>>> --- tomcat/trunk/webapps/docs/config/manager.xml (original)
> >>>> +++ tomcat/trunk/webapps/docs/config/manager.xml Fri Jun 3 22:13:09 2011
> >>>> @@ -356,6 +356,13 @@
> >>>> session table.</p>
> >>>> </attribute>
> >>>>
> >>>> + <attribute name="dataSourceName" required="false">
> >>>> + <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>
> >>>> + </attribute>
> >>>> +
> >>>> <attribute name="driverName" required="true">
> >>>> <p>Java class name of the JDBC driver to be used.</p>
> >>>> </attribute>
> >>>>
> >>>>
> >>>>
> >>>> ---------------------------------------------------------------------
> >>>> To unsubscribe, e-mail: [email protected]
> >>>> For additional commands, e-mail: [email protected]
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: [email protected]
> >> For additional commands, e-mail: [email protected]
> >>
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [email protected]
> > For additional commands, e-mail: [email protected]
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>
diff --git a/java/org/apache/catalina/session/JDBCStore.java b/java/org/apache/catalina/session/JDBCStore.java
index 7580c2b..a9d0cc4 100644
--- a/java/org/apache/catalina/session/JDBCStore.java
+++ b/java/org/apache/catalina/session/JDBCStore.java
@@ -874,11 +874,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 +889,7 @@ public class JDBCStore extends StoreBase {
ex.toString()));
}
- return dbConnection;
+ return _conn;
}
/**
@@ -916,8 +918,8 @@ public class JDBCStore extends StoreBase {
}
if (dataSource != null) {
- dbConnection = dataSource.getConnection();
- return dbConnection;
+ Connection _conn = dataSource.getConnection();
+ return _conn;
}
// Instantiate our database driver if necessary
@@ -1014,13 +1016,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);
+ }
}
/**
@@ -1034,7 +1038,13 @@ public class JDBCStore extends StoreBase {
protected synchronized void startInternal() throws LifecycleException {
// Open connection to the database
- this.dbConnection = getConnection();
+ Connection _conn = getConnection();
+ if (dataSource != null) {
+ // if connection was created by a dataSource, we will release it
+ close(_conn);
+ } else {
+ this.dbConnection = _conn;
+ }
super.startInternal();
}
diff --git a/java/org/apache/catalina/session/LocalStrings.properties b/java/org/apache/catalina/session/LocalStrings.properties
index b16f942..f7f6a6d 100644
--- a/java/org/apache/catalina/session/LocalStrings.properties
+++ b/java/org/apache/catalina/session/LocalStrings.properties
@@ -27,7 +27,7 @@ JDBCStore.checkConnectionDBClosed=The database connection is null or was found t
JDBCStore.checkConnectionDBReOpenFail=The re-open on the database failed. The database could be down.
JDBCStore.checkConnectionSQLException=A SQL exception occurred {0}
JDBCStore.checkConnectionClassNotFoundException=JDBC driver class not found {0}
-JDBCStore.wrongDataSource=Can't open JNDI DataSource [{0}]
+JDBCStore.wrongDataSource=Can not open JNDI DataSource [{0}]
JDBCStore.missingDataSourceName=No valid JNDI name was given.
managerBase.createRandom=Created random number generator for session ID generation in {0}ms.
managerBase.createSession.ise=createSession: Too many active sessions
diff --git a/webapps/docs/config/manager.xml b/webapps/docs/config/manager.xml
index f5b6bdc..2b9835e 100644
--- 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">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
