From 35dd9809915243be62d04ca7c4242eafae7ca439 Mon Sep 17 00:00:00 2001 From: Chris Rankin Date: Tue, 1 Apr 2014 22:16:48 +0100 Subject: [PATCH 1/2] Closing a connection that has usageCount > 1 "should never happen". However, it can happen if JdbcPooledConnection.getConnectionHandle() throws an exception - e.g. if testConnection() fails. So ensure that JdbcPooledConnection.getConnectionHandle() decrements usageCount again if it fails, and sets the connection's state to NOT_ACCESSIBLE. This fix is currently only for "correctness" w.r.t. JdbcPooledConnection's documented semantics. The only visible effect is seeing fewer warnings in the log. --- .../resource/jdbc/JdbcPooledConnection.java | 59 +++++++++++-------- 1 file changed, 33 insertions(+), 26 deletions(-) diff --git a/btm/src/main/java/bitronix/tm/resource/jdbc/JdbcPooledConnection.java b/btm/src/main/java/bitronix/tm/resource/jdbc/JdbcPooledConnection.java index d3dd235a..be0ec876 100644 --- a/btm/src/main/java/bitronix/tm/resource/jdbc/JdbcPooledConnection.java +++ b/btm/src/main/java/bitronix/tm/resource/jdbc/JdbcPooledConnection.java @@ -270,35 +270,42 @@ public Object getConnectionHandle() throws Exception { // Increment the usage count usageCount++; + try { + // Only transition to STATE_ACCESSIBLE on the first usage. If we're not sharing + // connections (default behavior) usageCount is always 1 here, so this transition + // will always occur (current behavior unchanged). If we _are_ sharing connections, + // and this is _not_ the first usage, it is valid for the state to already be + // STATE_ACCESSIBLE. Calling setState() with STATE_ACCESSIBLE when the state is + // already STATE_ACCESSIBLE fails the sanity check in AbstractXAStatefulHolder. + // Even if the connection is shared (usageCount > 1), if the state was STATE_NOT_ACCESSIBLE + // we transition back to STATE_ACCESSIBLE. + if (usageCount == 1 || oldState == STATE_NOT_ACCESSIBLE) { + setState(STATE_ACCESSIBLE); + } - // Only transition to STATE_ACCESSIBLE on the first usage. If we're not sharing - // connections (default behavior) usageCount is always 1 here, so this transition - // will always occur (current behavior unchanged). If we _are_ sharing connections, - // and this is _not_ the first usage, it is valid for the state to already be - // STATE_ACCESSIBLE. Calling setState() with STATE_ACCESSIBLE when the state is - // already STATE_ACCESSIBLE fails the sanity check in AbstractXAStatefulHolder. - // Even if the connection is shared (usageCount > 1), if the state was STATE_NOT_ACCESSIBLE - // we transition back to STATE_ACCESSIBLE. - if (usageCount == 1 || oldState == STATE_NOT_ACCESSIBLE) { - setState(STATE_ACCESSIBLE); - } - - if (oldState == STATE_IN_POOL) { - if (log.isDebugEnabled()) { log.debug("connection " + xaConnection + " was in state IN_POOL, testing it"); } - testConnection(connection); - applyIsolationLevel(); - applyCursorHoldabilty(); - if (TransactionContextHelper.currentTransaction() == null) { - // it is safe to set the auto-commit flag outside of a global transaction - applyLocalAutoCommit(); + if (oldState == STATE_IN_POOL) { + if (log.isDebugEnabled()) { log.debug("connection " + xaConnection + " was in state IN_POOL, testing it"); } + testConnection(connection); + applyIsolationLevel(); + applyCursorHoldabilty(); + if (TransactionContextHelper.currentTransaction() == null) { + // it is safe to set the auto-commit flag outside of a global transaction + applyLocalAutoCommit(); + } + } + else { + if (log.isDebugEnabled()) { log.debug("connection " + xaConnection + " was in state " + Decoder.decodeXAStatefulHolderState(oldState) + ", no need to test it"); } } - } - else { - if (log.isDebugEnabled()) { log.debug("connection " + xaConnection + " was in state " + Decoder.decodeXAStatefulHolderState(oldState) + ", no need to test it"); } - } - if (log.isDebugEnabled()) { log.debug("got connection handle from " + this); } - return getConnectionHandle(connection); + if (log.isDebugEnabled()) { log.debug("got connection handle from " + this); } + return getConnectionHandle(connection); + } catch (Exception e) { + // This connection must be invalid, so make it inaccessible and revert the usage counter. + // Note: Closing a handle with usageCount > 0 "should never happen". + setState(STATE_NOT_ACCESSIBLE); + --usageCount; + throw e; + } } public void stateChanged(XAStatefulHolder source, int oldState, int newState) { From b27976573f2d94579e9c3689164fb925fcccc95e Mon Sep 17 00:00:00 2001 From: Chris Rankin Date: Mon, 26 May 2014 15:39:13 +0100 Subject: [PATCH 2/2] Fix some straggling JDK6 issues. Fix references to XA states in comments, and remove unused import. More final variables and @Override annotations. Add generic types to List and Iterator. --- .../jdbc/proxy/ConnectionJavaProxy.java | 8 ++++--- .../jdbc/proxy/LrcXAConnectionJavaProxy.java | 3 +-- .../proxy/PreparedStatementJavaProxy.java | 1 + .../tm/resource/jms/DualSessionWrapper.java | 2 +- .../tm/resource/jms/JmsPooledConnection.java | 4 ++-- .../java/bitronix/tm/timer/TaskScheduler.java | 1 + .../bitronix/tm/ExceptionAnalyzerTest.java | 4 +++- .../java/bitronix/tm/JdbcFailedPoolTest.java | 4 +++- .../test/java/bitronix/tm/RestartTest.java | 6 ++--- .../java/bitronix/tm/mock/JdbcPoolTest.java | 9 ++++---- .../java/bitronix/tm/mock/JmsPoolTest.java | 5 +++-- .../tm/mock/NewJdbcProperUsageMockTest.java | 5 +++-- .../tm/mock/NewJdbcStrangeUsageMockTest.java | 3 ++- .../java/bitronix/tm/mock/events/Event.java | 8 +++---- .../tm/mock/events/EventRecorder.java | 8 +++---- .../bitronix/tm/twopc/Phase2FailureTest.java | 22 +++++++------------ 16 files changed, 49 insertions(+), 44 deletions(-) diff --git a/btm/src/main/java/bitronix/tm/resource/jdbc/proxy/ConnectionJavaProxy.java b/btm/src/main/java/bitronix/tm/resource/jdbc/proxy/ConnectionJavaProxy.java index 4ec266a2..085d2728 100644 --- a/btm/src/main/java/bitronix/tm/resource/jdbc/proxy/ConnectionJavaProxy.java +++ b/btm/src/main/java/bitronix/tm/resource/jdbc/proxy/ConnectionJavaProxy.java @@ -64,16 +64,18 @@ void initialize(JdbcPooledConnection jdbcPooledConnection, Connection connection } @Override - public String toString() { + public String toString() { return "a ConnectionJavaProxy of " + jdbcPooledConnection + " on " + delegate; } /* PooledConnectionProxy interface methods */ + @Override public JdbcPooledConnection getPooledConnection() { return jdbcPooledConnection; } + @Override public Connection getProxiedDelegate() { return delegate; } @@ -383,9 +385,9 @@ private void enlistResource() throws SQLException { try { TransactionContextHelper.enlistInCurrentTransaction(jdbcPooledConnection); } catch (SystemException ex) { - throw (SQLException) new SQLException("error enlisting " + this).initCause(ex); + throw new SQLException("error enlisting " + this, ex); } catch (RollbackException ex) { - throw (SQLException) new SQLException("error enlisting " + this).initCause(ex); + throw new SQLException("error enlisting " + this, ex); } } // if getAutomaticEnlistingEnabled } diff --git a/btm/src/main/java/bitronix/tm/resource/jdbc/proxy/LrcXAConnectionJavaProxy.java b/btm/src/main/java/bitronix/tm/resource/jdbc/proxy/LrcXAConnectionJavaProxy.java index 8e68f2f1..012fc4ad 100644 --- a/btm/src/main/java/bitronix/tm/resource/jdbc/proxy/LrcXAConnectionJavaProxy.java +++ b/btm/src/main/java/bitronix/tm/resource/jdbc/proxy/LrcXAConnectionJavaProxy.java @@ -75,8 +75,7 @@ public void removeConnectionEventListener(ConnectionEventListener listener) { private void fireCloseEvent() { if (log.isDebugEnabled()) { log.debug("notifying " + connectionEventListeners.size() + " connectionEventListeners(s) about closing of " + this); } - for (int i = 0; i < connectionEventListeners.size(); i++) { - ConnectionEventListener connectionEventListener = connectionEventListeners.get(i); + for (ConnectionEventListener connectionEventListener : connectionEventListeners) { connectionEventListener.connectionClosed(new ConnectionEvent((PooledConnection) delegate)); } } diff --git a/btm/src/main/java/bitronix/tm/resource/jdbc/proxy/PreparedStatementJavaProxy.java b/btm/src/main/java/bitronix/tm/resource/jdbc/proxy/PreparedStatementJavaProxy.java index be8bd15c..a2c67950 100644 --- a/btm/src/main/java/bitronix/tm/resource/jdbc/proxy/PreparedStatementJavaProxy.java +++ b/btm/src/main/java/bitronix/tm/resource/jdbc/proxy/PreparedStatementJavaProxy.java @@ -51,6 +51,7 @@ void initialize(JdbcPooledConnection jdbcPooledConnection, PreparedStatement sta this.pretendClosed = false; } + @Override public String toString() { return "a PreparedStatementJavaProxy wrapping [" + delegate + "]"; } diff --git a/btm/src/main/java/bitronix/tm/resource/jms/DualSessionWrapper.java b/btm/src/main/java/bitronix/tm/resource/jms/DualSessionWrapper.java index 405fe21c..e3776afe 100644 --- a/btm/src/main/java/bitronix/tm/resource/jms/DualSessionWrapper.java +++ b/btm/src/main/java/bitronix/tm/resource/jms/DualSessionWrapper.java @@ -249,7 +249,7 @@ else if (newState == State.CLOSED) { } messageConsumers.clear(); - } // if newState == STATE_CLOSED + } // if newState == State.CLOSED } @Override diff --git a/btm/src/main/java/bitronix/tm/resource/jms/JmsPooledConnection.java b/btm/src/main/java/bitronix/tm/resource/jms/JmsPooledConnection.java index c11cc9f7..a73b7b22 100644 --- a/btm/src/main/java/bitronix/tm/resource/jms/JmsPooledConnection.java +++ b/btm/src/main/java/bitronix/tm/resource/jms/JmsPooledConnection.java @@ -246,7 +246,7 @@ public Collection getTransactionGtridsCurrentlyHoldingThis() { /** * {@link JmsPooledConnection} {@link bitronix.tm.resource.common.StateChangeListener}. - * When state changes to STATE_CLOSED, the conenction is unregistered from + * When state changes to State.CLOSED, the connection is unregistered from * {@link bitronix.tm.utils.ManagementRegistrar}. */ private final class JmsPooledConnectionStateChangeListener implements StateChangeListener { @@ -271,7 +271,7 @@ public void stateChanging(XAStatefulHolder source, State currentState, State fut /** * {@link JmsConnectionHandle} {@link bitronix.tm.resource.common.StateChangeListener}. - * When state changes to STATE_CLOSED, the session is removed from the list of opened sessions. + * When state changes to State.CLOSED, the session is removed from the list of opened sessions. */ private final class JmsConnectionHandleStateChangeListener implements StateChangeListener { @Override diff --git a/btm/src/main/java/bitronix/tm/timer/TaskScheduler.java b/btm/src/main/java/bitronix/tm/timer/TaskScheduler.java index 3db906c9..2b354b07 100644 --- a/btm/src/main/java/bitronix/tm/timer/TaskScheduler.java +++ b/btm/src/main/java/bitronix/tm/timer/TaskScheduler.java @@ -103,6 +103,7 @@ public int countTasksQueued() { } } + @Override public void shutdown() { boolean wasActive = setActive(false); diff --git a/btm/src/test/java/bitronix/tm/ExceptionAnalyzerTest.java b/btm/src/test/java/bitronix/tm/ExceptionAnalyzerTest.java index 6b3d6729..6c02143a 100644 --- a/btm/src/test/java/bitronix/tm/ExceptionAnalyzerTest.java +++ b/btm/src/test/java/bitronix/tm/ExceptionAnalyzerTest.java @@ -40,13 +40,15 @@ public void testExceptionAnalyzer() throws Exception { TransactionManagerServices.getConfiguration().setExceptionAnalyzer(TestExceptionAnalyzer.class.getName()); assertEquals(TestExceptionAnalyzer.class, TransactionManagerServices.getExceptionAnalyzer().getClass()); } - + public static class TestExceptionAnalyzer implements ExceptionAnalyzer { + @Override public String extractExtraXAExceptionDetails(XAException ex) { return ""; } + @Override public void shutdown() { } } diff --git a/btm/src/test/java/bitronix/tm/JdbcFailedPoolTest.java b/btm/src/test/java/bitronix/tm/JdbcFailedPoolTest.java index 53191126..7f006dd3 100644 --- a/btm/src/test/java/bitronix/tm/JdbcFailedPoolTest.java +++ b/btm/src/test/java/bitronix/tm/JdbcFailedPoolTest.java @@ -31,15 +31,17 @@ */ public class JdbcFailedPoolTest extends TestCase { + @Override protected void setUp() throws Exception { TransactionManagerServices.getJournal().open(); TransactionManagerServices.getTaskScheduler(); } + @Override protected void tearDown() throws Exception { TransactionManagerServices.getJournal().close(); TransactionManagerServices.getTaskScheduler().shutdown(); - + MockitoXADataSource.setStaticGetXAConnectionException(null); } diff --git a/btm/src/test/java/bitronix/tm/RestartTest.java b/btm/src/test/java/bitronix/tm/RestartTest.java index e78ca55b..4f18bc38 100644 --- a/btm/src/test/java/bitronix/tm/RestartTest.java +++ b/btm/src/test/java/bitronix/tm/RestartTest.java @@ -29,11 +29,11 @@ */ public class RestartTest extends TestCase { - + @Override protected void setUp() throws Exception { - Iterator it = ResourceRegistrar.getResourcesUniqueNames().iterator(); + Iterator it = ResourceRegistrar.getResourcesUniqueNames().iterator(); while (it.hasNext()) { - String name = (String) it.next(); + String name = it.next(); ResourceRegistrar.unregister(ResourceRegistrar.get(name)); } } diff --git a/btm/src/test/java/bitronix/tm/mock/JdbcPoolTest.java b/btm/src/test/java/bitronix/tm/mock/JdbcPoolTest.java index 6232c194..9f02b72d 100644 --- a/btm/src/test/java/bitronix/tm/mock/JdbcPoolTest.java +++ b/btm/src/test/java/bitronix/tm/mock/JdbcPoolTest.java @@ -46,6 +46,7 @@ public class JdbcPoolTest extends TestCase { private final static Logger log = LoggerFactory.getLogger(JdbcPoolTest.class); private PoolingDataSource pds; + @Override protected void setUp() throws Exception { TransactionManagerServices.getConfiguration().setJournal("null").setGracefulShutdownInterval(2); TransactionManagerServices.getTransactionManager(); @@ -64,11 +65,11 @@ protected void setUp() throws Exception { pds.init(); } - + @Override protected void tearDown() throws Exception { pds.close(); - TransactionManagerServices.getTransactionManager().shutdown(); + TransactionManagerServices.getTransactionManager().shutdown(); } public void testObjectProperties() throws Exception { @@ -186,7 +187,7 @@ public void testPoolShrink() throws Exception { TransactionManagerServices.getTaskScheduler().interrupt(); // wake up the task scheduler Thread.sleep(1200); // leave enough time for the scheduled shrinking task to do its work - if (log.isDebugEnabled()) { log.debug("*** checking pool sizes"); } + if (log.isDebugEnabled()) { log.debug("*** checking pool sizes"); } assertEquals(1, pool.inPoolSize()); assertEquals(1, pool.totalPoolSize()); } @@ -395,7 +396,7 @@ public void testPoolNotStartingTransactionManager() throws Exception { if (log.isDebugEnabled()) { log.debug("*** Starting testPoolNotStartingTransactionManager"); } // make sure TM is not running TransactionManagerServices.getTransactionManager().shutdown(); - + PoolingDataSource pds = new PoolingDataSource(); pds.setMinPoolSize(1); pds.setMaxPoolSize(2); diff --git a/btm/src/test/java/bitronix/tm/mock/JmsPoolTest.java b/btm/src/test/java/bitronix/tm/mock/JmsPoolTest.java index 6d9781d8..b4578507 100644 --- a/btm/src/test/java/bitronix/tm/mock/JmsPoolTest.java +++ b/btm/src/test/java/bitronix/tm/mock/JmsPoolTest.java @@ -37,6 +37,7 @@ public class JmsPoolTest extends TestCase { private PoolingConnectionFactory pcf; + @Override protected void setUp() throws Exception { TransactionManagerServices.getConfiguration().setJournal("null").setGracefulShutdownInterval(2); TransactionManagerServices.getTransactionManager(); @@ -55,7 +56,7 @@ protected void setUp() throws Exception { pcf.init(); } - + @Override protected void tearDown() throws Exception { pcf.close(); @@ -248,5 +249,5 @@ public void testPoolResetErrorHandling() throws Exception { pcf.reset(); assertEquals(1, pool.inPoolSize()); } - + } diff --git a/btm/src/test/java/bitronix/tm/mock/NewJdbcProperUsageMockTest.java b/btm/src/test/java/bitronix/tm/mock/NewJdbcProperUsageMockTest.java index 8eeb1241..88d71347 100644 --- a/btm/src/test/java/bitronix/tm/mock/NewJdbcProperUsageMockTest.java +++ b/btm/src/test/java/bitronix/tm/mock/NewJdbcProperUsageMockTest.java @@ -1044,8 +1044,8 @@ static class LooseTransactionThread extends Thread { static int successes = 0; static int failures = 0; - private int number; - private PoolingDataSource poolingDataSource; + private final int number; + private final PoolingDataSource poolingDataSource; private boolean succesful = false; public LooseTransactionThread(int number, PoolingDataSource poolingDataSource) { @@ -1053,6 +1053,7 @@ public LooseTransactionThread(int number, PoolingDataSource poolingDataSource) { this.poolingDataSource = poolingDataSource; } + @Override public void run() { try { UserTransaction ut = TransactionManagerServices.getTransactionManager(); diff --git a/btm/src/test/java/bitronix/tm/mock/NewJdbcStrangeUsageMockTest.java b/btm/src/test/java/bitronix/tm/mock/NewJdbcStrangeUsageMockTest.java index f9f7c3bc..049c4f57 100644 --- a/btm/src/test/java/bitronix/tm/mock/NewJdbcStrangeUsageMockTest.java +++ b/btm/src/test/java/bitronix/tm/mock/NewJdbcStrangeUsageMockTest.java @@ -19,6 +19,7 @@ import bitronix.tm.TransactionManagerServices; import bitronix.tm.mock.events.ConnectionDequeuedEvent; import bitronix.tm.mock.events.ConnectionQueuedEvent; +import bitronix.tm.mock.events.Event; import bitronix.tm.mock.events.EventRecorder; import bitronix.tm.mock.events.JournalLogEvent; import bitronix.tm.mock.events.XAResourceCommitEvent; @@ -301,7 +302,7 @@ public void testClosingSuspendedConnectionsInDifferentContext() throws Exception assertEquals(POOL_SIZE, pool1.inPoolSize()); // check flow - List orderedEvents = EventRecorder.getOrderedEvents(); + List orderedEvents = EventRecorder.getOrderedEvents(); log.info(EventRecorder.dumpToString()); assertEquals(18, orderedEvents.size()); diff --git a/btm/src/test/java/bitronix/tm/mock/events/Event.java b/btm/src/test/java/bitronix/tm/mock/events/Event.java index 368f242f..ebcc581b 100644 --- a/btm/src/test/java/bitronix/tm/mock/events/Event.java +++ b/btm/src/test/java/bitronix/tm/mock/events/Event.java @@ -21,10 +21,10 @@ */ public abstract class Event { - private Exception callStack; - private Object source; - private Exception exception; - private long timestamp; + private final Exception callStack; + private final Object source; + private final Exception exception; + private final long timestamp; protected Event(Object source, Exception ex) { this.callStack = new Exception(); diff --git a/btm/src/test/java/bitronix/tm/mock/events/EventRecorder.java b/btm/src/test/java/bitronix/tm/mock/events/EventRecorder.java index f8a23dd4..dda898ba 100644 --- a/btm/src/test/java/bitronix/tm/mock/events/EventRecorder.java +++ b/btm/src/test/java/bitronix/tm/mock/events/EventRecorder.java @@ -42,12 +42,12 @@ public static Map getEventRecorders() { return eventRecorders; } - public static Iterator iterateEvents() { + public static Iterator iterateEvents() { return new EventsIterator(eventRecorders); } - public static List getOrderedEvents() { - Iterator iterator = iterateEvents(); + public static List getOrderedEvents() { + Iterator iterator = iterateEvents(); List orderedEvents = new ArrayList(); while (iterator.hasNext()) { Event ev = iterator.next(); @@ -60,7 +60,7 @@ public static String dumpToString() { StringBuilder sb = new StringBuilder(); int i = 0; - Iterator it = iterateEvents(); + Iterator it = iterateEvents(); while (it.hasNext()) { Event event = it.next(); sb.append(i++); diff --git a/btm/src/test/java/bitronix/tm/twopc/Phase2FailureTest.java b/btm/src/test/java/bitronix/tm/twopc/Phase2FailureTest.java index 0cefb847..5b63efa5 100644 --- a/btm/src/test/java/bitronix/tm/twopc/Phase2FailureTest.java +++ b/btm/src/test/java/bitronix/tm/twopc/Phase2FailureTest.java @@ -98,10 +98,8 @@ public void testExpectNoHeuristic() throws Exception { int journalCommittingEventCount = 0; int journalCommittedEventCount = 0; int commitEventCount = 0; - List events = EventRecorder.getOrderedEvents(); - for (int i = 0; i < events.size(); i++) { - Event event = (Event) events.get(i); - + List events = EventRecorder.getOrderedEvents(); + for (Event event : events) { if (event instanceof XAResourceCommitEvent) commitEventCount++; @@ -168,10 +166,8 @@ public void testHeuristicCommit() throws Exception { int journalCommittedEventCount = 0; int commitEventCount = 0; int forgetEventCount = 0; - List events = EventRecorder.getOrderedEvents(); - for (int i = 0; i < events.size(); i++) { - Event event = (Event) events.get(i); - + List events = EventRecorder.getOrderedEvents(); + for (Event event : events) { if (event instanceof XAResourceCommitEvent) commitEventCount++; @@ -235,10 +231,8 @@ public void testHeuristicMixed() throws Exception { int journalUnknownEventCount = 0; int commitEventCount = 0; - List events = EventRecorder.getOrderedEvents(); - for (int i = 0; i < events.size(); i++) { - Event event = (Event) events.get(i); - + List events = EventRecorder.getOrderedEvents(); + for (Event event : events) { if (event instanceof XAResourceCommitEvent) commitEventCount++; @@ -253,9 +247,9 @@ public void testHeuristicMixed() throws Exception { @Override protected void setUp() throws Exception { - Iterator it = ResourceRegistrar.getResourcesUniqueNames().iterator(); + Iterator it = ResourceRegistrar.getResourcesUniqueNames().iterator(); while (it.hasNext()) { - String name = (String) it.next(); + String name = it.next(); ResourceRegistrar.unregister(ResourceRegistrar.get(name)); }