From 3c6b50a91bad955a68348cdc702408248029e84d Mon Sep 17 00:00:00 2001
From: Daniele Varrazzo <daniele.varrazzo@gmail.com>
Date: Sun, 14 Feb 2021 00:45:50 +0100
Subject: [PATCH] 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).
---
 psycopg3/psycopg3/connection.py | 15 ++++++++++-----
 psycopg3/psycopg3/pool.py       | 18 +++++++++++++-----
 tests/test_pool.py              | 17 ++++++++++++++++-
 3 files changed, 39 insertions(+), 11 deletions(-)

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
-- 
2.47.3