Skip to content

Close connection immediately after failed open#719

Merged
flavorjones merged 2 commits into
sparklemotion:mainfrom
katafrakt:close-after-failed-open
Jul 16, 2026
Merged

Close connection immediately after failed open#719
flavorjones merged 2 commits into
sparklemotion:mainfrom
katafrakt:close-after-failed-open

Conversation

@katafrakt

@katafrakt katafrakt commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Per documentation sqlite3_close_v2 should be called to free up resources even after sqlite3_open (and family) fails:

Whether or not an error occurs when it is opened, resources associated with the database connection handle should be released by passing it to sqlite3_close() when it is no longer required.

This is a bit verbose, because we need to get the message before closing and pass to CHECK_MSG. Alternative would be to keep using CHECK, but then a generic message would be printed.

This is required per the documentation, otherwise can result in memory
leaks due to unfreed resources.
@katafrakt katafrakt changed the title Close connection after fialed open Close connection after failed open Jul 15, 2026
@flavorjones

Copy link
Copy Markdown
Member

@katafrakt Thanks for the PR. Reading the docs, this does sound like the right thing to do.

Can I ask how you ran into this? Is there an easy way to reproduce the underlying failure conditions? I'd like to try to write a test and run it under valgrind/ASan to make sure memory leaks are all quashed.

@flavorjones

flavorjones commented Jul 16, 2026

Copy link
Copy Markdown
Member

Ah - ignore my question, I see it's as easy as opening a nonexistent file. I'll try to put a test around this, but the change itself seems fine. Thanks!

Worth noting: in the current codebase the memory is eventually cleaned up when the object is GCed, but this change would make that cleanup happen immediately which is definitely better.

@flavorjones flavorjones changed the title Close connection after failed open Close connection immediately after failed open Jul 16, 2026
Specifically I want valgrind to be checking for correctness.
Also add a changelog entry.
@flavorjones
flavorjones merged commit 2d7649d into sparklemotion:main Jul 16, 2026
135 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