https://github.com/python/cpython/commit/d88b47b5a396aa8d66f9a0e6b13a0825d59... commit: d88b47b5a396aa8d66f9a0e6b13a0825d59d0eff branch: main author: Erlend Egeberg Aasland <erlend.aasland@innova.no> committer: miss-islington <31488909+miss-islington@users.noreply.github.com> date: 2021-06-03T09:38:19-07:00 summary: bpo-42213: Remove redundant cyclic GC hack in sqlite3 (GH-26517) The sqlite3 module now fully implements the GC protocol, so there's no need for this workaround anymore. - Add and use managed resource helper for connections using TESTFN - Sort test imports - Split open-tests into their own test case Automerge-Triggered-By: GH:vstinner files: M Lib/sqlite3/test/dbapi.py M Lib/sqlite3/test/hooks.py M Modules/_sqlite/cache.c M Modules/_sqlite/cache.h M Modules/_sqlite/connection.c diff --git a/Lib/sqlite3/test/dbapi.py b/Lib/sqlite3/test/dbapi.py index ab3313533940a..77fafe093002e 100644 --- a/Lib/sqlite3/test/dbapi.py +++ b/Lib/sqlite3/test/dbapi.py @@ -20,14 +20,26 @@ # misrepresented as being the original software. # 3. This notice may not be removed or altered from any source distribution. -import threading -import unittest +import contextlib import sqlite3 as sqlite import sys +import threading +import unittest from test.support.os_helper import TESTFN, unlink +# Helper for tests using TESTFN +@contextlib.contextmanager +def managed_connect(*args, **kwargs): + cx = sqlite.connect(*args, **kwargs) + try: + yield cx + finally: + cx.close() + unlink(TESTFN) + + class ModuleTests(unittest.TestCase): def test_api_level(self): self.assertEqual(sqlite.apilevel, "2.0", @@ -190,26 +202,27 @@ def test_in_transaction_ro(self): with self.assertRaises(AttributeError): self.cx.in_transaction = True +class OpenTests(unittest.TestCase): + _sql = "create table test(id integer)" + def test_open_with_path_like_object(self): """ Checks that we can successfully connect to a database using an object that is PathLike, i.e. has __fspath__(). """ - self.addCleanup(unlink, TESTFN) class Path: def __fspath__(self): return TESTFN path = Path() - with sqlite.connect(path) as cx: - cx.execute('create table test(id integer)') + with managed_connect(path) as cx: + cx.execute(self._sql) def test_open_uri(self): - self.addCleanup(unlink, TESTFN) - with sqlite.connect(TESTFN) as cx: - cx.execute('create table test(id integer)') - with sqlite.connect('file:' + TESTFN, uri=True) as cx: - cx.execute('insert into test(id) values(0)') - with sqlite.connect('file:' + TESTFN + '?mode=ro', uri=True) as cx: - with self.assertRaises(sqlite.OperationalError): - cx.execute('insert into test(id) values(1)') + with managed_connect(TESTFN) as cx: + cx.execute(self._sql) + with managed_connect(f"file:{TESTFN}", uri=True) as cx: + cx.execute(self._sql) + with self.assertRaises(sqlite.OperationalError): + with managed_connect(f"file:{TESTFN}?mode=ro", uri=True) as cx: + cx.execute(self._sql) class CursorTests(unittest.TestCase): diff --git a/Lib/sqlite3/test/hooks.py b/Lib/sqlite3/test/hooks.py index 8c60bdcf5d70a..520a5b9f11cd4 100644 --- a/Lib/sqlite3/test/hooks.py +++ b/Lib/sqlite3/test/hooks.py @@ -254,11 +254,15 @@ def trace(statement): self.addCleanup(unlink, TESTFN) con1 = sqlite.connect(TESTFN, isolation_level=None) con2 = sqlite.connect(TESTFN) - con1.set_trace_callback(trace) - cur = con1.cursor() - cur.execute(queries[0]) - con2.execute("create table bar(x)") - cur.execute(queries[1]) + try: + con1.set_trace_callback(trace) + cur = con1.cursor() + cur.execute(queries[0]) + con2.execute("create table bar(x)") + cur.execute(queries[1]) + finally: + con1.close() + con2.close() self.assertEqual(traced_statements, queries) diff --git a/Modules/_sqlite/cache.c b/Modules/_sqlite/cache.c index fd4e619f6a011..8196e3c578372 100644 --- a/Modules/_sqlite/cache.c +++ b/Modules/_sqlite/cache.c @@ -97,9 +97,6 @@ pysqlite_cache_init(pysqlite_Cache *self, PyObject *args, PyObject *kwargs) } self->factory = Py_NewRef(factory); - - self->decref_factory = 1; - return 0; } @@ -108,9 +105,7 @@ cache_traverse(pysqlite_Cache *self, visitproc visit, void *arg) { Py_VISIT(Py_TYPE(self)); Py_VISIT(self->mapping); - if (self->decref_factory) { - Py_VISIT(self->factory); - } + Py_VISIT(self->factory); pysqlite_Node *node = self->first; while (node) { @@ -124,9 +119,7 @@ static int cache_clear(pysqlite_Cache *self) { Py_CLEAR(self->mapping); - if (self->decref_factory) { - Py_CLEAR(self->factory); - } + Py_CLEAR(self->factory); /* iterate over all nodes and deallocate them */ pysqlite_Node *node = self->first; diff --git a/Modules/_sqlite/cache.h b/Modules/_sqlite/cache.h index 083356f93f9e4..209c80dcd54ad 100644 --- a/Modules/_sqlite/cache.h +++ b/Modules/_sqlite/cache.h @@ -52,10 +52,6 @@ typedef struct pysqlite_Node* first; pysqlite_Node* last; - - /* if set, decrement the factory function when the Cache is deallocated. - * this is almost always desirable, but not in the pysqlite context */ - int decref_factory; } pysqlite_Cache; extern PyTypeObject *pysqlite_NodeType; diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 62c4dc3bbb394..c1a5677d490ef 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -149,14 +149,6 @@ pysqlite_connection_init(pysqlite_Connection *self, PyObject *args, return -1; } - /* By default, the Cache class INCREFs the factory in its initializer, and - * decrefs it in its deallocator method. Since this would create a circular - * reference here, we're breaking it by decrementing self, and telling the - * cache class to not decref the factory (self) in its deallocator. - */ - self->statement_cache->decref_factory = 0; - Py_DECREF(self); - self->detect_types = detect_types; self->timeout = timeout; (void)sqlite3_busy_timeout(self->db, (int)(timeout*1000));