fix: finish the PGconn upon connection failure

Attaching the non-finished PGconn to exceptions raised in connect() is
causing problem, as described in issue #565, because the PGconn might
not get finished soon enough in application code and the socket would
remain open.

On the other hand, just removing the pgconn attribute from Error would
be a breaking change and we'd loose the inspection features introduced
in commit 9220293dc023b757f2a57702c14b1460ff8f25b0.

As an alternative, we define a new PGconn implementation that is
error-specific: it captures all attributes of the original PGconn and
fails upon other operations (methods call). Some attributes have a
default value since they are not available in old PostgreSQL versions.

Finally, the (real) PGconn is finished before raising exception in
generators.connect().

diff --git a/docs/api/errors.rst b/docs/api/errors.rst
index 2fca7c6b284a8e5467bbdfc6b17f404cc43c9596..6d3b2aab5496fae3fa1eb86567952e4ce385fba6 100644 (file)
--- a/docs/api/errors.rst
+++ b/docs/api/errors.rst
@@ -50,10 +50,12 @@ These classes are exposed both by this module and the root `psycopg` module.
 
     .. autoattribute:: pgconn
 
-        Most likely it will be in `~psycopg.pq.ConnStatus.BAD` state;
+        It has been closed and will be in `~psycopg.pq.ConnStatus.BAD` state;
         however it might be useful to verify precisely what went wrong, for
         instance checking the `~psycopg.pq.PGconn.needs_password` and
         `~psycopg.pq.PGconn.used_password` attributes.
+        Attempting to operate this connection will raise an
+        :exc:`OperationalError`.
 
         .. versionadded:: 3.1
 

diff --git a/docs/news.rst b/docs/news.rst
index 1f2377717c72e78f4798861845c3960eed56f961..32da4ff5d291da78b8ed5818058d6f9e69318728 100644 (file)
--- a/docs/news.rst
+++ b/docs/news.rst
@@ -16,6 +16,10 @@ Psycopg 3.1.10
 - Fix prepared statement cache validation when exiting pipeline mode (or
   `~Cursor.executemany()`) in case an error occurred within the pipeline
   (:ticket:`#585`).
+- Fix `connect()` to avoid "leaking" an open `~pq.PGconn` attached to the
+  `OperationalError` in case of connection failure. `Error.pgconn` is now a
+  shallow copy of the real libpq connection, and the latter is closed before
+  the exception propagates (:ticket:`#565`).
 
 Current release
 ---------------

diff --git a/psycopg/psycopg/errors.py b/psycopg/psycopg/errors.py
index 8fe13f1d0ee78352423a2a657616baabb019e2ff..d0a08b7249a6b97412a523713fb7569c8653e6a4 100644 (file)
--- a/psycopg/psycopg/errors.py
+++ b/psycopg/psycopg/errors.py
@@ -18,19 +18,202 @@ DBAPI-defined Exceptions are defined in the following hierarchy::
 
 # Copyright (C) 2020 The Psycopg Team
 
-from typing import Any, Dict, Optional, Sequence, Tuple, Type, Union
+from dataclasses import dataclass, field, fields
+from typing import Any, Callable, Dict, List, NoReturn, Optional, Sequence, Tuple, Type
+from typing import Union, TYPE_CHECKING
 from typing_extensions import TypeAlias
 from asyncio import CancelledError
 
 from .pq.abc import PGconn, PGresult
-from .pq._enums import DiagnosticField
+from .pq._enums import ConnStatus, DiagnosticField, PipelineStatus, TransactionStatus
 from ._compat import TypeGuard
 
+if TYPE_CHECKING:
+    from .pq.misc import PGnotify, ConninfoOption
+
 ErrorInfo: TypeAlias = Union[None, PGresult, Dict[int, Optional[bytes]]]
 
 _sqlcodes: Dict[str, "Type[Error]"] = {}
 
 
