From: Daniele Varrazzo <daniele.varrazzo@gmail.com>
Date: Sat, 13 Feb 2021 23:45:50 +0000 (+0100)
Subject: Make sure the pool can be deleted with no warning
X-Git-Tag: 3.0.dev0~87^2~73
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3c6b50a91bad955a68348cdc702408248029e84d;p=thirdparty%2Fpsycopg.git

Make sure the pool can be deleted with no warning

Make sure to delete reference loops between the pool and the maintenance
tasks after they have run.

Do not raise a warning if a connection in a pool is deleted without
being closed as this is a normal condition (imagining a pool being
created as a global object).
---

diff --git a/psycopg3/psycopg3/connection.py b/psycopg3/psycopg3/connection.py
index 8fef6f82b..d6af27d43 100644
--- a/psycopg3/psycopg3/connection.py
+++ b/psycopg3/psycopg3/connection.py
@@ -125,18 +125,23 @@ class BaseConnection(AdaptContext):
         pgconn.notice_handler = partial(BaseConnection._notice_handler, wself)
         pgconn.notify_handler = partial(BaseConnection._notify_handler, wself)
 
-        self._pool: Optional["ConnectionPool"] = None
+        # Attribute is only set if the connection is from a pool so we can tell
+        # apart a connection in the pool too (when _pool = None)
+        self._pool: Optional["ConnectionPool"]
 
     def __del__(self) -> None:
         # If fails on connection we might not have this attribute yet
         if not hasattr(self, "pgconn"):
             return
 
-        status = self.pgconn.transaction_status
-        if status == TransactionStatus.UNKNOWN:
+        # Connection correctly closed
+        if self.closed:
+            return
+
+        # Connection in a pool so terminating with the program is normal
+        if hasattr(self, "_pool"):
             return
 
-        status = TransactionStatus(status)
         warnings.warn(
             f"connection {self} was deleted while still open."
             f" Please use 'with' or '.close()' to close the connection",
@@ -466,7 +471,7 @@ class Connection(BaseConnection):
             self.commit()
 
         # Close the connection only if it doesn't belong to a pool.
-        if not self._pool:
+        if not getattr(self, "_pool", None):
             self.close()
 
     def close(self) -> None:
diff --git a/psycopg3/psycopg3/pool.py b/psycopg3/psycopg3/pool.py
index b4a9eaf17..769b008dd 100644
--- a/psycopg3/psycopg3/pool.py
+++ b/psycopg3/psycopg3/pool.py
@@ -121,9 +121,10 @@ class ConnectionPool:
         return conn
 
     def putconn(self, conn: Connection) -> None:
-        if conn._pool is not self:
-            if conn._pool:
-                msg = f"it comes from pool {conn._pool.name!r}"
+        pool = getattr(conn, "_pool", None)
+        if pool is not self:
+            if pool:
+                msg = f"it comes from pool {pool.name!r}"
             else:
                 msg = "it doesn't come from any pool"
             raise ValueError(
@@ -136,6 +137,7 @@ class ConnectionPool:
     def _return_connection(self, conn: Connection) -> None:
         # Remove the pool reference from the connection before returning it
         # to the state, to avoid to create a reference loop.
+        # Also disable the warning for open connection in conn.__del__
         conn._pool = None
 
         self._reset_transaction_status(conn)
@@ -219,6 +221,10 @@ class ConnectionPool:
             if isinstance(task, StopWorker):
                 return
 
+            # delete reference loops which may keep the pool alive
+            del task.pool
+            del task
+
     def _connect(self) -> Connection:
         """Return a connection configured for the pool."""
         conn = Connection.connect(self.conninfo, **self.kwargs)
@@ -260,7 +266,9 @@ class MaintenanceTask:
         logger.debug("task running: %s", self)
 
     def __repr__(self) -> str:
-        return f"{self.__class__.__name__}({self.pool.name})"
+        return (
+            f"<{self.__class__.__name__} {self.pool.name!r} at 0x{id(self):x}>"
+        )
 
 
 class StopWorker(MaintenanceTask):
@@ -293,7 +301,7 @@ class AddConnection(MaintenanceTask):
         super().__call__()
 
         conn = self.pool._connect()
-        conn._pool = self.pool  # make it acceptable
+        conn._pool = self.pool  # make it accepted by the pool
         self.pool.putconn(conn)
 
 
diff --git a/tests/test_pool.py b/tests/test_pool.py
index 66772a871..c4ec9e388 100644
--- a/tests/test_pool.py
+++ b/tests/test_pool.py
@@ -1,5 +1,6 @@
 import logging
-from time import time
+import weakref
+from time import time, sleep
 from threading import Thread
 
 import pytest
@@ -266,3 +267,17 @@ def test_putconn_wrong_pool(dsn):
     conn = p1.getconn()
     with pytest.raises(ValueError):
         p2.putconn(conn)
+
+
+def test_del_no_warning(dsn, recwarn):
+    p = pool.ConnectionPool(minconn=2)
+    with p.connection() as conn:
+        conn.execute("select 1")
+
+    while len(p._pool) < p.minconn:
+        sleep(0.01)
+
+    ref = weakref.ref(p)
+    del p
+    assert not ref()
+    assert not recwarn