-
Notifications
You must be signed in to change notification settings - Fork 619
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
jdbc potential memory leak #938
Comments
openjdk version "17" 2021-09-14 OpenJDK Runtime Environment (build 17+35-2724) OpenJDK 64-Bit Server VM (build 17+35-2724, mixed mode, sharing)
…________________________________
From: Rhys Williams ***@***.***>
Sent: Tuesday, July 18, 2023 11:02:01 AM
To: xerial/sqlite-jdbc ***@***.***>
Cc: Mhanna AbuTareef ***@***.***>; Author ***@***.***>
Subject: Re: [xerial/sqlite-jdbc] jdbc potential memory leak (Issue #938)
I am potentially experiencing a similar issue on Java 11.0.19, but not Java 11.0.16. Which version of Java are you running?
—
Reply to this email directly, view it on GitHub<#938 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABAZ4TA3EXWS7WKILN43YFLXQY7HTANCNFSM6AAAAAA2I4TA2U>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
* CAUTION: This email originated from outside of K2view *
|
@gotson can anyone shed light on this? |
Someone would need to provide evidence of where the leak happens. |
@mhanna I had exactly the same problem as you. A program running for 6+ hours doing requests and requests to an SQLite db. The debian box has 6 GB of RAM and a variant of the program that does not make use of SQLite runs with less than 512MB. After inspecting with VisualVM I found out that the instances of Simply closing the Statement calling I assume not closing the statement leaves some memory out of the JVM to remain allocated and when the |
Can you share the code you used? Try using try-with-resources to have your statements always automatically closed. |
On my end it was less practical to use a try-with-resources due to a catch and error forwarding nightmare, there was a But the JavaDoc for Statement/Prepared's
So technically, while not a good practice, it should not leak memory if not done. I have no proof of causality, but adding the close no matter what the outcome of the execution eliminated the memory leak. Example based on the code that caused the leak. It's not ideal but it's the code I have to work with. The try-catch is removed for clarity. // columns is a String[], value is Map<String, String>, query is String
PreparedStatement pStmt = connection.prepareStatement(query);
for (int i = 1; i <= columns.length; ++i) {
pStmt.setString(i, this.values.get(columns[i - 1]));
}
pStmt.execute();
// This line fixes it
pStmt.close(); To give a bit of context, this code is called in average 270 per file we process and we process around 4000 per run. A run lasts 8 hours. So over the course of those 8 hours, a little bit over a million times. The leak will likely not be noticeable for normal loads and only becomes a problem in large scale situations. |
I can confirm that try-resource blocks (specifically around Here is a commit I did that fixed the memory leak for my project: |
The question is: when are the statement objects closed? |
Based on the observation that it's not If you look at the protected void finalize() throws Throwable {
super.finalize();
try {
close();
} catch (SQLException e) {
// Ignore errors
}
} I was not able to find such usage of the However since finalization is deprecated in release 18 through JEP 421 I don't know what would be the proper way of cleaning underlying resources upon garbage collection of the Statement objects. The documentation mentions the Cleaner class but I've never actually used it. The JEP 421 also mentions the use of What's interesting is that I've seen numerous example in internal codebases but also other projects where JDBC statements are not closed (the one I talk here being one of them), and the documentation does not seem to state that it's as important as properly closing filesystem I/O for example. In fact back when I had a Java course we were never told about the need to close JDBC statements. |
Connection and Statement are AutoCloseable. So we should really care about closing them in order to avoid leaks (ideally with try-with-resources). Most of this confusion about not closing Statements really comes mostly from examples in the docs and code that completely forget to close them (like in readme.md and in demo/Sample.java). I strongly believe we should update every example in the project to also correctly close Statements (not only Connections) so then they would not induce or mislead into leaking issues. |
as always, PRs are welcome :) |
Submitted PR #1073 . |
will have a look when time allows, thanks ! |
🎉 This issue has been resolved in |
Describe the bug
Using a yourkit to capture memory used by java application that uses thousands of prepared statements and non prepared statements to sqlite, i can see that the NativeDB's safeStmt set is much more than the statements is should track.. see screenshot
To Reproduce
Maybe run many prepare/non-prepare statements
Expected behavior
A clear and concise description of what you expected to happen.
Logs
Environment (please complete the following information):
Do you see any issue here?
The text was updated successfully, but these errors were encountered: