MT-change to JBossManagedConnectionPool.BasePool.getConnecti

[dimitris]

Possible races condition in enlist(), when trackByTx is true, are probably caused one step back while a ConnectionListener is checkout from the ManagedConnectionPool.

I understand that 2 threads in the same Tx could both get a different managed connection, so I'm considering re-writing BasePool.getConnection(..) to temporarily lock on the ThreadLocal when trackByTx is active.

Code now

 public ConnectionListener getConnection(Transaction transaction, Subject subject, ConnectionRequestInfo cri)
 throws ResourceException
 {
 // Determine the pool key for this request
 boolean separateNoTx = false;
 if (noTxSeparatePools)
 separateNoTx = clf.isTransactional();
 Object key = getKey(subject, cri, separateNoTx);
 InternalManagedConnectionPool mcp = getPool(key, subject, cri);

 // Are we doing track by connection?
 TransactionLocal trackByTx = null;
 if (transaction != null)
 trackByTx = (TransactionLocal) trackByTxPools.get(key);

 // Do we have a previous connection in this transaction?
 if (trackByTx != null)
 {
 ConnectionListener cl = (ConnectionListener) trackByTx.get(transaction);
 if (cl != null)
 {
 if (traceEnabled)
 dump("Getting connection tracked by transaction " + cl);
 return cl;
 }
 }

 // Get a connection from the correct pool
 ConnectionListener cl = mcp.getConnection(subject, cri);

 // Are we tracking by connection?
 if (transaction != null && trackByTx != null)
 {
 cl.setTrackByTx(true);
 trackByTx.set(cl);
 }

 if (traceEnabled)
 dump("Getting connection from pool " + cl);
 return cl;
 }

Changed to

 /**
 * Return a connection from the pool, optionally tracked by a transaction
 */
 public ConnectionListener getConnection(Transaction transaction, Subject subject, ConnectionRequestInfo cri)
 throws ResourceException
 {
 // The connection listener to return
 ConnectionListener cl;

 // Use separate pools, if noTxSeparatePools configured in the
 // BasePool and the ConnectionListenerFactory is transactional
 boolean separateNoTx = false;
 if (noTxSeparatePools)
 {
 separateNoTx = clf.isTransactional();
 }
 // The overriding subclass determines the pool key for this request
 Object key = getKey(subject, cri, separateNoTx);

 // Get the pool for this particular key, create it first if needed
 InternalManagedConnectionPool mcp = getPool(key, subject, cri);

 // Are we tracking connection by transaction?
 //
 // TxConnectionManager will pass a non-null transaction if there
 // is a transaction associated with the thread and the connection
 // manager is configured to trackConnectionByTx

 if (transaction != null)
 {
 TransactionLocal trackByTx = (TransactionLocal) trackByTxPools.get(key);

 if (trackByTx != null)
 {
 // If trackByTx we need to synchronize competing threads
 // on the TransactionLocal, so they end up using the same
 // ConnectionListener/ManagedConnection

 trackByTx.lock();
 try
 {
 // Do we have a previous connection in this transaction?
 cl = (ConnectionListener) trackByTx.get(transaction);

 // If yes use it, if no get a one from the pool
 if (cl != null)
 {
 if (traceEnabled)
 log.trace("Getting existing connection tracked by transaction");
 }
 else
 {
 // Get a new connection (listener) from the pool
 cl = mcp.getConnection(subject, cri);
 // Mark that is tracked by transaction
 cl.setTrackByTx(true);
 // Store the connection listener to the TransactionLocal
 trackByTx.set(cl);

 if (traceEnabled)
 log.trace("Getting a connection from the pool to track the transaction");
 }
 }
 finally
 {
 trackByTx.unlock();
 }
 }
 else
 {
 // Weird, can happen only if BasePool.getTransationManager()
 // returns null, while constructing the TransactionLocal in
 // getPool(). Treat as if trackByTx is disabled
 if (traceEnabled)
 log.trace("TrackByTx true, but no TransactionManager associated with the pool;" +
 " getting a connection from the pool");

 // Get a new connection (listener) from the pool
 cl = mcp.getConnection(subject, cri);
 }
 }
 else
 {
 // Not tracking connection by transaction
 if (traceEnabled)
 log.trace("Getting a connection from the pool");

 // Get a connection from the correct pool
 cl = mcp.getConnection(subject, cri);
 }

 if (traceEnabled)
 dump("Returning connection " + cl);

 return cl;
 }

[adrian.brock]

Ok. But can I suggest a different approach.

Rather than getting us to validate every change you make to the software
by forcing us to read your patches, use the following alogorithm:

1) I think I've found a problem
2) Write a test that reproduces the problem
3) Write some stress tests to measure the performance (both contended and uncontended)
4) Fix the problem
5) Rerun the unit test to make sure it is fixed
6) Rerun the stress test to make sure you haven't introduced a bottleneck in the uncontended case or a deadlock in the contended case

NOTE: The contended case in (3) will likely fail as well if there is a real MT problem.
NOTE 2: We do read your patches on the cvs commit mailing list anyway

[dimitris]

Ok, thanks.