-
Notifications
You must be signed in to change notification settings - Fork 18
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 memory leak in free_res #39
Conversation
(this may be related to #29) |
This is great, thanks! |
I think a new release is due after this fix and the ones from your other PR. However I don't have an OCaml development environment right now. Would you like developer access to this repository? |
I can look into that time permitting, yes |
took me one year ocaml/opam-repository#21009 :) |
let start = handle_free (B.mysql_stmt_free_result_start raw) in | ||
let cont s = handle_free (B.mysql_stmt_free_result_cont raw s) in | ||
let () = match stmt.Common.Stmt.meta with None -> () | Some { res; _ } -> B.mysql_free_result res in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cannot be correct, since mysql_free_result
is a blocking function, and I think it is only meant for results from non-prepared queries. Isn't the mysql_stmt_free_result_start
and mysql_stmt_free_result_cont
above the correct functions to use?
I started to see ABRT and SEGV signals from the Caqti testsuite after upgrading to mariadb 1.1.5. Compiling against the master branch with this commit reverted, fixes that issue, though of that still leaves the original memory issue. |
do you have a stacktrace? |
In one case it terminates in OCaml:
In the other case, I get a core dump with backtrace from gdb:
|
This reverts commit 94659d3. see #39 (comment)
ftr I am using this code extensively and didn't observe any crashes.. Anyway released 1.1.6 with this commit reverted until this is properly investigated. |
Thanks. I had a look at it myself, without finding the source of the leak, though I'm wondering whether the slots for the bindings which are pre-allocated by prepare gets freed. |
Firstly, the problem with the original fix for the memory leak is that statement metadata can be used again after freeing. It's fine to free the result set, since it will be repopulated on the next execution, but the metadata is retrieved only on first execution. So if a statement is reused (ie, Secondly, it should actually be fine to use https://dev.mysql.com/doc/c-api/5.7/en/mysql-stmt-result-metadata.html I previously submitted a PR #41 which loads the metadata after each execution. This also fixes the memory leak, and should not have had the segfault issue because it avoids the use-after-free. |
@apeschar Thanks for your investigating.
What if the client calls |
There are two result sets:
For the statement result set it is necessary to use the non-blocking APIs because of the situations you mentioned; ie, not all rows have been read. For the metadata result set this is not applicable. Freeing the metadata should never block. |
CHANGES: - Added `Stmt.start_txn` (ocaml-community/ocaml-mariadb#59 by Corentin Leruth). - Added `Res.insert_id` as binding for `mysql_stmt_insert_id` (ocaml-community/ocaml-mariadb#58 by Corentin Leruth). - Updated to support recent OCaml versions (ocaml-community/ocaml-mariadb#45 by @kit-ty-kate). - Fixed too-early retrieval of statement metadata (ocaml-community/ocaml-mariadb#41 by Albert Peschar). - Fixed decoding bug for the integer type (ocaml-community/ocaml-mariadb#54 by Raman Varabets, tested by ocaml-community/ocaml-mariadb#61 by Corentin Leruth). - Fixed a memory leaks related to result metadata (ocaml-community/ocaml-mariadb#39 by Albert Peschar). - The build system is now dune and dune-configurator (ocaml-community/ocaml-mariadb#52 by Petter A. Urkedal) and some of the examples have been converted to a test suite (ocaml-community/ocaml-mariadb#60 by Petter A. Urkedal). - The project has been transferred to ocaml-community with Petter A. Urkedal as the new maintainer.
CHANGES: - Added `Stmt.start_txn` (ocaml-community/ocaml-mariadb#59 by Corentin Leruth). - Added `Res.insert_id` as binding for `mysql_stmt_insert_id` (ocaml-community/ocaml-mariadb#58 by Corentin Leruth). - Updated to support recent OCaml versions (ocaml-community/ocaml-mariadb#45 by @kit-ty-kate). - Fixed too-early retrieval of statement metadata (ocaml-community/ocaml-mariadb#41 by Albert Peschar). - Fixed decoding bug for the integer type (ocaml-community/ocaml-mariadb#54 by Raman Varabets, tested by ocaml-community/ocaml-mariadb#61 by Corentin Leruth). - Fixed a memory leaks related to result metadata (ocaml-community/ocaml-mariadb#39 by Albert Peschar). - The build system is now dune and dune-configurator (ocaml-community/ocaml-mariadb#52 by Petter A. Urkedal) and some of the examples have been converted to a test suite (ocaml-community/ocaml-mariadb#60 by Petter A. Urkedal). - The project has been transferred to ocaml-community with Petter A. Urkedal as the new maintainer.
CHANGES: - Added `Stmt.start_txn` (ocaml-community/ocaml-mariadb#59 by Corentin Leruth). - Added `Res.insert_id` as binding for `mysql_stmt_insert_id` (ocaml-community/ocaml-mariadb#58 by Corentin Leruth). - Updated to support recent OCaml versions (ocaml-community/ocaml-mariadb#45 by @kit-ty-kate). - Fixed too-early retrieval of statement metadata (ocaml-community/ocaml-mariadb#41 by Albert Peschar). - Fixed decoding bug for the integer type (ocaml-community/ocaml-mariadb#54 by Raman Varabets, tested by ocaml-community/ocaml-mariadb#61 by Corentin Leruth). - Fixed a memory leaks related to result metadata (ocaml-community/ocaml-mariadb#39 by Albert Peschar). - The build system is now dune and dune-configurator (ocaml-community/ocaml-mariadb#52 by Petter A. Urkedal) and some of the examples have been converted to a test suite (ocaml-community/ocaml-mariadb#60 by Petter A. Urkedal). - The project has been transferred to ocaml-community with Petter A. Urkedal as the new maintainer.
https://dev.mysql.com/doc/c-api/8.0/en/mysql-stmt-result-metadata.html
Repro case
dune
(devkit is used for Memory.reclaim) :testdb.ml
:before the fix:
(important number is the last bytes measurement, which is resident set size after malloc_release)
after the fix: