Connection pooling does not appear to be correctly supported #1254

Closed
robertandrewbain opened this Issue Mar 12, 2015 · 29 comments

Projects

None yet

6 participants

@robertandrewbain
Member

Using database connection pooling with org.apache.commons.dbcp2.BasicDataSource, upon executing a SQLQuery the connection is never returned to the pool, unless you put it inside a transaction, which is not always convenient. The following workaround manually closes the connection, returning it to the pool.

protected Connection getSQLQueryConnection(@NonNull SQLQuery query) {
    Field declaredField;
    try {
        declaredField = SQLQuery.class.getSuperclass().getDeclaredField("conn");
        declaredField.setAccessible(true);
        return (Connection) declaredField.get(query);
    } catch (NoSuchFieldException | SecurityException | IllegalArgumentException | IllegalAccessException e) {
        log.error("Could not retrieve connection object from Object {}", query);
    }

    return null;
}

protected void closeSQLQuery(@NonNull SQLQuery query) {
    try {
        if (!TransactionSynchronizationManager.isActualTransactionActive()) {
            getSQLQueryConnection(query).close();
        } else {
            log.warn("Not closing connection on transaction wrapped connection {}", query);
        }
    } catch (SQLException e) {
        log.error("Could not close connection {}", query, e);
    }
}

The most notable drawback with this mechanism is the fact that it relies on the programmer knowing that this must be called, otherwise the connection pool will soon run out of available connections.

It would be far better if querydsl could provide support for connection pooling, ensuring that connections are returned to the pool upon execution finishing.

@Shredder121
Member

We explicitly don't close the connections, since the general idea is:

  • If you create an instance, you should clean it up (e.g. close)
  • If you get supplied an instance, don't clean it up

@timowest The only place I think we can adjust this is in the QueryFactorys, since the Provider<Connection> has no way of cleaning up the provided connection.

@robertandrewbain as for your issue, how do you create your queries?
It should look something like this:

//In Java 7 this can be achieved with a try-with-resources block
Connection conn;
try {
    conn = datasource.getConnection();
    //...
} finally {
    if (conn != null) {
        conn.close();
    }
}
@robertandrewbain
Member

Hi @Shredder121, I'm injecting a datasource through spring and creating a com.mysema.query.QueryFactory from its connection. Something like:

QueryFactory<SQLQuery, SQLSubQuery> queryFactory = new SQLQueryFactory(configuration, getConnection(dataSource));

private Provider<Connection> getConnection(final DataSource dataSource) {
    return new Provider<Connection>() {
        @Override
        public Connection get() {
            return DataSourceUtils
                    .getConnection(QueryDslJdbcTemplate.this.dataSource);
        }
    };
}

From there I call queryFactory.query() and queryFactory.subQuery() to get queries and subqueries respectively. This makes it tricky to handle connection closing. Do you have any ideas?

@robertandrewbain
Member

