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

Use sqlite3_close_v2 to close databases. #557

Merged
merged 1 commit into from
Sep 15, 2024
Merged

Conversation

flavorjones
Copy link
Member

Close databases in a deferred manner if there are unclosed prepared statements. Previously closing a database while statements were open resulted in a BusyException.

See https://www.sqlite.org/c3ref/close.html for more context.

Close databases in a deferred manner if there are unclosed prepared
statements. Previously closing a database while statements were open
resulted in a `BusyException`.

See https://www.sqlite.org/c3ref/close.html for more context.
@flavorjones flavorjones merged commit ccddd85 into main Sep 15, 2024
130 checks passed
@flavorjones flavorjones deleted the flavorjones-use-close-v2 branch September 15, 2024 04:05
flavorjones added a commit that referenced this pull request Sep 19, 2024
This also restores the ability (now tested!) to call Database#close
successfully and defer its cleanup until after Statements are closed,
which was the promise of #557 and sqlite3_close_v2.

Closes #564
flavorjones added a commit that referenced this pull request Sep 19, 2024
This also restores the ability (now tested!) to call Database#close
successfully and defer its cleanup until after Statements are closed,
which was the promise of #557 and sqlite3_close_v2.

Closes #564
flavorjones added a commit that referenced this pull request Sep 19, 2024
This also restores the ability (now tested!) to call Database#close
successfully and defer its cleanup until after Statements are closed,
which was the promise of #557 and sqlite3_close_v2.

Closes #564
flavorjones added a commit that referenced this pull request Sep 19, 2024
This also restores the ability (now tested!) to call Database#close
successfully and defer its cleanup until after Statements are closed,
which was the promise of #557 and sqlite3_close_v2.

Closes #564
@tenderlove
Copy link
Member

Close databases in a deferred manner if there are unclosed prepared statements. Previously closing a database while statements were open resulted in a BusyException.

I know I'm late to the party (again, really sorry), but do we actually want to do this? If someone closes a database while they have statements open, that is a bug, so I'd expect an exception. We definitely need the "close without exception" behavior for fork safety, but could we just introduce a "force close" or something? Maybe like db.close(force: true)

@flavorjones
Copy link
Member Author

@tenderlove Been thinking about this.

I think what you're saying is: maybe people will be silently leaking db connections because they're not being reminded to clean up their statements by a failing Database#close call?

I guess I get it, and I acknowledge it's a change in behavior from previous versions. But also how is this any different than anything else in Ruby? It was only a bug before because GC didn't always clean up databases and statements; now this is handled well.

Is there a specific use case you're worried about leaking in non-obvious ways?

@tenderlove
Copy link
Member

Just to close the loop on this (since we discussed it privately already):

I think what you're saying is: maybe people will be silently leaking db connections because they're not being reminded to clean up their statements by a failing Database#close call?

I'm not worried that people will be silently leaking database connections. I'm worried that people will mistakenly leave statements in an "unfinished" state. Previously, users would have to specifically close their statements before they could close the database, effectively forcing them to say "I'm done with this statement now" (even if the statement had more rows to read, etc) before being allowed to close the db. After this patch, there's no feedback to users that they've not completed using a statement.

I guess I get it, and I acknowledge it's a change in behavior from previous versions. But also how is this any different than anything else in Ruby?

Yep, totally agree with this. I think it's what we agreed upon privately, and I just want to make sure we document it publicly. 😄

It was only a bug before because GC didn't always clean up databases and statements; now this is handled well.

I was thinking we could do both: have good GC cleanup and raise an exception if there are "dangling" statements. I think the behavior we have in main now is good. If not raising an exception breaks someone's code, I believe we can put back the old behavior and have good GC handling. But lets not consider that until we find out it's actually a problem. I'm worried that I worried over nothing (if that makes sense).

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