Threading problems in connection pooling - suggested code change

[Elli Albek <el...@sustainlane.com>]

Hi,
* see code below

I use DBCP, and in our multithreaded performance test of DAOs we found that
multiple threads are basically blocking on the connection pool class, either
waiting to get a connection or to return a connection. In the test most
threads were idle blocking on the DBCP pool, and not doing database work.
This behavior becomes more severe as the number of threads grows.

The problem identified was over synchronization of the pool. Calls to get
and return a connection, were performing database calls, while other threads
block during these calls. This includes resetting connection state (auto
commit), and validation on borrow/return.

I modified the code to avoid this. The changes ensure that all access to
local variables in the pool object are synchronized, but calls to the
database happen on the calling thread. The calls on the caller thread are
the ones that go to the database, and are the expensive ones.

I see a minor problem using assertOpen(), but in a web application
environment the server handles shutdown properly, and we do not add and
remove pools on the fly, so for our needs it is fine. For a general purpose
use, it may not be.

The results are good. The threads spend virtually no time in the connection
pool code after warm-up, and do database work during the test rather than
block. These code changes were reviewed carefully in multithreaded debugger,
and survived our test harness. However, as with any multithreaded code, it
is suspect until proven otherwise :)

I have one grief: It was very difficult to modify this code. I had to copy a
few java files and change small number of lines in each. I would rather
subclass and replace a few methods, or use IOC to inject modified classes
rather than change hard coding of actual implementation classes in many
files. I think a spring like approach will make the code more developer
friendly and encourage more contributions.

Any feedback is welcome.