My initial thoughts were that perhaps there could be some configuration to automatically close connections, that the programmer could activate if required. I can see the cohesion in explicitly not closing the connections but within large systems that use DI etc. it would be much easier and far safer (if one programmer misses closing one pooled connection manually, soon the system will be brought to its knees, and it's not an obvious one to pinpoint) to have a QueryFactory that you inject all your configuration into, then just call query() and subquery() and have the connections returned to the pool.

@Shredder121
Member

After some inspection, I think this can be solved using Configuration.listeners.
When end() is called, you can retrieve the connection and clean it up.

@timowest I don't think it is a good idea to enable this by default, but I think it might be valuable to make some configuration possible to turn such a feature on.

@timowest
Member

@Shredder121 This is a good suggestion. We can improve the Querydsl SQL reference docs with usage patterns

  • externally managed and closed connection
  • connection which is closed via Querydsl listener, needs a fresh connection for each query / clause
  • what else?

@robertandrewbain Why don't you have a transactionally managed connection in Spring? querydsl-sql-spring gives you SpringConnectionProvider which provides the transactionally bound connection.

@Shredder121
Member
  • Support for the concept of reused connections.(custom listener override)
  • Late providing of Connection via clone(connection)(if that isn't already included).

These usage patterns come to mind.

@robertandrewbain
Member

@timowest I was unaware of SpringConnectionProvider, thanks for that, it could well be exactly what I'm looking for, I'll have a look first thing. The more I've thought about this issue and discussed it with friends / peers, the more I understand why the programmer is responsible for the connection. My implementation abstracts the connection details away, which is really nice to work with but leaves responsibility for the connection to no-one. It's un-cohesive to have a query object handling connections but at the same time, as a configurable option, it would seriously reduce boiler-plate and ensure good resource management. Thank you both for your input.

@timowest timowest added the feature label Mar 14, 2015
@jgoyer3304

Have just spent an entire day tracking down and trying to fix connection leak issue resulting from numskull usage of query dsl. If that were not enough, I have to resort to the trick printed here to close
the connections. This is a large project and everyone wants to use their own framework for integration testing, hence we are using c3p0 with DBunit with QueryDSL with Spring with (insert framework name here). Testcases run inside a transaction managed by a DBUnit listener. I tried several solutions but am resorting to the crappy code above because QueryDSL makes it so hard to close a connection.
How about a close method on the SQLQuery and similar query classes? Or at least a method to get the Connection I used to create the ^&(^& instance in the first place? Why is this so hard to do?

Java systems are built in layers and as the previous post said, even figuring out whether a transaction is running is not always convenient. I like QueryDSL but am extremely pissed that I have to deal with such an obviously simple issue. All the QueryDSL examples (and apparently the developers of the framework) ignore connection management. Get a clue people.

@timowest
Member
timowest commented May 9, 2015

@jgoyer3304 We don't handle connection management in the examples since Querydsl was mostly designed with external connection management in mind. Querydsl doesn't implicate in the configuration that supplied connections would be closed.

What we could do though is provide a listener that does the closing and mention that in the docs.

@jgoyer3304

Yes I think a listener solution is appropriate. However I looked at the SQLListener interface and
without testing it it appears to notify the listener before a query is done. There is mention of an end() routine above in this thread but I did not see it. Is there any way to write a listener using version
3.6.3 such that it would issue a callback when a query was finished?

I apologize for the rant -- it was very late at night. One observation I can make though is that software
designers cannot always predict how people are going to use their code. A great example is Java itself. The killer app they envisioned was the applet :)

@jgoyer3304

Never mind, found the lifecycle routines in the listener.

@timowest
Member
timowest commented May 9, 2015

The end() method is in SQLDetailedListener. Just implement that interface and close the connection in end, that should do the trick. If you want to have that as an official part of Querydsl, feel free to provide a pull request for that.

Concerning the rant, apology accepted ;) And you are right about software / framework designers coding with their known use cases in mind. I usually do JDBC/JPA stuff in AOP managed context, where the session/connection is opened and closed outside of any Querydsl class.

@jgoyer3304

So this is what I will put in a base test class. I will have to chase down any developer that doesn't
use it!

    public <T> SQLDeleteClause getSQLDeleteClause( RelationalPath<T> relation ) {
        Connection conn = null;      
        try {
            conn = dataSource.getConnection();
        } 
        catch (SQLException e) {
            logger.error( "Unable to get javax.sql.Connection from DataSource with impl class " + dataSource.getClass().getName() );
            return null;
        }

        SQLDeleteClause q = new SQLDeleteClause( conn, dialect, relation );
        q.addListener( new SQLCloseListener() );
        return q;
    }
@timowest
Member
timowest commented May 9, 2015

I assume that you use a singleton Configuration object for Querydsl SQL. Just register the listener there then it will be used for all queries and clauses.

Also it makes sense to use a SQLQueryFactory instance for query creation.

Example:

// configuration
Configuration configuration = new Configuration(dialect).
configuration.addListener(new SQLCloseListener());
SQLQueryFactory queryFactory = new SQLQueryFactory(configuration, dataSource);

// usage
SQLDeleteClause deleteClause = queryFactory.delete(relation);
@timowest
Member
timowest commented May 9, 2015

@jgoyer3304 Also why don't you use AOP to open and close a connection?

@jgoyer3304

Thanks for the suggestions. Yes AOP is probably better given what (I think) you are trying to do here, which is not have the framework keep any state, as opposed to an entity manager type solution. We are using Spring and have public void test methods so that might be possible without aspectj weaving. I will give it a try and post something.

@robertandrewbain
Member

@timowest do you mean SQLDetailedListener when you say SQLExtendedListener? Would you just create no-ops methods for all but void end(SQLListenerContext context) and call context.getConnection().close() from there?

Would this work if an Exception were to be thrown between the connection opening and void end(SQLListenerContext context) being called?

@timowest
Member

@timowest do you mean SQLDetailedListener when you say SQLExtendedListener?

Yes, sorry, I meant SQLDetailedListener. I updated that comment.

Would you just create no-ops methods for all but void end(SQLListenerContext context) and call context.getConnection().close() from there?

Yes.

Would this work if an Exception were to be thrown between the connection opening and void end(SQLListenerContext context) being called?

Yes, it handles exceptional cases as well.

@timowest timowest added the progress label May 14, 2015
@robertandrewbain
Member

@timowest this is really great stuff, thanks for this. When you say this will work for exceptional cases, right now I'm closing connections in a finally block. Essentially if connections don't get closed they are never returned to the pool, so it's an absolute necessity that they are closed, or our application will grind to a halt. Is the listener implementation similar in that end(SQLListenerContext context) is guaranteed to get called?

@timowest
Member

I continued with testing the SQLCloseListener of the pull request and noticed that the lifecycle state end happens after the query has been executed, but before the result set has been consumed.

So the logic needs some changes before the SQLCloseListener works in general. If we get this to work properly we will backport it also for Querydsl 3.

For now it works only for fetch() (list(...) in Querydsl 3).

@timowest
Member

@Shredder121 @johnktims What do you think about moving the end() call after the result set of the iteration has been consumed? Or to be more precise, when the close method of the CloseableIterator of iterate() is called.

@robertandrewbain
Member

@timowest this sounds reasonable to me, from the perspective of closing connections. After all, the method is called end() and query hasn't met its end if there's still an open cursor traversing the results. I assume this will be bespoke logic for the iterate method.

@Shredder121
Member

@Shredder121 @johnktims What do you think about moving the end() call after the result set of the iteration has been consumed? Or to be more precise, when the close method of the CloseableIterator of iterate() is called.

Just now noticed I didn't reply.
I think that sounds like a reasonable idea as well, and must agree with @robertandrewbain that this is only logical behavior for iterate.

@johnktims johnktims closed this in #1374 Jun 17, 2015
@timowest timowest removed the progress label Jun 17, 2015
@timowest timowest added this to the 4.0.2 milestone Jun 17, 2015
@ssaarela
Contributor

Does waiting for the results to be consumed before closing actually work in AbstractSQLQuery.getResults()? It seems to be calling endContext in a finally-clause before ResultSet is even returned to the caller. Shouldn't it call it the ResultSetAdapter's close, above (and in a catch-block in case of an exception)?

@timowest
Member

You are right @ssaarela. Will be fixed in the next release. Thanks for the heads up.

@morungos

Sorry to revisit this, but I'm spending days on this problem. Basically, I'm using Spring in a command-line application, and we're exhausting the connection pool after about 8 queries. Even if this is a corner oddity, it deserves some documentation.

  1. SpringConnectionProvider doesn't work at the command line, as far as I can tell it's a thin wrapper around something that Spring won't document well. I'd love to use transactions and Spring, but nothing is working right. At a guess, everything's assuming some additional context. And we're using XML config which means mapping from fragments of Spring code with annotations is a little hard.

I can't believe it really is that hard to build a command-line application which can run lots of queries with QueryDSL through a Spring setup, without it hanging up in a connection pool after a few seconds.

I'm using QueryDSL SQL 3.* -- do I need to update to 4.* for something to become a little more tractable?

@ssaarela
Contributor
ssaarela commented Sep 13, 2016 edited

@morungos, like you said, SpringConnectionProvider is a thin wrapper for DataSourceUtils, which in turn is tied to Spring's transaction management. So you need to set up and use Spring's transaction management in order to get connections. There's multiple ways of doing that with XML and/or annotations, but all require PlatformTransactionManager bean to be defined. Then you either use AOP proxies or TransactionTemplate for actual connection+transaction management.

http://docs.spring.io/autorepo/docs/spring/4.2.x/spring-framework-reference/html/transaction.html

@morungos

@ssaarela Yes, I have all that, and everything is working fine in Spring-managed tests. But if I use the exact same XML and try to use it independently, SpringConnectionProvider just throws errors saying the connection isn't transactional. I'm suspicious it's some AOP or AspectJ thing that needs to be done manually because I'm not running in a "Spring-managed container" (i.e., this isn't a web app). I've tried the declarative approach because it seems closest to that used in testing, all the declarations in place, but all I get from SpringConnectionProvider is an error.

Like you said, it's probably some Spring subtlety that they can't be troubled to actually help people with. That page on transactions (which I read before posting) is about 100 pages of unstructured ramble with no outline, so it's hard to find what you really need.

Having said that, a simple working application which runs from the command line and uses QueryDSL with Spring transactions and valid connection pooling would be extremely helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment