Terry explains the issue well so take a look at his message for a
description of the problem.
This patch fixes the problem by keeping track of the number of threads
that are in the wait state. If a new thread calls the
getInternalPooledConnection method and the wait count is not zero then
the calling thread drops into the wait state. This allows a potential
thread "awakened" from the wait state by a notify call to acquire the
synchronize lock and obtain the connection. It also acts in a "fair"
way since if there are existing threads in a wait state when a thread
calls the get connection method then the thread will also go into the
wait state, essentially getting in the back of the line.
Byron
On Tue, 2001-11-27 at 12:50, Terry August wrote:
>
> I have ran into a problem with the ConnectionPool class from Turbine 2.x. I
> noticed the problem while making many requests to a pool configured with a
> maximum of 5 connections. What happens is the following:
>
> 1. All 5 connections go into use.
> 2. Several threads are waiting for a connection to be returned to the
> stack.
> 3. 1 connection is returned and notify() is called - line 655
> 4. One of the threads is awakened and is now actively competing for the
> lock.
> 5. A new thread (non-waiting) is created and gets the lock before the
> awakened thread.
> 6. The new thread takes the newly available connection from the stack.
> 7. The awakened thread now gets the lock and discovers that there is no
> available connection in the pool even though it was awakened.
> 8. The code in lines 580-585 are now executed and an exception is thrown.
>
> The problem is that this code will result in exceptions all the time on
> heavily hit dynamic pages (with several connections being made in the page)
> unless the maxConnections is raised unjustly. Also connectionWaitTimeout
> specifies the amount of time a thread will wait, but doesn't consider the
> fact that the above sequence might occur. In this case the awakened thread
> may have only waited 1 second and caused an error, even if
> connectionWaitTimeout is set to 10 seconds. I can see two solutions. One
> solution is to revamp the synchronization to disallow new threads from
> taking the lock before the awakened thread gets the lock. Or, the
> connectionWaitTimeout property can be considered a TOTAL wait time and the
> code can loop in that section. Something like the following may work:
>
? idbroker.patch2.txt
? transkey.patch.txt
? con_pool.patch.txt
Index: src/java/org/apache/torque/pool/ConnectionPool.java
===================================================================
RCS file:
/home/cvspublic/jakarta-turbine-torque/src/java/org/apache/torque/pool/ConnectionPool.java,v
retrieving revision 1.5
diff -u -r1.5 ConnectionPool.java
--- src/java/org/apache/torque/pool/ConnectionPool.java 7 Dec 2001 16:13:10 -0000
1.5
+++ src/java/org/apache/torque/pool/ConnectionPool.java 17 Jan 2002 03:46:25 -0000
@@ -488,6 +488,13 @@
"connection from empty pool!");
}
+
+ /**
+ * Counter that keeps track of the number of threads that are in
+ * the wait state, waiting to aquire a connection.
+ */
+ private static int waitCount = 0;
+
/**
* Gets a pooled database connection.
*
@@ -498,10 +505,21 @@
private synchronized DBConnection getInternalPooledConnection()
throws ConnectionWaitTimeoutException, Exception
{
- DBConnection dbconn = null;
-
- if ( pool.empty() )
+ if ( waitCount > 0 || pool.empty() )
{
+ // We test waitCount > 0 to make sure no other threads are
+ // waiting for a connection. If waitCount is not tested
+ // the following situation occurs: After a thread
+ // returns a connection it calls notify. If another thread
+ // is currently in the wait state, it will awaken and
+ // begin competing for the synchronization lock.
+ // Meanwhile, if another thread is attempting to call this
+ // method, it may acquire the lock first and "steal" the
+ // connection. The thread that was waiting will then
+ // acquire the lock, but the connection is no longer
+ // available, so a ConnectionWaitTimeoutExecption is
+ // thrown.
+
connectionAttemptsCounter++;
// The connection pool is empty and we cannot allocate any new
@@ -509,12 +527,18 @@
// a connection is returned.
try
{
+ waitCount++;
wait( connectionWaitTimeout );
}
catch (InterruptedException ignored)
{
// Don't care how we come out of the wait state.
}
+ finally
+ {
+ waitCount--;
+ }
+
// Check for a returned connection.
if ( pool.empty() )
@@ -523,14 +547,9 @@
// someone returning a connection.
throw new ConnectionWaitTimeoutException(url);
}
- dbconn = popConnection();
- }
- else
- {
- dbconn = popConnection();
}
- return dbconn;
+ return popConnection();
}
/**
--
To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]>
For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>
