LruStatementCache is broken.

[rankinc]

The following test fails:

PreparedStatement stmt = c.prepareStatement(DB_SELECT_SYSDATE_SQL);
assertFalse(stmt.isClosed());
stmt.close();
assertTrue(stmt.isClosed());

stmt = c.prepareStatement(DB_SELECT_SYSDATE_SQL);
assertFalse(stmt.isClosed());

The problem is that the proxies inside the LruStatementCache are never replaced, which means that once "pretendClosed = true", it remains true until the statement is evicted from the cache.

[brettwooldridge]

If you can submit a pull request it would be greatly appreciated.

[rankinc]

Brett, I am trying to think of the best way of fixing this. It seems closely coupled with my "PreparedStatement leak" issue, because the root of both problems is that the LruStatementCache contains proxy objects instead of the underlying PreparedStatement objects from the DB driver. So I was thinking of modifying all of the "prepareStatement()" functions in ConnectionJavaProxy like this:

    if (useStatementCache) {
        CacheKey cacheKey = new CacheKey(sql);
        PreparedStatement cachedStmt = jdbcPooledConnection.getCachedStatement(cacheKey);
        if (cachedStmt == null) {
            PreparedStatement stmt = delegate.prepareStatement(sql);
            cachedStmt = jdbcPooledConnection.putCachedStatement(cacheKey, stmt);
        }

        return JdbcProxyFactory.INSTANCE.getProxyPreparedStatement(jdbcPooledConnection, cachedStmt, cacheKey);
    }
    else {

This way, the LruStatementCache contains delegates instead of proxies, which also means that the LruEvictionListener will close delegates - thus fixing my memory leak.

It also means that JdbcProxyTest.testCachePrepared() now breaks because the prepareStatement() methods now return different proxies for the same delegate. I.e. this fails:

PreparedStatement prepareStatement1 = connection.prepareStatement("SELECT 1 FROM nothing WHERE a=? AND b=? AND c=? AND d=?");
PreparedStatement prepareStatement2 = connection.prepareStatement("SELECT 1 FROM nothing WHERE a=? AND b=? AND c=? AND d=?");

Assert.assertEquals(prepareStatement1, prepareStatement2);

Can you tell me what the "crucial semantics" of the LruStatementCache are please? The properties / assumptions that must not be broken at any cost? I am already concerned that the cache hands out the same PreparedStatement simultaneously to two different callers - regardless of whether or not it has been proxied.

Cheers,
Chris

[rankinc]

Actually, it looks like putCachedStatement() can return "null" under some conditions. So this is probably better:

if (useStatementCache) {
    CacheKey cacheKey = new CacheKey(sql, columnNames);
    PreparedStatement cachedStmt = jdbcPooledConnection.getCachedStatement(cacheKey);
    if (cachedStmt == null) {
        cachedStmt = delegate.prepareStatement(sql, columnNames);
        jdbcPooledConnection.putCachedStatement(cacheKey, cachedStmt);
    }

    return JdbcProxyFactory.INSTANCE.getProxyPreparedStatement(jdbcPooledConnection, cachedStmt, cacheKey);
}
else {

[rankinc]

This fix is going to require moving to JDK6, so that I can use isWrapperFor() and unwrap() to examine the underlying delegate objects. According to issue #17, this should be OK.

[brettwooldridge]

Has then been addressed in your recent pull requests or is this outstanding?

[rankinc]

The points that I raised in the original comment have all been addressed. However, I have lingering concerns over the way that the cache allows a PreparedStatement to be "used" by more than one caller at a time. I just don't see how that can work. Fortunately, a connection is typically used by a single thread at a time, which makes my Doomsday Scenario much less likely.

[brettwooldridge]

It should not be possible for more than one thread to access a JdbcPooledConnection (and therefore PreparedStatement) at one time, unless somewhere else we are violating the JTA spec.

[rankinc]

True, so the only scenario where the cache could cause problems is when someone does the following within a single thread:

String sql = "SELECT * FROM table WHERE ...";
PreparedStatement stmt1 = connection.prepareStatement(sql);

... // no-one closes stmt1

PreparedStatement stmt2 = connection.prepareStatement(sql);

At the moment, stmt1 and stmt2 would be proxy wrappers around the same underlying statement delegate. I don't believe that the statement cache should ever permit this to happen - if a statement delegate is in use then we should probably create a brand new and uncached PreparedStatement for the query instead.

So basically:

• the statement's usageCount would become a boolean instead of an int,
• PreparedStatementJavaProxy.close() would become able to close a delegate that threw an SQLException and then remove it from the cache, without needing to worry about side-effects.