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.

Sign up for GitHub

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

Jump to bottom

gh-105539: Emit ResourceWarning if sqlite3 database is not closed explicitly #108015

Merged
merged 12 commits into from Aug 22, 2023
Merged

gh-105539: Emit ResourceWarning if sqlite3 database is not closed explicitly #108015

merged 12 commits into from Aug 22, 2023

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 16, 2023 •
edited by github-actions bot

@bedevere-bot bedevere-bot mentioned this pull request Aug 16, 2023
Closed
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Aug 16, 2023 •
edited

@vstinner: with this PR, sqlite3 emits a ResourceWarning if close() was not called explicitly on a sqlite3.Connection object before it is deleted. I did not (yet) add a resource warning for the case where close fails (future enhancement).

As you can see from the test changes, we've been pretty lax with resource handling in the sqlite3 test suite 😆

UPDATE: Tests updated in gh-108017

vstinner

This comment was marked as outdated.

Sign in to view

@erlend-aasland

This comment was marked as outdated.

Sign in to view
@erlend-aasland erlend-aasland marked this pull request as ready for review August 17, 2023 07:02
@erlend-aasland
Copy link
Contributor Author

cc. @felixxm

This was referenced Aug 17, 2023
Closed
Closed
@erlend-aasland
Copy link
Contributor Author

cc. @simonw

vstinner
vstinner reviewed Aug 17, 2023
View reviewed changes
Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Modules/_sqlite/connection.c Outdated Show resolved Hide resolved
Modules/_sqlite/connection.c Show resolved Hide resolved
Modules/_sqlite/connection.c Show resolved Hide resolved
@erlend-aasland erlend-aasland mentioned this pull request Aug 17, 2023
Closed
@erlend-aasland erlend-aasland mentioned this pull request Aug 17, 2023
Merged
vstinner
vstinner approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. This change does what it says. There is room for enhancement, but it can be addressed separately (@erlend-aasland already creates issues to track remaining points).

@erlend-aasland
Copy link
Contributor Author

Thanks for the review, Victor! I asked about #108015 (comment) on the core dev Discord. If it is an issue, we can fix it later.

@erlend-aasland
Copy link
Contributor Author

I'll wait with merging until I've heard from @simonw and @felixxm, in case they've got opinions about this.

@erlend-aasland
Copy link
Contributor Author

Note: I intend to add ResourceWarnings in case sqlite3_close*() fails as well, but I will do that in a follow-up PR. Currently, the sqlite3 module asserts that sqlite3_close*() did not fail, which is not optimal.

@felixxm
Copy link
Contributor

felixxm commented Aug 17, 2023

I'll wait with merging until I've heard from @simonw and @felixxm, in case they've got opinions about this.

Thanks for pinging me. I'm going to check it out next week, after my holidays 🏖️

felixxm added a commit to felixxm/django that referenced this pull request Aug 21, 2023
@felixxm
Fixed ResourceWarning from unclosed SQLite connection.
510dd4d 
- backends.tests.ThreadTests.test_default_connection_thread_local:
    on SQLite, close() doesn't explicitly close in-memory connections.

Check out python/cpython#108015.
@felixxm felixxm mentioned this pull request Aug 21, 2023
Merged
@felixxm
Copy link
Contributor

felixxm commented Aug 21, 2023

I'll wait with merging until I've heard from @simonw and @felixxm, in case they've got opinions about this.

Thanks for pinging me. I'm going to check it out next week, after my holidays 🏖️

This change doesn't break anything important in Django 👍 (at the first glance). I've started fixing ResourceWarnings in our tests, check out django/django#17178.

@erlend-aasland
Copy link
Contributor Author

This change doesn't break anything important in Django 👍 (at the first glance). I've started fixing ResourceWarnings in our tests, check out django/django#17178.

Great, thanks for chiming in.

@erlend-aasland erlend-aasland merged commit 1a1bfc2 into python:main Aug 22, 2023
22 checks passed
@erlend-aasland erlend-aasland deleted the sqlite/resource-warning branch August 22, 2023 11:10
@erlend-aasland
Copy link
Contributor Author

Thanks for the reviews!

@vstinner
Copy link
Member

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

felixxm added a commit to felixxm/django that referenced this pull request Aug 23, 2023
@felixxm
Fixed ResourceWarning from unclosed SQLite connection.
7224e39 
- backends.sqlite.tests.ThreadSharing.test_database_sharing_in_threads
- backends.tests.ThreadTests.test_default_connection_thread_local:
    on SQLite, close() doesn't explicitly close in-memory connections.
- servers.tests.LiveServerInMemoryDatabaseLockTest
- test_runner.tests.SQLiteInMemoryTestDbs.test_transaction_support

Check out python/cpython#108015.
felixxm added a commit to django/django that referenced this pull request Aug 23, 2023
@felixxm
Fixed ResourceWarning from unclosed SQLite connection on Python 3.13+.
dd45d52 
- backends.sqlite.tests.ThreadSharing.test_database_sharing_in_threads
- backends.tests.ThreadTests.test_default_connection_thread_local:
    on SQLite, close() doesn't explicitly close in-memory connections.
- servers.tests.LiveServerInMemoryDatabaseLockTest
- test_runner.tests.SQLiteInMemoryTestDbs.test_transaction_support

Check out python/cpython#108015.
@erlend-aasland erlend-aasland linked an issue Aug 23, 2023 that may be closed by this pull request
Emit resource warning if sqlite3 fails to close the database #105539
Closed
@jeandet jeandet mentioned this pull request Oct 22, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@felixxm felixxm felixxm left review comments

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag berkerpeksag is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Emit resource warning if sqlite3 fails to close the database
4 participants
@erlend-aasland @felixxm @vstinner @bedevere-bot