Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connection: fix statement leak from commiting transactions #300

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Adam-
Copy link

@Adam- Adam- commented Jun 20, 2018

Closing the connection with closeJdbcConnection() prevented close() from
closing registered statements. Closes #202.

Closing the connection with closeJdbcConnection() prevented close() from
closing registered statements. Closes aaberg#202.
@rlfaber
Copy link

rlfaber commented Sep 26, 2022

Sorry to repeat my comment from #202, but I have found this fix to be a good fix
for a problem I had with a very different test case than others perhaps. I was
testing sql2o with transactions using 1.6.0 on Quarkus with Agroal connection polling and noticed
Agroal leak warnings like: Datasource '': JDBC resources leaked: 2 ResultSet(s) and 2 Statement(s).
With this fix (#300), however, the warnings went away. I am eager to see this
get into master... and also eager for the next release with it... as this seems to
me like a very important fix.

@Adam-
Copy link
Author

Adam- commented Sep 26, 2022

Sorry to repeat my comment from #202, but I have found this fix to be a good fix for a problem I had with a very different test case than others perhaps. I was testing sql2o with transactions using 1.6.0 on Quarkus with Agroal connection polling and noticed Agroal leak warnings like: Datasource '': JDBC resources leaked: 2 ResultSet(s) and 2 Statement(s). With this fix (#300), however, the warnings went away. I am eager to see this get into master... and also eager for the next release with it... as this seems to me like a very important fix.

I was able to work around this in my code via:

query.executeBatch();
con.commit(false);

at every place that I open a transaction

@rlfaber
Copy link

rlfaber commented Sep 27, 2022

  1. There are still leaks with the above suggestion (batch), and I suspect the important difference is that I am testing with a quality leak detection connection pool, Quarkus Agroal, which is much better at leak detection than some other connection poolers (e.g. dbcp). I have found Agroal excellent at finding leaks in my own raw JDBC. https://agroal.github.io/. What connection pooler were you testing with?

  2. A second issue is that using batch is not a sufficient solution (even if it did work).

  3. I applied this pull request to sql2o and it solves the transaction leak issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

commit(true) leaks Statements/cursors when using connectionPool
2 participants