VELOCITY-989 Fixed multiple connection leaks in DataSourceResourceLoader.

[FrancisS]

The first case is in the implementation of FilterReader. It attempts to get a connection from a result set after closing the result set. This always throws an exception as it is an invalid operation after closing the result set, which results in not closing the connection.

The second is in the getResourceReader method. In the happy path a FilterReader is returned which eventually closes the connection when close is called. However, if a template is not found, an exception is thrown without closing the connection.

Opened issue here https://issues.apache.org/jira/browse/VELOCITY-989

[michael-o]

I wonder why this never surfaced before...

[FrancisS]

First, correction to my first comment, the original issue was attempting to get a statement from a result set, which then was used to get the connection to close. Getting the statement was an invalid operation after closing a result set.

This PR doesn't fix the issue depending on what JDBC implementation you are using. It does not seem like it's safe to rely on Statement.getConnection at all rather then keeping a reference to the original connection returned from DataSource.getConnection.
I have seen two additional issues in two different pool implementations.

org.apache.tomcat.jdbc.pool:
Statement.getConnection returns the unwrapped, driver specific connection, rather then the wrapped connection provided by the pool and returned from DataSource.getConnection. This means the underlying connection gets closed directly instead of just marking it as available in the pool which again leads to connection leaks.

org.apache.commons.dbcp2:
Statement.getConnection returns the wrapped connection, but returns null if the statement is closed. This leads to a NullPointerException which is not ignored in the current implementation since it is a RuntimeException, unlike the original issue I found where it would throw a non RuntimeException.
This happens because close is called twice on the Reader, once by VelocityCharStream when it reaches the end of the stream, and once in Template when the parsing is over. This was fine when the second close just resulted in an exception caught and ignored but blows up after my change due to the NPE.

The NPE issue could be handled with a null check but I don't see any way around the issue with org.apache.tomcat.jdbc.pool without keeping a reference to the connection. There are probably still more other unknown issues since the behavior of ResultSet.getStatement and Statement.getConnection does not appear to be standardized and varies across implementations.

I have a custom DataSourceResourceLoader I am now using which does not rely on Statement.getConnection and can update this PR if it seems like this issue will get any traction.

[arkanovicz]

I wonder why this never surfaced before...

The DatasourceResourceLoader has always been a second zone citizen, not well tested, not well documented, and its javadoc states that it's a simple loader.

[FrancisS]

XPosting from the jira issue:
I updated the PR with a fix that I don't love but I can't find a way to do it differently without changing the API of DatabaseObjectsFactory. I updated the factory to return a proxy of Statement and ResultSet that guarantees Statement.getConnection and ResultSet.getStatement behave in a way that will not cause issues.

However I would prefer to change DatabaseObjectsFactory.prepareStatement and DatabaseObjectsFactory.releaseStatement to use an explicit wrapper object instead. This would avoid the somewhat hard to follow proxy code and would be a bit more obvious to anyone writing custom implementations of DatabaseObjectsFactory that the original connection and statement are needed rather then relying on Statement.getConnection or ResultSet.getStatement.

[michael-o]

@arkanovicz has the expertise to review this.

[FrancisS]

Any chance this will get reviewed or should I just close it?