--------------------------------
public Object borrowObject() throws Exception {
		assertOpen();
		long starttime = System.currentTimeMillis();
		for (;;) {
			ObjectTimestampPair pair = null;

			// EA: Added synchronization on shared variable and
notification
			synchronized (this) {
				// if there are any sleeping, just grab one
of those
				try {
					pair = (ObjectTimestampPair)
(_pool.removeFirst());
				} catch (NoSuchElementException e) {
					; /* ignored */
				}
				// otherwise
				if (null == pair) {
					// check if we can create one
					// (note we know that the num
sleeping is 0, else we wouldn't be here)
					if (_maxActive < 0 || _numActive <
_maxActive) {
						// allow new object to be
created
					} else {
						// the pool is exhausted
						switch
(_whenExhaustedAction) {
						case WHEN_EXHAUSTED_GROW:
							// allow new object
to be created
							break;
						case WHEN_EXHAUSTED_FAIL:
							throw new
NoSuchElementException("Pool exhausted");
						case WHEN_EXHAUSTED_BLOCK:
							try {
								if (_maxWait
<= 0) {
	
wait();
								} else {
									//
this code may be executed again after a notify then
									//
continue cycle
									//
so, need to calculate the amount of time to wait
	
final long elapsed = (System.currentTimeMillis() - starttime);
	
final long waitTime = _maxWait - elapsed;
									if
(waitTime > 0) {
	
wait(waitTime);
									}
								}
							} catch
(InterruptedException e) {
								// ignored
							}
							if (_maxWait > 0
									&&
((System.currentTimeMillis() - starttime) >= _maxWait)) {
								throw new
NoSuchElementException(
	
"Timeout waiting for idle object");
							} else {
								continue; //
keep looping
							}
						default:
							throw new
IllegalArgumentException(
	
"WhenExhaustedAction property " + _whenExhaustedAction
	
+ " not recognized.");
						}
					}
				}
				_numActive++;
			}

			// create new object when needed
			boolean newlyCreated = false;
			if (null == pair) {
				try {
					Object obj = _factory.makeObject();
					pair = new ObjectTimestampPair(obj);
					newlyCreated = true;
				} finally {
					if (!newlyCreated) {
						// EA: Added synchronization
on shared variable and notification
						synchronized (this) {
							// object cannot be
created
							_numActive--;
							notifyAll();
						}
					}
				}
			}

			// activate & validate the object
			try {
				_factory.activateObject(pair.value);
				if (_testOnBorrow &&
!_factory.validateObject(pair.value)) {
					throw new Exception("ValidateObject
failed");
				}
				return pair.value;
			} catch (Throwable e) {
				// object cannot be activated or is invalid
				// EA added synchronization on access to
shared variables and
				// notification:
				synchronized (this) {
					// object cannot be activated or is
invalid
					_numActive--;
					notifyAll();
				}
				try {
					_factory.destroyObject(pair.value);
				} catch (Throwable e2) {
					// cannot destroy broken object
				}
				if (newlyCreated) {
					throw new NoSuchElementException(
							"Could not create a
validated object, cause: " + e.getMessage());
				} else {
					continue; // keep looping
				}
			}
		}
	}

	// EA Removed synchronization
	public void returnObject(Object obj) throws Exception {
		assertOpen();
		addObjectToPool(obj, true);
	}

	private void addObjectToPool(Object obj, boolean decrementNumActive)
throws Exception {
		boolean success = true;
		if (_testOnReturn && !(_factory.validateObject(obj))) {
			success = false;
		} else {
			try {
				_factory.passivateObject(obj);
			} catch (Exception e) {
				success = false;
			}
		}

		boolean shouldDestroy = !success;

		// EA Added synchronization on shared access variables and
notification
		synchronized (this) {
			if (decrementNumActive) {
				_numActive--;
			}
			// EA Added local variable with synchronized access,
to be on the safe
			// side
			final int _maxIdle = getMaxIdle();
			if ((_maxIdle >= 0) && (_pool.size() >= _maxIdle)) {
				shouldDestroy = true;
			} else if (success) {
				_pool.addLast(new ObjectTimestampPair(obj));
			}
			notifyAll(); // _numActive has changed
		}
		if (shouldDestroy) {
			try {
				_factory.destroyObject(obj);
			} catch (Exception e) {
				// ignored
			}
		}
	}

	/**
   * EA: Removed outside synchronization, synchronized shared variables and
   * notifications inside.
   */
	public void invalidateObject(Object obj) throws Exception {
		assertOpen();
		try {
			_factory.destroyObject(obj);
		} finally {
			// EA Added synchronization on shared access
variables and notification
			synchronized (this) {
				_numActive--;
				notifyAll(); // _numActive has changed
			}
		}
	}


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org

RE: Threading problems in connection pooling - suggested code change

[Elli Albek <el...@sustainlane.com>]

Good point on access to configuration variables. I am thinking of the local
variables in the class as two types: Configuration, and runtime. The
configuration variables do not change in our application, therefore access
was not always synchronized. This will be a problem for applications that
wish to change the configuration of the pool on the fly. The runtime
variables (like numActive) are changed when threads use the pool, and they
are all synchronized. 
Since we do not plan on dynamic configuration (yet), we think of the
configuration variables as constants. This of course is not acceptable for
general purpose use, because the class was written to support dynamic
configuration.
To make this implementation usable in the general case, access to all
variables probably needs to be synchronized. 
I think of a few of ways to do that:

1. Use the getters, which are mostly synchronized
2. Localize variables on the stack in all functions that are not fully
synchronized, and initialize in a synchronized block. This is more atomic,
as values that change while the code is running will not affect it. Example:

final int whenExhaustedAction;
final int maxWait;
final List pool;

synchronized(this){
  whenExhaustedAction = _whenExhaustedAction;
  maxWait = _maxWait;
  pool = _pool;
}

3. Use an object that holds all static configuration data, and get a
reference to it in the beginning of the function. This is the most atomic. A
synchronized getter would be nice:

synchronized Config getConfig() { return _config; }

As for changing the structure of the functions, I agree there are benefits.
However, we ended up with massive copy-pastes because there was no other way
to extend the code. We would like to keep things as close as possible to the
original code, it is already non trivial to follow the changes. Changing the
function signatures will make it harder to deal with when we upgrade.

Thanks for feedback. To actually submit a patch this needs more work as you
noticed, maybe in a few weeks. To be honest, I didn't see much response to
your patch, which I have to say is very surprising. This is a critical piece
of code and it is widely used (shipped as standard with Tomcat). There is
also the release candidate that contains performance improvements that is on
hold for a while. The download page has the latest version as June 2004!


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org

Re: Threading problems in connection pooling - suggested code change

[Holger Hoffstaette <ho...@wizards.de>]

On Fri, 16 Mar 2007 13:18:23 -0700, Elli Albek wrote:

> I use DBCP, and in our multithreaded performance test of DAOs we found
> that multiple threads are basically blocking on the connection pool class,
> either waiting to get a connection or to return a connection. In the test
> most threads were idle blocking on the DBCP pool, and not doing database
> work. This behavior becomes more severe as the number of threads grows.
> 
> The problem identified was over synchronization of the pool. Calls to get
> and return a connection, were performing database calls, while other
> threads block during these calls. This includes resetting connection state
> (auto commit), and validation on borrow/return.
> ...
(snip)

The "oversynchronization" of pool is both a bug and a feature; after all
the whole point of a pool is to be a bottleneck...sometimes. ;)
I have not yet read the new borrowObject() code but can at least provide
some feedback for returnObject():

1) branching off addObjectToPool() with a constant "decrementNumActive"
argument can be refactored into returnObject() again. :)

2) you have esentially reimplemented my work for GKOP from
https://issues.apache.org/jira/browse/POOL-93

That issue also provides some insight as to why really effective changes
require much more than just splitting up of synchronization blocks, even
though that's a good start. When pooling objects with heavyweight
creation/validation/destruction, something even more advanced like a
concurrently pipelined factory might be the only way to increase
scalability. Search through the commons archives for past discussion on
this.
Btw: your code has a better chance of being reviewed and adopted if you
file a separate JIRA for tracking.

regards
Holger
(just another pool user)



---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org