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

fix: EXPOSED-373 Close ResultSet before closing Statement to suppress Agroal leak warning #2247

Merged
merged 2 commits into from
Sep 23, 2024

Conversation

ivan-gomes
Copy link
Contributor

@ivan-gomes ivan-gomes commented Sep 21, 2024

Description

Summary of the change:

Close ResultSet before closing Statement. Motivation is to suppress "JDBC resource leaked" warning when used with agroal.

Detailed description:
Agroal, a popular javax.sql.DataSource implementation as used by Quarkus and others, issues a "leak report" before automatically closing tracked ResultSets and Statements. If a Statement is closed before related ResultSets are closed it will trigger a leak report - ref.

ex.

2024-09-21 11:14:47,325 WARN  [io.agr.pool] (executor-thread-1) Datasource '<default>': JDBC resources leaked: 4 ResultSet(s) and 0 Statement(s)

This warning occurs on every query with results and can cause developers concern - ref.

The following is a trace of the "leaked" ResultSet being accounted.

addLeakedResultSets:804, ConnectionWrapper (io.agroal.pool.wrapper)
closeTrackedResultSets:72, StatementWrapper (io.agroal.pool.wrapper)
close:84, StatementWrapper (io.agroal.pool.wrapper)
close:72, PreparedStatementWrapper (io.agroal.pool.wrapper)
setHasNext:96, AbstractQuery$ResultIterator (org.jetbrains.exposed.sql)
next:111, AbstractQuery$ResultIterator (org.jetbrains.exposed.sql)
next:91, AbstractQuery$ResultIterator (org.jetbrains.exposed.sql)
next:170, IterableExKt$mapLazy$1$iterator$2 (org.jetbrains.exposed.sql)
firstOrNull:279, CollectionsKt___CollectionsKt (kotlin.collections)
findById:95, EntityClass (org.jetbrains.exposed.dao)
findById:57, EntityClass (org.jetbrains.exposed.dao)
...

Type of Change

Please mark the relevant options with an "X":

  • Bug fix
  • New feature
  • Documentation update

Updates/remove existing public API methods:

  • Is breaking change

Affected databases:

  • MariaDB
  • Mysql5
  • Mysql8
  • Oracle
  • Postgres
  • SqlServer
  • H2
  • SQLite

Checklist

  • Unit tests are in place
  • The build is green (including the Detekt check)
  • All public methods affected by my PR has up to date API docs
  • Documentation for my change is up to date

Related Issues

EXPOSED-373

suppresses "JDBC resources leaked" warning with agroal
@bog-walk bog-walk self-assigned this Sep 22, 2024
@bog-walk bog-walk changed the title fix: close ResultSet before closing Statement - suppresses "JDBC resources leaked" warning with agroal fix: [EXPOSED-373] Close ResultSet before closing Statement to suppress Agroal leak warning Sep 22, 2024
Copy link
Member

@bog-walk bog-walk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for going ahead and opening this PR @ivan-gomes !

Please also add the same fix to the 2 other query ResultIterators: ReturningStatement and ExplainQuery.

Normally in an attempt to keep improving coverage, I'd also ask for a test, but I'll have to deal with that in the backlog. Based on my tests (using AgroalDataSource from agroal-pool with h2), the leak warnings only start from version 2.3+, which requires at minimum jdk 11 and we have to keep using jdk 8 atm.

Thanks again and, once the changes above are made, this fix should make it into the upcoming release.

@bog-walk
Copy link
Member

@ivan-gomes From my tests, I'm also seeing that insert statements that return auto-generated keys may produce a leak.
Please add rs?.close() either after the while block in InsertStatement.processResults() or after processResults() is called in executeInternal().

@bog-walk bog-walk changed the title fix: [EXPOSED-373] Close ResultSet before closing Statement to suppress Agroal leak warning fix: EXPOSED-373 Close ResultSet before closing Statement to suppress Agroal leak warning Sep 23, 2024
@ivan-gomes
Copy link
Contributor Author

ivan-gomes commented Sep 23, 2024

Thanks very much for going ahead and opening this PR @ivan-gomes !

Please also add the same fix to the 2 other query ResultIterators: ReturningStatement and ExplainQuery.

Normally in an attempt to keep improving coverage, I'd also ask for a test, but I'll have to deal with that in the backlog. Based on my tests (using AgroalDataSource from agroal-pool with h2), the leak warnings only start from version 2.3+, which requires at minimum jdk 11 and we have to keep using jdk 8 atm.

Thanks again and, once the changes above are made, this fix should make it into the upcoming release.

Done, and checked for any other unhandled occurrences of PreparedStatement.close() - didn't find any but you probably already knew that. :)

@ivan-gomes From my tests, I'm also seeing that insert statements that return auto-generated keys may produce a leak. Please add rs?.close() either after the while block in InsertStatement.processResults() or after processResults() is called in executeInternal().

Done, went with the latter so the assignment and cleanup are colocated.

@ivan-gomes ivan-gomes requested a review from bog-walk September 23, 2024 16:26
Copy link
Member

@bog-walk bog-walk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the contribution @ivan-gomes !

@bog-walk bog-walk merged commit 27baa04 into JetBrains:main Sep 23, 2024
3 checks passed
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.

2 participants