GeezerVison is showing github.com — SQLite connections are not closed when calling .close() in 3.11 · Issue #103837 · python/cpython
SQLite connections are not closed when calling .close() in 3.11 #103837DescriptionBug report
In our tests we are using in-memory SQLite databases with a shared cache. This makes it possible to create multiple connections with the same connection string and see the same data, as recommended in In-Memory Databases. After updating from Python 3.10 to 3.11 our tests started failing.
The minimal reproduction case:
import sqlite3
import pytest
def write_value():
with sqlite3.connect("file:test?mode=memory&cache=shared", uri=True) as db:
db.execute("insert into Test (value) values ('hello')")
@pytest.fixture
def db():
conn_1 = sqlite3.connect("file:test?mode=memory&cache=shared", uri=True)
conn_1.execute("create table Test (value text, unique(value))")
yield write_value
conn_1.close()
def test_hello_1(db):
db()
def test_hello_2(db):
db()This works in 3.10 (and worked in 3.8 and 3.9), but fails in 3.11:
ERROR sqlite-connection-close.py::test_hello_2 - sqlite3.OperationalError: table Test already exists
The SQLite documentations includes:
The database is automatically deleted and memory is reclaimed when the last connection to the database closes.
We used a workaround mentioned in #97641 for now (calling gc.collect()).
I opened a new issue, since that one looks similar, but is Windows specific, while this one happens on Linux as well.
Your environment
- CPython versions tested on: 3.10.10, 3.11.3
- Operating system and architecture: both on Ubuntu 22.04 (from deadsnakes PPA) and Arch (AUR)
- SQLite versions: 3.37.2 and 3.41.2
Activityerlend-aasland commented on May 8, 2023 I cannot reproduce this on 3.11.3 or 3.12.0a7 (macOS). erlend-aasland commented on May 8, 2023 I cannot reproduce with 3.11 on Ubuntu 22.04 either. Unless you have a more deterministic reproducer, I'm not sure what to do with this :( WouterWouters commented on May 8, 2023 For me it fails on 3.11.3 (macOS). Running python -m pytest test.py results in:
test.py .E [100%]
================================================================================ ERRORS ================================================================================
____________________________________________________________________ ERROR at setup of test_hello_2 ____________________________________________________________________
@pytest.fixture
def db():
conn_1 = sqlite3.connect("file:test?mode=memory&cache=shared", uri=True)
> conn_1.execute("create table Test (value text, unique(value))")
E sqlite3.OperationalError: table Test already exists
test.py:12: OperationalError
======================================================================= short test summary info ========================================================================
ERROR test.py::test_hello_2 - sqlite3.OperationalError: table Test already exists
====================================================================== 1 passed, 1 error in 0.06s ======================================================================
erlend-aasland commented on May 8, 2023 erlend-aasland commented on May 8, 2023 What's the output of this script?
import sqlite3
cx = sqlite3.connect(":memory:")
for row in cx.execute("pragma compile_options"):
print(row[0])erlend-aasland commented on May 8, 2023 erlend-aasland commented on May 8, 2023 All right, I can reproduce this now. Looking into it. erlend-aasland commented on May 8, 2023 Yeah, I can confirm this is caused the cyclic reference between the statement cache and the connection. I'm not sure what's the best solution for this ATM. erlend-aasland commented on May 8, 2023 @vstinner, do you have tips for how we can handle this better? Some background:
In f461a7f, I tore out the _sqlite3 LRU cache and replaced it with functools.lru_cache. It seemed like a good idea at the time: less code to maintain, don't repeat yourself (don't repeat functools), and remove the old fishy refcount manipulation that was used to avoid the cyclic ref problems. With functools.lru_cache, that ref count hack could be removed. As Python 3.11 gets wider adoption, we now get reports (see also gh-97641) where third party test suites start failing because the sqlite3 connection objects are not collected, because of the ref cycle not being explicitly broken. Eryk Sun suggested wrapping functools.lru_cache in a shim and handling the cycle there, kinda like the old code; it may work. Previously, I was not convinced this was a real bug; Python delivers no guarantees on when objects are collected, so explicit resource management is required where deterministic output is expected (the case in test suites). jhoekx commented on May 8, 2023
Python delivers no guarantees on when objects are collected, so explicit resource management is required where deterministic output is expected (the case in test suites).
From the Python user perspective: we are calling .close() to achieve exactly that (the title given to this issue). erlend-aasland commented on May 8, 2023 .close does not guarantee that an object is collected or deallocated, and it never has. Perhaps you meant del con?
erlend-aasland commented on May 10, 2023 In the sqlite3 module, the underlying SQLite C API we use is sqlite3_close Ah, no we're using sqlite3_close_v2, not sqlite3_close. Perhaps using the latter will help improve things. Quoting the SQLite docs:
If the database connection is associated with unfinalized prepared statements, BLOB handlers, and/or unfinished sqlite3_backup objects then sqlite3_close() will leave the database connection open and return SQLITE_BUSY.
We're asserting that the sqlite3_closesqlite3_close_v2 API return SQLITE_OK1. This means that as far as we can tell (from the information given to us by the SQLite library), the connection was closed. It should not matter if the sqlite3.Connection object lives longer, as it now only wraps a closed sqlite3 * database pointer.
EDIT: I misremembered; we're using sqlite3_close_v2, not sqlite3_close. I'll switch to the former and see what happens.
Footnotes
-
come think of it, we should probably add code for emitting a ResourceWarning instead of just asserting the assumed return value ↩
erlend-aasland commented on May 10, 2023 Quick update after #103837 (comment):
Changing the underlying SQLite C API from sqlite3_close_v2 (which might not deallocate the connection immediately) to sqlite3_close (which require explicit resource management wrt. the SQLite data structures) did not help; SQLite reports SQLITE_OK, indicating that the connection was closed and deallocated. Unfortunately, the error still occurs. jhoekx commented on May 10, 2023 Ok. I think you can close this issue.
The documentation clearly spells out that what we're doing is based on incorrect assumptions.
I assumed that the context manager closed the connection, but it only manages a transaction. As @vstinner points out, we are not calling close() on the connection. Then I can agree that this is not a bug.
If I change the sample code to this, then the test passes:
import sqlite3
import pytest
def write_value():
db = sqlite3.connect("file:test?mode=memory&cache=shared", uri=True)
with db:
db.execute("insert into Test (value) values ('hello')")
db.close()
@pytest.fixture
def db():
conn_1 = sqlite3.connect("file:test?mode=memory&cache=shared", uri=True)
conn_1.execute("create table Test (value text, unique(value))")
yield write_value
conn_1.close()
def test_hello_1(db):
db()
def test_hello_2(db):
db()
Thanks and sorry for the 'panic'. Reacted by Erlend E. Aasland Reacted by Erlend E. Aasland and lifr0m erlend-aasland commented on May 10, 2023 I did not spot the missing close! That explains the problem, indeed 😃 I still think we should consider changing the underlying API so we can emit a resource warning if close failed. erlend-aasland commented on May 10, 2023 jhoekx commented on May 10, 2023 In our application we do have a context manager that closes the connection in a finally. It also sets the PRAGMAs we need before or after use and starts a transaction immediately on writes so we don't get hit with an SQLITE_BUSY when the transaction turns from reading to writing. That is why I made the mistake in the 'reproduction' case - we never need to close() it manually.
But obviously there is a place where we're not using that context manager in the proper way. We did have some errors in the past we could not explain on application startup where a database was busy during database migrations. That's also the place were we use that shared memory trick with in-memory databases for during tests. More work needed on our side.
Thanks again. Reacted by Erlend E. Aasland vstinner commented on May 11, 2023
changing the underlying API so we can emit a resource warning if close failed.
In the Python developer mode, FileIO.close() logs an unraisable exception on error. It's only in developer mode to avoid any backward compatibilty issue, but I always want to always log this unraisable exception.
ResourceWarning is more if the user didn't call close() explicitly (which is a different case). vstinner commented on May 11, 2023 IMO sqlite3 should be modified to emit a ResourceWarning.
Currently, no ResourceWarning is emitted when a connection is not closed explicitly:
$ ./python -Wdefault
Python 3.12.0a7+ (heads/main:fe694a6db6, May 10 2023, 00:29:39) [GCC 12.2.1 20221121 (Red Hat 12.2.1-4)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sqlite3
>>> con=sqlite3.connect("file:test?mode=memory&cache=shared", uri=True)
>>> con=None
>>> import gc; gc.collect()
32
erlend-aasland commented on May 11, 2023
IMO sqlite3 should be modified to emit a ResourceWarning.
Currently, no ResourceWarning is emitted when a connection is not closed explicitly:
Yes, I support this, as stated in #103837 (comment) vstinner commented on May 11, 2023 Oh ok. I was just confused by "if close failed" in your statement. erlend-aasland commented on May 12, 2023
Oh ok. I was just confused by "if close failed" in your statement.
Yeah, I got too side tracked to see the real issue. erlend-aasland commented on Aug 17, 2023
IMO sqlite3 should be modified to emit a ResourceWarning.
Currently, no ResourceWarning is emitted when a connection is not closed explicitly:
See gh-108015
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|