Author: kahatlen Date: Wed May 28 07:22:42 2008 New Revision: 660959 URL: http://svn.apache.org/viewvc?rev=660959&view=rev Log: DERBY-3401 (partial) Fixed ConcurrentModificationException if a statement event listener adds or removes a listener on the same pooled connection. Modified: db/derby/code/trunk/java/client/org/apache/derby/client/ClientPooledConnection40.java db/derby/code/trunk/java/client/org/apache/derby/client/ClientXAConnection40.java db/derby/code/trunk/java/engine/org/apache/derby/jdbc/EmbedPooledConnection40.java db/derby/code/trunk/java/engine/org/apache/derby/jdbc/EmbedXAConnection40.java db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementEventsTest.java Modified: db/derby/code/trunk/java/client/org/apache/derby/client/ClientPooledConnection40.java URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/client/org/apache/derby/client/ClientPooledConnection40.java?rev=660959&r1=660958&r2=660959&view=diff ============================================================================== --- db/derby/code/trunk/java/client/org/apache/derby/client/ClientPooledConnection40.java (original) +++ db/derby/code/trunk/java/client/org/apache/derby/client/ClientPooledConnection40.java Wed May 28 07:22:42 2008 @@ -23,7 +23,7 @@ import java.sql.PreparedStatement; import java.sql.SQLException; -import java.util.ArrayList; +import java.util.concurrent.CopyOnWriteArrayList; import javax.sql.StatementEventListener; import javax.sql.StatementEvent; import org.apache.derby.jdbc.ClientBaseDataSource; @@ -38,10 +38,14 @@ public class ClientPooledConnection40 extends ClientPooledConnection { - /** List of statement event listeners. */ - //@GuardedBy("this") - private final ArrayList statementEventListeners = - new ArrayList(); + /** + * List of statement event listeners. The list is copied on each write, + * ensuring that it can be safely iterated over even if other threads or + * the listeners fired in the same thread add or remove listeners. + */ + private final CopyOnWriteArrayList + statementEventListeners = + new CopyOnWriteArrayList(); public ClientPooledConnection40(ClientBaseDataSource ds, org.apache.derby.client.am.LogWriter logWriter, @@ -73,7 +77,7 @@ * interface and wants to be notified of Statement closed or * or Statement error occurred events */ - public synchronized void addStatementEventListener(StatementEventListener listener){ + public void addStatementEventListener(StatementEventListener listener) { if (logWriter_ != null) { logWriter_.traceEntry(this, "addStatementEventListener", listener); } @@ -89,7 +93,7 @@ * @param listener The previously registered event listener that needs to be * removed from the list of components */ - public synchronized void removeStatementEventListener(StatementEventListener listener){ + public void removeStatementEventListener(StatementEventListener listener) { if (logWriter_ != null) { logWriter_.traceEntry(this, "removeConnectionEventListener", listener); } @@ -104,7 +108,7 @@ * @param statement The PreparedStatement that was closed * */ - public synchronized void onStatementClose(PreparedStatement statement) { + public void onStatementClose(PreparedStatement statement) { if (!statementEventListeners.isEmpty()) { StatementEvent event = new StatementEvent(this,statement); for (StatementEventListener l : statementEventListeners) { @@ -123,9 +127,8 @@ * caused the invalidation of the PreparedStatements * */ - public synchronized void onStatementErrorOccurred( - PreparedStatement statement, - SQLException sqle) { + public void onStatementErrorOccurred(PreparedStatement statement, + SQLException sqle) { if (!statementEventListeners.isEmpty()) { StatementEvent event = new StatementEvent(this,statement,sqle); for (StatementEventListener l : statementEventListeners) { Modified: db/derby/code/trunk/java/client/org/apache/derby/client/ClientXAConnection40.java URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/client/org/apache/derby/client/ClientXAConnection40.java?rev=660959&r1=660958&r2=660959&view=diff ============================================================================== --- db/derby/code/trunk/java/client/org/apache/derby/client/ClientXAConnection40.java (original) +++ db/derby/code/trunk/java/client/org/apache/derby/client/ClientXAConnection40.java Wed May 28 07:22:42 2008 @@ -23,14 +23,9 @@ import java.sql.PreparedStatement; import java.sql.SQLException; -import java.util.Enumeration; -import java.util.Vector; +import java.util.concurrent.CopyOnWriteArrayList; import javax.sql.StatementEvent; import javax.sql.StatementEventListener; -import org.apache.derby.client.am.SqlException; -import org.apache.derby.client.net.NetLogWriter; -import org.apache.derby.client.net.NetXAConnection; -import org.apache.derby.jdbc.ClientDataSource; import org.apache.derby.jdbc.ClientXADataSource; /** @@ -38,10 +33,14 @@ */ public class ClientXAConnection40 extends ClientXAConnection { - //using generics to avoid casting problems - protected final Vector statementEventListeners = - new Vector(); - + /** + * List of statement event listeners. The list is copied on each write, + * ensuring that it can be safely iterated over even if other threads or + * the listeners fired in the same thread add or remove listeners. + */ + private final CopyOnWriteArrayList + statementEventListeners = + new CopyOnWriteArrayList(); /** * Constructor for ClientXAConnection40. @@ -73,7 +72,7 @@ if (logWriter_ != null) { logWriter_.traceEntry(this, "removeConnectionEventListener", listener); } - statementEventListeners.removeElement(listener); + statementEventListeners.remove(listener); } /** @@ -92,7 +91,7 @@ if (logWriter_ != null) { logWriter_.traceEntry(this, "addStatementEventListener", listener); } - statementEventListeners.addElement(listener); + statementEventListeners.add(listener); } /** @@ -103,12 +102,8 @@ public void onStatementClose(PreparedStatement statement) { if (!statementEventListeners.isEmpty()) { StatementEvent event = new StatementEvent(this,statement); - //synchronized block on statementEventListeners to make it thread - //safe - synchronized(statementEventListeners) { - for (StatementEventListener l : statementEventListeners) { - l.statementClosed(event); - } + for (StatementEventListener l : statementEventListeners) { + l.statementClosed(event); } } } @@ -127,12 +122,8 @@ SQLException sqle) { if (!statementEventListeners.isEmpty()) { StatementEvent event = new StatementEvent(this,statement,sqle); - //synchronized block on statementEventListeners to make it thread - //safe - synchronized(statementEventListeners) { - for (StatementEventListener l : statementEventListeners) { - l.statementErrorOccurred(event); - } + for (StatementEventListener l : statementEventListeners) { + l.statementErrorOccurred(event); } } } Modified: db/derby/code/trunk/java/engine/org/apache/derby/jdbc/EmbedPooledConnection40.java URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/engine/org/apache/derby/jdbc/EmbedPooledConnection40.java?rev=660959&r1=660958&r2=660959&view=diff ============================================================================== --- db/derby/code/trunk/java/engine/org/apache/derby/jdbc/EmbedPooledConnection40.java (original) +++ db/derby/code/trunk/java/engine/org/apache/derby/jdbc/EmbedPooledConnection40.java Wed May 28 07:22:42 2008 @@ -21,11 +21,9 @@ package org.apache.derby.jdbc; -import java.sql.Connection; import java.sql.SQLException; -import java.util.Enumeration; -import java.util.Vector; import java.sql.PreparedStatement; +import java.util.concurrent.CopyOnWriteArrayList; import javax.sql.StatementEvent; import javax.sql.StatementEventListener; @@ -41,11 +39,15 @@ */ class EmbedPooledConnection40 extends EmbedPooledConnection { - - //using generics to avoid casting problems - protected final Vector statementEventListeners = - new Vector(); - + + /** + * List of statement event listeners. The list is copied on each write, + * ensuring that it can be safely iterated over even if other threads or + * the listeners fired in the same thread add or remove listeners. + */ + private final CopyOnWriteArrayList + statementEventListeners = + new CopyOnWriteArrayList(); EmbedPooledConnection40 (ReferenceableDataSource ds, String user, String password, boolean requestPassword) throws SQLException { @@ -66,7 +68,7 @@ public void removeStatementEventListener(StatementEventListener listener) { if (listener == null) return; - statementEventListeners.removeElement(listener); + statementEventListeners.remove(listener); } /** @@ -89,7 +91,7 @@ return; if (listener == null) return; - statementEventListeners.addElement(listener); + statementEventListeners.add(listener); } /** @@ -100,12 +102,8 @@ public void onStatementClose(PreparedStatement statement) { if (!statementEventListeners.isEmpty()){ StatementEvent event = new StatementEvent(this,statement); - //synchronized block on statementEventListeners to make it thread - //safe - synchronized(statementEventListeners) { - for (StatementEventListener l : statementEventListeners) { - l.statementClosed(event); - } + for (StatementEventListener l : statementEventListeners) { + l.statementClosed(event); } } } @@ -119,12 +117,8 @@ public void onStatementErrorOccurred(PreparedStatement statement,SQLException sqle) { if (!statementEventListeners.isEmpty()){ StatementEvent event = new StatementEvent(this,statement,sqle); - //synchronized block on statementEventListeners to make it thread - //safe - synchronized(statementEventListeners) { - for (StatementEventListener l : statementEventListeners){ - l.statementErrorOccurred(event); - } + for (StatementEventListener l : statementEventListeners) { + l.statementErrorOccurred(event); } } } Modified: db/derby/code/trunk/java/engine/org/apache/derby/jdbc/EmbedXAConnection40.java URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/engine/org/apache/derby/jdbc/EmbedXAConnection40.java?rev=660959&r1=660958&r2=660959&view=diff ============================================================================== --- db/derby/code/trunk/java/engine/org/apache/derby/jdbc/EmbedXAConnection40.java (original) +++ db/derby/code/trunk/java/engine/org/apache/derby/jdbc/EmbedXAConnection40.java Wed May 28 07:22:42 2008 @@ -21,9 +21,9 @@ package org.apache.derby.jdbc; -import java.util.Vector; import java.sql.PreparedStatement; import java.sql.SQLException; +import java.util.concurrent.CopyOnWriteArrayList; import javax.sql.StatementEvent; import javax.sql.StatementEventListener; import javax.sql.XAConnection; @@ -35,9 +35,14 @@ final class EmbedXAConnection40 extends EmbedXAConnection implements XAConnection { - //using generics to avoid casting problems - protected final Vector statementEventListeners = - new Vector(); + /** + * List of statement event listeners. The list is copied on each write, + * ensuring that it can be safely iterated over even if other threads or + * the listeners fired in the same thread add or remove listeners. + */ + private final CopyOnWriteArrayList + statementEventListeners = + new CopyOnWriteArrayList(); /** * Creates EmbedXAConnection40. @@ -67,7 +72,7 @@ public void removeStatementEventListener(StatementEventListener listener) { if (listener == null) return; - statementEventListeners.removeElement(listener); + statementEventListeners.remove(listener); } /** @@ -90,7 +95,7 @@ return; if (listener == null) return; - statementEventListeners.addElement(listener); + statementEventListeners.add(listener); } /** @@ -101,12 +106,8 @@ public void onStatementClose(PreparedStatement statement) { if (!statementEventListeners.isEmpty()){ StatementEvent event = new StatementEvent(this,statement); - //synchronized block on statementEventListeners to make it thread - //safe - synchronized(statementEventListeners) { - for (StatementEventListener l : statementEventListeners) { - l.statementClosed(event); - } + for (StatementEventListener l : statementEventListeners) { + l.statementClosed(event); } } } @@ -120,12 +121,8 @@ public void onStatementErrorOccurred(PreparedStatement statement,SQLException sqle) { if (!statementEventListeners.isEmpty()){ StatementEvent event = new StatementEvent(this,statement,sqle); - //synchronized block on statementEventListeners to make it thread - //safe - synchronized(statementEventListeners) { - for (StatementEventListener l : statementEventListeners){ - l.statementErrorOccurred(event); - } + for (StatementEventListener l : statementEventListeners) { + l.statementErrorOccurred(event); } } } Modified: db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementEventsTest.java URL: http://svn.apache.org/viewvc/db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementEventsTest.java?rev=660959&r1=660958&r2=660959&view=diff ============================================================================== --- db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementEventsTest.java (original) +++ db/derby/code/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/StatementEventsTest.java Wed May 28 07:22:42 2008 @@ -22,6 +22,7 @@ package org.apache.derbyTesting.functionTests.tests.jdbc4; import java.sql.*; +import java.util.Arrays; import javax.sql.*; import junit.framework.*; @@ -273,4 +274,221 @@ assertEquals("Incorrect error count.", 1, errorCount); } } + + /** + * Test that removing a listener from a listener works. (DERBY-3401) + */ + public void testRemoveListenerFromListener() throws SQLException { + + // First element is number of times the close listeners below have + // been triggered, second element is number of times the error + // listeners have been triggered. + final int[] counters = new int[2]; + + // Add some listeners that remove themselves + for (int i = 0; i < 5; i++) { + StatementEventListener close = new StatementEventListener() { + + public void statementClosed(StatementEvent event) { + pooledConnection.removeStatementEventListener(this); + counters[0]++; + } + + public void statementErrorOccurred(StatementEvent event) { + } + }; + pooledConnection.addStatementEventListener(close); + + StatementEventListener error = new StatementEventListener() { + + public void statementClosed(StatementEvent event) { + } + + public void statementErrorOccurred(StatementEvent event) { + pooledConnection.removeStatementEventListener(this); + counters[1]++; + } + }; + pooledConnection.addStatementEventListener(error); + } + + // Generate close event twice. The close listeners remove themselves + // in the first iteration, so no updates of the counters are expected + // in the second iteration. + for (int i = 0; i < 2; i++) { + prepare("VALUES (1)").close(); + assertEquals("unexpected number of close events", 5, counters[0]); + assertEquals("unexpected number of error events", 0, counters[1]); + } + + // reset counters + Arrays.fill(counters, 0); + + // Generate error event twice. Only expect counters to be updated in + // the first iteration since the listeners remove themselves. + for (int i = 0; i < 2; i++) { + PreparedStatement ps = prepare("VALUES (1)"); + connection.close(); + try { + ps.execute(); + fail("Execute on closed connection should fail"); + } catch (SQLNonTransientConnectionException e) { + assertSQLState("08003", e); + } + assertEquals("unexpected number of close events", 0, counters[0]); + assertEquals("unexpected number of error events", 5, counters[1]); + connection = pooledConnection.getConnection(); + } + + // The listeners that are automatically added for all test cases have + // been active all the time. + assertEquals("Incorrect error count", 2, errorCount); + // Embedded doesn't receive close events when the connection is + // closed, whereas the client driver does. This is therefore an + // expected difference. + if (usingEmbedded()) { + assertEquals("Incorrect close count", 2, closedCount); + } else if (usingDerbyNetClient()) { + assertEquals("Incorrect close count", 4, closedCount); + } else { + fail("unknown framework"); + } + } + + /** + * Test that adding a listener from a listener works. (DERBY-3401) + */ + public void testAddListenerFromListener() throws SQLException { + + // First element is number of times the close listeners below have + // been triggered, second element is number of times the error + // listeners have been triggered. Third element is the number of + // times listeners added by close listeners have been triggered, + // fourth element is the number of times listeners added by error + // listeners have been triggered. + final int[] counters = new int[4]; + + // Add some listeners that add another listener + for (int i = 0; i < 5; i++) { + StatementEventListener close = new StatementEventListener() { + + public void statementClosed(StatementEvent event) { + counters[0]++; + pooledConnection.addStatementEventListener( + new StatementEventListener() { + public void statementClosed(StatementEvent e) { + counters[2]++; + } + public void statementErrorOccurred(StatementEvent e) { + counters[2]++; + } + }); + } + + public void statementErrorOccurred(StatementEvent event) { + } + }; + + pooledConnection.addStatementEventListener(close); + + StatementEventListener error = new StatementEventListener() { + + public void statementClosed(StatementEvent event) { + } + + public void statementErrorOccurred(StatementEvent event) { + counters[1]++; + pooledConnection.addStatementEventListener( + new StatementEventListener() { + public void statementClosed(StatementEvent e) { + counters[3]++; + } + public void statementErrorOccurred(StatementEvent e) { + counters[3]++; + } + }); + } + }; + + pooledConnection.addStatementEventListener(error); + } + + // Generate close event + prepare("VALUES (1)").close(); + assertEquals("unexpected number of close events", 5, counters[0]); + assertEquals("unexpected number of error events", 0, counters[1]); + assertEquals("unexpected number of added close listeners triggered", + 0, counters[2]); + assertEquals("unexpected number of added error listeners triggered", + 0, counters[3]); + + // Generate another close event + prepare("VALUES (1)").close(); + assertEquals("unexpected number of close events", 10, counters[0]); + assertEquals("unexpected number of error events", 0, counters[1]); + assertEquals("unexpected number of added close listeners triggered", + 5, counters[2]); + assertEquals("unexpected number of added error listeners triggered", + 0, counters[3]); + + // Generate a statement that doesn't work + PreparedStatement ps = prepare("VALUES (1)"); + connection.close(); + // reset counters + Arrays.fill(counters, 0); + + // Generate an error event + try { + ps.execute(); + fail("Execute on closed connection should fail"); + } catch (SQLNonTransientConnectionException e) { + assertSQLState("08003", e); + } + + assertEquals("unexpected number of close events", 0, counters[0]); + assertEquals("unexpected number of error events", 5, counters[1]); + // difference between embedded and client because client gets + // statement-closed event when the connection is closed, whereas + // embedded doesn't + assertEquals("unexpected number of added close listeners triggered", + usingEmbedded() ? 10 : 15, counters[2]); + assertEquals("unexpected number of added error listeners triggered", + 0, counters[3]); + + // reset counters + Arrays.fill(counters, 0); + + // Generate another error event, now with more listeners active + try { + ps.execute(); + fail("Execute on closed connection should fail"); + } catch (SQLNonTransientConnectionException e) { + assertSQLState("08003", e); + } + + assertEquals("unexpected number of close events", 0, counters[0]); + assertEquals("unexpected number of error events", 5, counters[1]); + // difference between embedded and client because client gets + // statement-closed event when the connection is closed, whereas + // embedded doesn't + assertEquals("unexpected number of added close listeners triggered", + usingEmbedded() ? 10 : 15, counters[2]); + assertEquals("unexpected number of added error listeners triggered", + 5, counters[3]); + + // The listeners that are automatically added for all test cases have + // been active all the time. + assertEquals("Incorrect error count", 2, errorCount); + // Embedded doesn't receive close events when the connection is + // closed, whereas the client driver does. This is therefore an + // expected difference. + if (usingEmbedded()) { + assertEquals("Incorrect close count", 2, closedCount); + } else if (usingDerbyNetClient()) { + assertEquals("Incorrect close count", 3, closedCount); + } else { + fail("unknown framework"); + } + } + }