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

Emit resource warning if sqlite3 fails to close the database #105539

Closed
erlend-aasland opened this issue Jun 8, 2023 · 7 comments · Fixed by #108360, #108015 or #108017
Closed

Emit resource warning if sqlite3 fails to close the database #105539

erlend-aasland opened this issue Jun 8, 2023 · 7 comments · Fixed by #108360, #108015 or #108017
Assignees
Labels
3.13 new features, bugs and security fixes topic-sqlite3 type-feature A feature request or enhancement

Comments

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jun 8, 2023

See #103837 (comment) and #103837 (comment):

EAA:

I still think we should consider changing the underlying API so we can emit a resource warning if close failed.

VS:

IMO sqlite3 should be modified to emit a ResourceWarning.

Currently, no ResourceWarning is emitted when a connection is not closed explicitly:

Linked PRs

@erlend-aasland erlend-aasland self-assigned this Jun 8, 2023
@erlend-aasland erlend-aasland added topic-sqlite3 3.13 new features, bugs and security fixes type-feature A feature request or enhancement labels Jun 8, 2023
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Aug 16, 2023
@vstinner
Copy link
Member

What is the consequences if a database is not closed explicitly and fails to be closed at Python exit? Is there a risk of losing data?

@erlend-aasland
Copy link
Contributor Author

What is the consequences if a database is not closed explicitly and fails to be closed at Python exit? Is there a risk of losing data?

If sqlite3_close() or sqlite3_close_v2() fails, there is of course a risk of losing data. However, we do a pretty good job at cleaning up the connection and its debris (cursors, statements, registered callbacks, etc.), so that risk is almost non-existent :)

@vstinner
Copy link
Member

By "fail to be closed", I was thinking about a database connection involved in a reference cycle which is impossible to break, and so close() is never called and the object is never destroyed.

@erlend-aasland
Copy link
Contributor Author

By "fail to be closed", I was thinking about a database connection involved in a reference cycle which is impossible to break, and so close() is never called and the object is never destroyed.

If the connection object is never destroyed (dealloc or finalize are never called), there is no way to detect that in the extension module that I know about. The proposed change involves both the finalizer and the dealloc callbacks, so I'm relying on them actually being called at some point.

erlend-aasland added a commit that referenced this issue Aug 17, 2023
…te3 tests (#108017)

- Use memory_database() helper
- Move test utility functions to util.py
- Add convenience memory database mixin
- Add check() helper for closed connection tests
@vstinner
Copy link
Member

Congrats :-) IMO it's a nice enhancement.

@erlend-aasland
Copy link
Contributor Author

Thanks for bringing the idea up in the first place, Victor! :)

@erlend-aasland
Copy link
Contributor Author

Apparently, I did not cover all cases in #108017; the following now emit resource warnings:

 ./python.exe -m test -R : test_sqlite3 -m ClosedCurTests 

felixxm added a commit to felixxm/cpython that referenced this issue Aug 23, 2023
…in tests.

Follow up to 1a1bfc2.

Co-authored-by: Erlend E. Aasland <erlend@python.org>
felixxm added a commit to felixxm/cpython that referenced this issue Aug 23, 2023
…in tests.

Follow up to 1a1bfc2.

Co-authored-by: Erlend E. Aasland <erlend@python.org>
erlend-aasland added a commit that referenced this issue Aug 23, 2023
…st_sqlite3 (#108360)

Follow up to 1a1bfc2.

Explicitly manage connections in:

- test_audit.test_sqlite3
- test_sqlite3.test_audit
- test_sqlite3.test_backup

Co-authored-by: Erlend E. Aasland <erlend@python.org>
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment