GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-105539: Emit ResourceWarning if sqlite3 database is not closed explicitly by erlend-aasland · Pull Request #108015 · python/cpython

Conversation

@erlend-aasland

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: Emit resource warning if sqlite3 fails to close the database #105539

📚 Documentation preview 📚: https://cpython-previews--108015.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@erlend-aasland
pythongh-105539: Emit ResourceWarning if sqlite3 database is not clos…
4a8d209 
…ed explicitly
@erlend-aasland
erlend-aasland requested a review from vstinner August 16, 2023 12:30
@bedevere-bot bedevere-bot mentioned this pull request Aug 16, 2023
Emit resource warning if sqlite3 fails to close the database #105539
Closed
@erlend-aasland

erlend-aasland commented Aug 16, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner

This comment was marked as outdated.

Sign in to view

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

This comment was marked as outdated.

Sign in to view
erlend-aasland added 4 commits August 16, 2023 15:10
@erlend-aasland
Fix Sphinx warning
c00bdcf
@erlend-aasland
Pull in main
8075485
@erlend-aasland
Add back test
a12b7a4
@erlend-aasland
Fix indent
d102ea0
@erlend-aasland
erlend-aasland marked this pull request as ready for review August 17, 2023 07:02
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner August 17, 2023 07:02
@bedevere-bot bedevere-bot added the awaiting core review label Aug 17, 2023
@erlend-aasland
erlend-aasland requested a review from vstinner August 17, 2023 07:02
@erlend-aasland

erlend-aasland commented Aug 17, 2023

Copy link
Copy Markdown
Contributor Author

cc. @felixxm
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Aug 17, 2023
SQLite connections are not closed when calling .close() in 3.11 #103837
Closed
Potential regression in Python 3.11 (multiprocess shutdown?) #97641
Closed
@erlend-aasland

erlend-aasland commented Aug 17, 2023

Copy link
Copy Markdown
Contributor Author

cc. @simonw
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Aug 17, 2023
View reviewed changes
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland erlend-aasland mentioned this pull request Aug 17, 2023
sqlite3: some code paths ignore exceptions #108083
Closed
erlend-aasland added 3 commits August 17, 2023 13:02
@erlend-aasland
Pull in main
c573f15
@erlend-aasland
Address review: move finalizing check from the close helper to the fi…
8ebc8a7 
…nalizer
@erlend-aasland
Fix pointer deref and move exception guard
b67a3c0
@erlend-aasland erlend-aasland mentioned this pull request Aug 17, 2023
gh-108083: Don't ignore exceptions in sqlite3.Connection.__init__() and .close() #108084
Merged
@erlend-aasland
Slightly more informative error message
980d4d2
vstinner
vstinner approved these changes Aug 17, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 17, 2023
@erlend-aasland

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland
Only remove callbacks if there is a database to close
03a034d
@erlend-aasland

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@felixxm

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 🏖️
erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland added 2 commits August 21, 2023 10:44
@erlend-aasland
Pull in main
6f6cd82
@erlend-aasland
Fix merge
744bd1d
@felixxm felixxm mentioned this pull request Aug 21, 2023
Fixed ResourceWarning from unclosed SQLite connection on Python 3.13+. django/django#17178
Merged
@felixxm

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 reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland
erlend-aasland merged commit 1a1bfc2 into python:main Aug 22, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 22, 2023
@erlend-aasland
erlend-aasland deleted the sqlite/resource-warning branch August 22, 2023 11:10
@erlend-aasland

erlend-aasland commented Aug 22, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Aug 22, 2023

Copy link
Copy Markdown
Member

Congrats :-) IMO it's a nice enhancement.
erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland commented Aug 22, 2023
View reviewed changes
Comment thread Lib/test/test_sqlite3/test_dbapi.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
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
"unable to open database file" with Python 3.11 and PySide6 threads grantjenks/python-diskcache#295
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
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
+1 more reviewer
@felixxm felixxm felixxm left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@erlend-aasland @felixxm @vstinner @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.