On 30 January 2014 21:54, <ma...@apache.org> wrote:
> Author: markt
> Date: Thu Jan 30 21:54:56 2014
> New Revision: 1562992
>
> URL: http://svn.apache.org/r1562992
> Log:
> Fix DBCP-369
> Fix threading issue when using multiple instances of the SharedPoolDataSource
> concurrently.
> I can't reproduce this but the issue can be seen from looking at the code.
>
> Modified:
> commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/changes/changes.xml
>
> commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/main/java/org/apache/commons/dbcp/datasources/InstanceKeyObjectFactory.java
>
> commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/test/java/org/apache/commons/dbcp/datasources/TestSharedPoolDataSource.java
>
> Modified:
> commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/changes/changes.xml
> URL:
> http://svn.apache.org/viewvc/commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/changes/changes.xml?rev=1562992&r1=1562991&r2=1562992&view=diff
> ==============================================================================
> --- commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/changes/changes.xml
> (original)
> +++ commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/changes/changes.xml
> Thu Jan 30 21:54:56 2014
> @@ -74,6 +74,10 @@ The <action> type attribute can be add,u
> Allow accessToUnderlyingConnectionAllowed to be configured when
> configuration takes place via JNDI in a JavaEE container.
> </action>
> + <action dev="markt" issue="DBCP-369" type="fix" due-to="Michael
> Pradel">
> + Fix threading issue when using multiple instances of the
> + SharedPoolDataSource concurrently.
> + </action>
> </release>
> <release version="1.4.1" date="TBD" description="TBD">
> <action dev="psteitz" issue="DBCP-367" type="fix" due-to="Ken
> Tatsushita">
>
> Modified:
> commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/main/java/org/apache/commons/dbcp/datasources/InstanceKeyObjectFactory.java
> URL:
> http://svn.apache.org/viewvc/commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/main/java/org/apache/commons/dbcp/datasources/InstanceKeyObjectFactory.java?rev=1562992&r1=1562991&r2=1562992&view=diff
> ==============================================================================
> ---
> commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/main/java/org/apache/commons/dbcp/datasources/InstanceKeyObjectFactory.java
> (original)
> +++
> commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/main/java/org/apache/commons/dbcp/datasources/InstanceKeyObjectFactory.java
> Thu Jan 30 21:54:56 2014
> @@ -14,7 +14,6 @@
> * See the License for the specific language governing permissions and
> * limitations under the License.
> */
> -
> package org.apache.commons.dbcp.datasources;
>
> import java.io.ByteArrayInputStream;
> @@ -23,9 +22,9 @@ import java.io.ObjectInputStream;
>
> import java.util.Hashtable;
> import java.util.Map;
> -import java.util.HashMap;
> import java.util.Iterator;
> import java.util.Properties;
> +import java.util.concurrent.ConcurrentHashMap;
>
> import javax.naming.Context;
> import javax.naming.Name;
> @@ -39,10 +38,9 @@ import javax.naming.spi.ObjectFactory;
> *
> * @version $Revision$ $Date$
> */
> -abstract class InstanceKeyObjectFactory
> - implements ObjectFactory
> -{
> - private static final Map instanceMap = new HashMap();
> +abstract class InstanceKeyObjectFactory implements ObjectFactory {
> +
> + private static final Map instanceMap = new ConcurrentHashMap();
Does not work - HashMap allows null as a key, but ConcurrentHashMap does not.
And the key can be null - not sure whether that is a feaure of the
unit tests or if it is allowed for regular use as well.
> synchronized static String registerNewInstance(InstanceKeyDataSource ds)
> {
> int max = 0;
>
> Modified:
> commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/test/java/org/apache/commons/dbcp/datasources/TestSharedPoolDataSource.java
> URL:
> http://svn.apache.org/viewvc/commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/test/java/org/apache/commons/dbcp/datasources/TestSharedPoolDataSource.java?rev=1562992&r1=1562991&r2=1562992&view=diff
> ==============================================================================
> ---
> commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/test/java/org/apache/commons/dbcp/datasources/TestSharedPoolDataSource.java
> (original)
> +++
> commons/proper/dbcp/branches/DBCP_1_5_x_BRANCH/src/test/java/org/apache/commons/dbcp/datasources/TestSharedPoolDataSource.java
> Thu Jan 30 21:54:56 2014
> @@ -5,9 +5,9 @@
> * 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.
> @@ -21,6 +21,7 @@ import java.sql.Connection;
> import java.sql.PreparedStatement;
> import java.sql.ResultSet;
> import java.sql.SQLException;
> +import java.util.ArrayList;
>
> import javax.sql.DataSource;
>
> @@ -79,14 +80,14 @@ public class TestSharedPoolDataSource ex
>
> /**
> * Switching 'u1 -> 'u2' and 'p1' -> 'p2' will
> - * exhibit the bug detailed in
> + * exhibit the bug detailed in
> * http://issues.apache.org/bugzilla/show_bug.cgi?id=18905
> - *
> + *
> * Starting with a successful connection, then incorrect password,
> * then correct password for same user illustrates
> * JIRA: DBCP-245
> */
> - public void testIncorrectPassword() throws Exception
> + public void testIncorrectPassword() throws Exception
> {
> ds.getConnection("u2", "p2").close();
> try {
> @@ -97,26 +98,26 @@ public class TestSharedPoolDataSource ex
> // should fail
>
> }
> -
> +
> // Use good password
> ds.getConnection("u1", "p1").close();
> - try
> + try
> {
> ds.getConnection("u1", "x");
> fail("Able to retrieve connection with incorrect password");
> }
> catch (SQLException e)
> {
> - if (!e.getMessage().startsWith("Given password did not match"))
> + if (!e.getMessage().startsWith("Given password did not match"))
> {
> throw e;
> }
> // else the exception was expected
> }
> -
> +
> // Make sure we can still use our good password.
> ds.getConnection("u1", "p1").close();
> -
> +
> // Try related users and passwords
> ds.getConnection("foo", "bar").close();
> try {
> @@ -132,7 +133,7 @@ public class TestSharedPoolDataSource ex
> }
>
>
> - public void testSimple() throws Exception
> + public void testSimple() throws Exception
> {
> Connection conn = ds.getConnection();
> assertNotNull(conn);
> @@ -146,7 +147,7 @@ public class TestSharedPoolDataSource ex
> conn.close();
> }
>
> - public void testSimpleWithUsername() throws Exception
> + public void testSimpleWithUsername() throws Exception
> {
> Connection conn = ds.getConnection("u1", "p1");
> assertNotNull(conn);
> @@ -160,12 +161,12 @@ public class TestSharedPoolDataSource ex
> conn.close();
> }
>
> - public void testClosingWithUserName()
> - throws Exception
> + public void testClosingWithUserName()
> + throws Exception
> {
> Connection[] c = new Connection[getMaxActive()];
> // open the maximum connections
> - for (int i=0; i<c.length; i++)
> + for (int i=0; i<c.length; i++)
> {
> c[i] = ds.getConnection("u1", "p1");
> }
> @@ -176,29 +177,29 @@ public class TestSharedPoolDataSource ex
> // get a new connection
> c[0] = ds.getConnection("u1", "p1");
>
> - for (int i=0; i<c.length; i++)
> + for (int i=0; i<c.length; i++)
> {
> c[i].close();
> }
>
> // open the maximum connections
> - for (int i=0; i<c.length; i++)
> + for (int i=0; i<c.length; i++)
> {
> c[i] = ds.getConnection("u1", "p1");
> }
> - for (int i=0; i<c.length; i++)
> + for (int i=0; i<c.length; i++)
> {
> c[i].close();
> }
> }
>
> - public void testSimple2()
> - throws Exception
> + public void testSimple2()
> + throws Exception
> {
> Connection conn = ds.getConnection();
> assertNotNull(conn);
>
> - PreparedStatement stmt =
> + PreparedStatement stmt =
> conn.prepareStatement("select * from dual");
> assertNotNull(stmt);
> ResultSet rset = stmt.executeQuery();
> @@ -206,7 +207,7 @@ public class TestSharedPoolDataSource ex
> assertTrue(rset.next());
> rset.close();
> stmt.close();
> -
> +
> stmt = conn.prepareStatement("select * from dual");
> assertNotNull(stmt);
> rset = stmt.executeQuery();
> @@ -214,14 +215,14 @@ public class TestSharedPoolDataSource ex
> assertTrue(rset.next());
> rset.close();
> stmt.close();
> -
> +
> conn.close();
> - try
> + try
> {
> conn.createStatement();
> fail("Can't use closed connections");
> - }
> - catch(SQLException e)
> + }
> + catch(SQLException e)
> {
> // expected
> }
> @@ -244,38 +245,38 @@ public class TestSharedPoolDataSource ex
> assertTrue(rset.next());
> rset.close();
> stmt.close();
> -
> +
> conn.close();
> conn = null;
> }
>
> - public void testOpening()
> - throws Exception
> + public void testOpening()
> + throws Exception
> {
> Connection[] c = new Connection[getMaxActive()];
> // test that opening new connections is not closing previous
> - for (int i=0; i<c.length; i++)
> + for (int i=0; i<c.length; i++)
> {
> c[i] = ds.getConnection();
> assertTrue(c[i] != null);
> - for (int j=0; j<=i; j++)
> + for (int j=0; j<=i; j++)
> {
> assertTrue(!c[j].isClosed());
> }
> }
>
> - for (int i=0; i<c.length; i++)
> + for (int i=0; i<c.length; i++)
> {
> c[i].close();
> }
> }
>
> - public void testClosing()
> - throws Exception
> + public void testClosing()
> + throws Exception
> {
> Connection[] c = new Connection[getMaxActive()];
> // open the maximum connections
> - for (int i=0; i<c.length; i++)
> + for (int i=0; i<c.length; i++)
> {
> c[i] = ds.getConnection();
> }
> @@ -283,36 +284,36 @@ public class TestSharedPoolDataSource ex
> // close one of the connections
> c[0].close();
> assertTrue(c[0].isClosed());
> -
> +
> // get a new connection
> c[0] = ds.getConnection();
>
> - for (int i=0; i<c.length; i++)
> + for (int i=0; i<c.length; i++)
> {
> c[i].close();
> }
> }
> -
> +
> /**
> * Test pool close. Illustrates BZ 37359.
> - *
> + *
> * @throws Exception
> */
> public void testClosePool() throws Exception {
> ((SharedPoolDataSource)ds).close();
> SharedPoolDataSource tds = new SharedPoolDataSource();
> - // NPE before BZ 37359 fix
> + // NPE before BZ 37359 fix
> tds.close();
> }
>
> - public void testMaxActive()
> - throws Exception
> + public void testMaxActive()
> + throws Exception
> {
> Connection[] c = new Connection[getMaxActive()];
> - for (int i=0; i<c.length; i++)
> + for (int i=0; i<c.length; i++)
> {
> c[i] = ds.getConnection();
> - assertTrue(c[i] != null);
> + assertTrue(c[i] != null);
> }
>
> try
> @@ -326,49 +327,49 @@ public class TestSharedPoolDataSource ex
> // throw an exception
> }
>
> - for (int i=0; i<c.length; i++)
> + for (int i=0; i<c.length; i++)
> {
> c[i].close();
> }
> }
> -
> +
> public void testMaxWait() throws Exception {
> final int maxWait = 1000;
> final int theadCount = 20;
> -
> +
> ((SharedPoolDataSource)ds).setMaxWait(maxWait);
> // Obtain all the connections from the pool
> Connection[] c = new Connection[getMaxActive()];
> for (int i=0; i<c.length; i++) {
> c[i] = ds.getConnection("foo","bar");
> - assertTrue(c[i] != null);
> + assertTrue(c[i] != null);
> }
>
> long start = System.currentTimeMillis();
> -
> +
> // Run a thread test with minimal hold time
> // All threads should end after maxWait - DBCP-291
> final PoolTest[] pts = new PoolTest[theadCount];
> ThreadGroup threadGroup = new ThreadGroup("testMaxWait");
>
> - // Should take ~maxWait for threads to stop
> + // Should take ~maxWait for threads to stop
> for (int i = 0; i < pts.length; i++) {
> (pts[i] = new PoolTest(threadGroup, 1, true)).start();
> }
> -
> +
> // Wait for all the threads to complete
> for (int i = 0; i < pts.length; i++) {
> final PoolTest poolTest = pts[i];
> poolTest.getThread().join();
> }
>
> -
> +
> long end = System.currentTimeMillis();
> -
> +
> System.out.println("testMaxWait took " + (end-start) + " ms.
> Maxwait: "+maxWait);
> -
> +
> // Threads should time out in parallel - allow double that to be safe
> - assertTrue((end-start) < (2 * maxWait));
> + assertTrue((end-start) < (2 * maxWait));
>
> // Put all the connections back in the pool
> for (int i=0; i<c.length; i++) {
> @@ -393,25 +394,25 @@ public class TestSharedPoolDataSource ex
> public void testTransactionIsolationBehavior() throws Exception {
> Connection conn = getConnection();
> assertNotNull(conn);
> - assertEquals(Connection.TRANSACTION_READ_COMMITTED,
> + assertEquals(Connection.TRANSACTION_READ_COMMITTED,
> conn.getTransactionIsolation());
>
> conn.setTransactionIsolation(Connection.TRANSACTION_READ_UNCOMMITTED);
> conn.close();
> -
> +
> Connection conn2 = getConnection();
> - assertEquals(Connection.TRANSACTION_READ_COMMITTED,
> + assertEquals(Connection.TRANSACTION_READ_COMMITTED,
> conn2.getTransactionIsolation());
> -
> +
> Connection conn3 = getConnection();
> - assertEquals(Connection.TRANSACTION_READ_COMMITTED,
> + assertEquals(Connection.TRANSACTION_READ_COMMITTED,
> conn3.getTransactionIsolation());
> conn2.close();
> conn3.close();
> - }
> + }
>
> - // Bugzilla Bug 24136 ClassCastException in DriverAdapterCPDS
> + // Bugzilla Bug 24136 ClassCastException in DriverAdapterCPDS
> // when setPoolPreparedStatements(true)
> - public void testPoolPrepareStatement() throws Exception
> + public void testPoolPrepareStatement() throws Exception
> {
> pcds.setPoolPreparedStatements(true);
>
> @@ -567,25 +568,66 @@ public class TestSharedPoolDataSource ex
> try {
> Connection con4 = ds.getConnection("foo", "bay"); // new password
> // Idle instances with old password should have been cleared
> - assertEquals("Should be no idle connections in the pool",
> + assertEquals("Should be no idle connections in the pool",
> 0, ((SharedPoolDataSource) ds).getNumIdle());
> con4.close();
> // Should be one idle instance with new pwd
> - assertEquals("Should be one idle connection in the pool",
> + assertEquals("Should be one idle connection in the pool",
> 1, ((SharedPoolDataSource) ds).getNumIdle());
> try {
> ds.getConnection("foo", "bar"); // old password
> - fail("Should have generated SQLException");
> + fail("Should have generated SQLException");
> } catch (SQLException expected) {
> }
> Connection con5 = ds.getConnection("foo", "bay"); // take the
> idle one
> con3.close(); // Return a connection with the old password
> ds.getConnection("foo", "bay").close(); // will try bad
> returned connection and destroy it
> - assertEquals("Should be one idle connection in the pool",
> + assertEquals("Should be one idle connection in the pool",
> 1, ((SharedPoolDataSource) ds).getNumIdle());
> con5.close();
> } finally {
> TesterDriver.addUser("foo","bar");
> }
> }
> +
> + public void testDbcp369() {
> + final ArrayList<SharedPoolDataSource> dataSources =
> + new ArrayList<SharedPoolDataSource>();
> + for (int j = 0; j < 10000; j++) {
> + SharedPoolDataSource dataSource = new SharedPoolDataSource();
> + dataSources.add(dataSource);
> + }
> +
> + Thread t1 = new Thread(new Runnable() {
> + @Override
> + public void run() {
> + for (SharedPoolDataSource dataSource : dataSources) {
> + dataSource.setDataSourceName("a");
> + }
> + }
> + });
> +
> + Thread t2 = new Thread(new Runnable() {
> + @Override
> + public void run() {
> + for (SharedPoolDataSource dataSource : dataSources) {
> + try {
> + dataSource.close();
> + } catch (Exception e) {
> + // Ignore
> + }
> + }
> + }
> + });
> +
> + t1.start();
> + t2.start();
> +
> + try {
> + t1.join();
> + t2.join();
> + } catch (InterruptedException ie) {
> + // Ignore
> + }
> + }
> }
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
