[
https://issues.apache.org/jira/browse/VELOCITY-989?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17950412#comment-17950412
]
Francis M Schlimmer edited comment on VELOCITY-989 at 5/9/25 2:06 AM:
----------------------------------------------------------------------
Turns out the 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, so it doesn't have the
issue above, 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.
was (Author: JIRAUSER309569):
Turns out the 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, so it doesn't have the
issue above, 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 is not
standardized and varies accross 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.
> Multiple connection leaks in DataSourceResourceLoader
> -----------------------------------------------------
>
> Key: VELOCITY-989
> URL: https://issues.apache.org/jira/browse/VELOCITY-989
> Project: Velocity
> Issue Type: Bug
> Reporter: Francis M Schlimmer
> Priority: Major
>
> The first case is in the implementation of FilterReader. It attempts to get a
> statement 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.
>
> Pull request here [https://github.com/apache/velocity-engine/pull/57]
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org
