 |
History
| Log In
|
|
|
|
Verrified issue, via this unit test:
public class PoolingConnectionProviderTest extends TestCase { public void testShutdown() throws Exception { final PoolingConnectionProvider poolingConnectionProvider = new PoolingConnectionProvider(...); // db args final Connection conn = poolingConnectionProvider.getConnection(); new Thread() { public void run() { try { poolingConnectionProvider.shutdown(); } catch (SQLException e) { e.printStackTrace(); } } }.start(); conn.close(); } } Placing a breakpoint at: 1. GenericObjectPool [line: 869] - addObjectToPool(Object, boolean) 2. GenericObjectPool [line: 896] - close() Once both breakpoints are hit, let them continue and you will get the above exception (this is with the latest dbcp and pool jars) This seems to be a pool library issue, and not really a Quartz issue. The only way I can see to fix it from the Quartz side is to have PoolingConnectionProvider.getConnection() return a wrapped Connection that will synchronize Connection.close() with PoolingConnectionProvider.shutdown(). At the end of the day, I'm actually enclined to submit this as a bug to the pools project, but not to fix it in Quartz. Though the exception is ugly, the reality is that the pool has been cleaned up, so there is no danger of a resource leak. It doesn't seem worth the overhead to me to fix it for an issue that will rarely crop up and, when it does, has no impact. Actually, maybe the real solution is just to have JobStoreSupport.closeConnection() (or cleanupConnection()) swallow all exceptions rather than propogating them. Generally speaking a connection failing to close is not an error that needs to be passed up the chain, but rather could be logged and dropped... Added commons-pool bug: http://issues.apache.org/bugzilla/show_bug.cgi?id=39052
The pool issue will be fixed in commons-pool 1.3 (verified against 1.3 RC2).
However, once Quartz upgrades to 1.3 we will no longer get a NullPointerException but instead get a java.lang.IllegalStateException, which isn't a huge improvement. pool 2.0 will supposedly handle this case gracefully though. After discussion with James, we agreed that there really is no advantage to propogating exceptions when closing connections and I will change the code to swallow any caught. As part of this change, I also changed the closeConnection() signature to no longer throw JobPersistenceException. This does have the small risk of breaking backwards compatibility if someone was overloading this class and was also throwing JobPersistenceException. The one case I know of, Spring, does not define any throws, and so it seems like a pretty low risk change. We can easily add it back in though if someone is concerned.
| ||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
Null pointer during quartz shutdown
Right now though it does look like the problem exists in both versions, namely that GenericObjectPool.returnObject() asserts that the pool is open before trying to return the connection to the pool, but GenericObjectPool.close() nulls out the pool's fields before actually calling the superclass BaseObjectPool.close() which is actually responsible for marking the pool as closed.
What must be happening is that GenericObjectPool is in the middle of closing, having already nulled out the underlying _pool member variable but not yet having set closed to true, when the cluster manager tries to return its connection. GenericObjectPool.returnObject() successfully passes the assertOpen() check since closed is still set to false, but then it blows up trying to return the connection to the underlying _pool which is null.
I will do some testing to make sure this case is correct, and also see if it is a reported issue in the commons-pool project.