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

Always call sqlite3_finalize in deallocate func #392

Merged

Conversation

haileys
Copy link
Contributor

@haileys haileys commented Aug 7, 2023

Prevents memory leak in the case that close is not called before a ResultSet is garbage collected.

close sets c->st to NULL immediately after calling sqlite3_finalize itself, so there is no double-free risk introduced with this change.

@flavorjones
Copy link
Member

flavorjones commented Aug 7, 2023

@haileys Thanks for submitting this PR.

Unfortunately, this is insufficient to prevent memory leaks -- I've added some tests that fail when rake test:valgrind is run (on a Linux system) to demonstrate the problem.

I think this is happening because we can't guarantee that the statement will be finalized before the database is closed. See Closing A Database Connection for more information on what happens when the order is reversed.

@flavorjones
Copy link
Member

I've rebased this onto current origin/main.

@tenderlove
Copy link
Member

I think this is happening because we can't guarantee that the statement will be finalized before the database is closed. See Closing A Database Connection for more information on wha

I suspect you're not wrong. But I think we might (possibly maybe) be able to do a GC trick that keeps the connection open 1 GC cycle past the statement. AFAIU, the only time we wouldn't be able to guarantee this order is in the case a database object and a statement object are collected at the same time. I think there's a way we can force the connection to stay alive for one more GC. IIRC we do this in CRuby for classes, but idk if the C APIs are public.

prevents memory leak when `close` is not called before a ResultSet is
garbage collected

also add coverage for statement resource cleanup

Co-authored-by: Mike Dalessio <mike.dalessio@gmail.com>
Copy link
Member

@tenderlove tenderlove left a comment

Choose a reason for hiding this comment

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

We might need more tricks in case a db and statement are collected simultaneously, but this is a good start.

@flavorjones flavorjones merged commit 5c53ad8 into sparklemotion:main Jan 3, 2024
106 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.

None yet

3 participants