FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-105539: Emit ResourceWarning if sqlite3 database is not closed explicitly by erlend-aasland · Pull Request #108015 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

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

Merged
erlend-aasland merged 12 commits into
python:mainfrom
erlend-aasland:sqlite/resource-warning
Aug 22, 2023
ConversationCommits (12)ChecksFiles changed
Merged

gh-105539: Emit ResourceWarning if sqlite3 database is not closed explicitly#108015
erlend-aasland merged 12 commits into
python:mainfrom
erlend-aasland:sqlite/resource-warning

Conversation

erlend-aasland commented Aug 16, 2023 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

bedevere-bot mentioned this pull request Aug 16, 2023
Closed

erlend-aasland commented Aug 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

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

erlend-aasland commented Aug 17, 2023

Copy link
Copy Markdown
Contributor Author

cc. @felixxm

This was referenced Aug 17, 2023
Closed
Closed

erlend-aasland commented Aug 17, 2023

Copy link
Copy Markdown
Contributor Author

cc. @simonw

vstinner reviewed Aug 17, 2023
View reviewed changes
Comment thread Modules/_sqlite/connection.c Outdated
Comment thread Modules/_sqlite/connection.c Outdated
Comment thread Modules/_sqlite/connection.c
Comment thread Modules/_sqlite/connection.c
erlend-aasland mentioned this pull request Aug 17, 2023
Closed
erlend-aasland mentioned this pull request Aug 17, 2023
Merged
vstinner approved these changes Aug 17, 2023
View reviewed changes

vstinner 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Aug 17, 2023

Copy link
Copy Markdown
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 commented Aug 17, 2023

Copy link
Copy Markdown
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 commented Aug 17, 2023

Copy link
Copy Markdown
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 commented Aug 17, 2023

Copy link
Copy Markdown
Contributor

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 mentioned this pull request Aug 21, 2023
Merged

felixxm commented Aug 21, 2023

Copy link
Copy Markdown
Contributor

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 commented Aug 22, 2023

Copy link
Copy Markdown
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 merged commit 1a1bfc2 into python:main Aug 22, 2023
erlend-aasland deleted the sqlite/resource-warning branch August 22, 2023 11:10

erlend-aasland commented Aug 22, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews!

vstinner commented Aug 22, 2023

Copy link
Copy Markdown
Member

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

felixxm added a commit to felixxm/django that referenced this pull request Aug 23, 2023
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
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 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 mentioned this pull request Oct 22, 2023
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner approved these changes
berkerpeksag Awaiting requested review from berkerpeksag
+1 more reviewer
felixxm felixxm left review comments
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL