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

5 files changed, 37 insertions, 39 deletions

diff --git a/Lib/sqlite3/test/dbapi.py b/Lib/sqlite3/test/dbapi.py
index ab33135..77fafe0 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 @@ class ConnectionTests(unittest.TestCase):
         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 8c60bdc..520a5b9 100644
--- a/Lib/sqlite3/test/hooks.py
+++ b/Lib/sqlite3/test/hooks.py
@@ -254,11 +254,15 @@ class TraceCallbackTests(unittest.TestCase):
         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 fd4e619..8196e3c 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 083356f..209c80d 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 62c4dc3..c1a5677 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));