+@dataclass
+class FinishedPGconn:
+    """Finished libpq connection.
+
+    Attributes are set from a real `~pscopg.pq.PGconn` but any operations will
+    raise an `~psycopg.OperationalError`.
+    """
+
+    info: List["ConninfoOption"] = field(default_factory=list)
+
+    db: bytes = b""
+    user: bytes = b""
+    password: bytes = b""
+    host: bytes = b""
+    hostaddr: bytes = b""
+    port: bytes = b""
+    tty: bytes = b""
+    options: bytes = b""
+    status: int = ConnStatus.BAD.value
+    transaction_status: int = TransactionStatus.UNKNOWN.value
+    pipeline_status: int = PipelineStatus.OFF.value
+
+    error_message: bytes = b""
+    server_version: int = 0
+
+    backend_pid: int = 0
+    needs_password: bool = False
+    used_password: bool = False
+    ssl_in_use: bool = False
+
+    nonblocking: int = 0
+
+    notice_handler: Optional[Callable[["PGresult"], None]] = None
+    notify_handler: Optional[Callable[["PGnotify"], None]] = None
+
+    @staticmethod
+    def _raise() -> NoReturn:
+        raise OperationalError("the connection is closed")
+
+    @classmethod
+    def connect(cls, *args: Any) -> NoReturn:
+        raise TypeError(f"{cls} is unusable")
+
+    @classmethod
+    def connect_start(cls, *args: Any) -> NoReturn:
+        raise TypeError(f"{cls} is unusable")
+
+    def connect_poll(self) -> NoReturn:
+        self._raise()
+
+    def finish(self) -> None:
+        pass
+
+    def reset(self) -> NoReturn:
+        self._raise()
+
+    def reset_start(self) -> NoReturn:
+        self._raise()
+
+    def reset_poll(self) -> NoReturn:
+        self._raise()
+
+    @classmethod
+    def ping(cls, *args: Any) -> NoReturn:
+        raise TypeError(f"{cls} is unusable")
+
+    def parameter_status(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    @property
+    def socket(self) -> NoReturn:
+        self._raise()
+
+    def exec_(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def send_query(self, *args: Any) -> None:
+        self._raise()
+
+    def exec_params(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def send_query_params(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def send_prepare(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def send_query_prepared(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def prepare(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def exec_prepared(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def describe_prepared(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def send_describe_prepared(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def describe_portal(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def send_describe_portal(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def get_result(self) -> NoReturn:
+        self._raise()
+
+    def consume_input(self) -> NoReturn:
+        self._raise()
+
+    def is_busy(self) -> NoReturn:
+        self._raise()
+
+    def flush(self) -> NoReturn:
+        self._raise()
+
+    def set_single_row_mode(self) -> NoReturn:
+        self._raise()
+
+    def get_cancel(self) -> NoReturn:
+        self._raise()
+
+    def notifies(self) -> NoReturn:
+        self._raise()
+
+    def put_copy_data(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def put_copy_end(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def get_copy_data(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def trace(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def set_trace_flags(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def untrace(self) -> NoReturn:
+        self._raise()
+
+    def encrypt_password(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def make_empty_result(self, *args: Any) -> NoReturn:
+        self._raise()
+
+    def enter_pipeline_mode(self) -> NoReturn:
+        self._raise()
+
+    def exit_pipeline_mode(self) -> NoReturn:
+        self._raise()
+
+    def pipeline_sync(self) -> NoReturn:
+        self._raise()
+
+    def send_flush_request(self) -> NoReturn:
+        self._raise()
+
+
+def finish_pgconn(pgconn: PGconn) -> PGconn:
+    args = {}
+    for f in fields(FinishedPGconn):
+        try:
+            args[f.name] = getattr(pgconn, f.name)
+        except Exception:
+            pass
+    pgconn.finish()
+    return FinishedPGconn(**args)
+
+
 class Warning(Exception):
     """
     Exception raised for important warnings.

diff --git a/psycopg/psycopg/generators.py b/psycopg/psycopg/generators.py
index 584fe47bf70e768ba4c4e673cb01a22766452dcd..81b52a2afc5d4dafe2618133df15146e3d522748 100644 (file)
--- a/psycopg/psycopg/generators.py
+++ b/psycopg/psycopg/generators.py
@@ -76,10 +76,12 @@ def _connect(conninfo: str) -> PQGenConn[PGconn]:
             encoding = conninfo_encoding(conninfo)
             raise e.OperationalError(
                 f"connection failed: {pq.error_message(conn, encoding=encoding)}",
-                pgconn=conn,
+                pgconn=e.finish_pgconn(conn),
             )
         else:
-            raise e.InternalError(f"unexpected poll status: {status}", pgconn=conn)
+            raise e.InternalError(
+                f"unexpected poll status: {status}", pgconn=e.finish_pgconn(conn)
+            )
 
     conn.nonblocking = 1
     return conn

diff --git a/psycopg_c/psycopg_c/_psycopg/generators.pyx b/psycopg_c/psycopg_c/_psycopg/generators.pyx
index 9ce9e54f74f44034e50c4b970f9c6c1da5d02d13..fbd901385992b3c83e7d4f22d82cb187fa61db0e 100644 (file)
--- a/psycopg_c/psycopg_c/_psycopg/generators.pyx
+++ b/psycopg_c/psycopg_c/_psycopg/generators.pyx
@@ -56,11 +56,12 @@ def connect(conninfo: str) -> PQGenConn[abc.PGconn]:
             encoding = conninfo_encoding(conninfo)
             raise e.OperationalError(
                 f"connection failed: {error_message(conn, encoding=encoding)}",
-                pgconn=conn
+                pgconn=e.finish_pgconn(conn),
             )
         else:
             raise e.InternalError(
-                f"unexpected poll status: {poll_status}", pgconn=conn
+                f"unexpected poll status: {poll_status}",
+                pgconn=e.finish_pgconn(conn),
             )
 
     conn.nonblocking = 1

diff --git a/tests/test_errors.py b/tests/test_errors.py
index b2ca66c1273b0c41de98239d96eef95f5ab1de3a..f9f33f141c6dbf0c563e645a2b9ff78656b6d203 100644 (file)
--- a/tests/test_errors.py
+++ b/tests/test_errors.py
@@ -12,6 +12,18 @@ from .utils import eur, gc_collect
 from .fix_crdb import is_crdb
 
 
+def test_finishedpgconn(pgconn):
+    with pytest.raises(TypeError):
+        e.FinishedPGconn.connect()
+
+    assert pgconn.socket
+    finished = e.finish_pgconn(pgconn)
+    with pytest.raises(e.OperationalError, match="connection is closed"):
+        finished.socket
+    with pytest.raises(e.OperationalError, match="connection is closed"):
+        pgconn.socket
+
+
 @pytest.mark.crdb_skip("severity_nonlocalized")
 def test_error_diag(conn):
     cur = conn.cursor()

diff --git a/tests/test_generators.py b/tests/test_generators.py
index f9b77616fd80aee18c51abf1c4dd4bc0013ff5a6..ecb8da987182ca7d626f3cb8c0a9c4608f3fc3e6 100644 (file)
--- a/tests/test_generators.py
+++ b/tests/test_generators.py
@@ -36,6 +36,12 @@ def test_connect_operationalerror_pgconn(generators, dsn, monkeypatch):
     pgconn = excinfo.value.pgconn
     assert pgconn is not None
     assert pgconn.needs_password
+    assert b"fe_sendauth: no password supplied" in pgconn.error_message
+    assert pgconn.status == pq.ConnStatus.BAD.value
+    assert pgconn.transaction_status == pq.TransactionStatus.UNKNOWN.value
+    assert pgconn.pipeline_status == pq.PipelineStatus.OFF.value
+    with pytest.raises(psycopg.OperationalError, match="connection is closed"):
+        pgconn.exec_(b"select 1")
 
 
 @pytest.fixture