From: Denis Laxalde Date: Fri, 19 May 2023 07:06:55 +0000 (+0200) Subject: fix: finish the PGconn upon connection failure X-Git-Tag: 3.1.10~14^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f8d927985b5c65ce60e0150977e9519f555b1903;p=thirdparty%2Fpsycopg.git 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 2fca7c6b2..6d3b2aab5 100644 --- 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 1f2377717..32da4ff5d 100644 --- 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 8fe13f1d0..d0a08b724 100644 --- 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 584fe47bf..81b52a2af 100644 --- 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 9ce9e54f7..fbd901385 100644 --- 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 b2ca66c12..f9f33f141 100644 --- 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 f9b77616f..ecb8da987 100644 --- 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