Skip to content

Always call sqlite3_finalize in deallocate func - #392

Merged
flavorjones merged 1 commit into
sparklemotion:mainfrom
haileys:resultset-close-memory-leak
Jan 3, 2024
Merged

Always call sqlite3_finalize in deallocate func#392
flavorjones merged 1 commit into
sparklemotion:mainfrom
haileys:resultset-close-memory-leak

Conversation

@haileys

@haileys haileys commented Aug 7, 2023

Copy link
Copy Markdown
Contributor

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

flavorjones commented Aug 7, 2023

Copy link
Copy Markdown
Member

@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 flavorjones closed this Jan 3, 2024
@flavorjones flavorjones reopened this Jan 3, 2024
@flavorjones
flavorjones force-pushed the resultset-close-memory-leak branch from 9574673 to e57e04b Compare January 3, 2024 22:30
@flavorjones

Copy link
Copy Markdown
Member

I've rebased this onto current origin/main.

@flavorjones
flavorjones requested a review from tenderlove January 3, 2024 22:31
@flavorjones
flavorjones force-pushed the resultset-close-memory-leak branch from e57e04b to 3ba5cd0 Compare January 3, 2024 22:34
@tenderlove

Copy link
Copy Markdown
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>
@flavorjones
flavorjones force-pushed the resultset-close-memory-leak branch from 3ba5cd0 to 0c24631 Compare January 3, 2024 22:35

@tenderlove tenderlove left a comment

Copy link
Copy Markdown
Member

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.

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.

3 participants