From: Daniele Varrazzo Date: Sat, 7 Oct 2023 22:01:57 +0000 (+0200) Subject: feat(pool) raise a warning if the pool is expected to be opened implicitly X-Git-Tag: pool-3.2.0~8^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=89bf99af261885f3bd01a0e1239625e63a416251;p=thirdparty%2Fpsycopg.git feat(pool) raise a warning if the pool is expected to be opened implicitly --- diff --git a/docs/news_pool.rst b/docs/news_pool.rst index b59ed4070..7cc79a815 100644 --- a/docs/news_pool.rst +++ b/docs/news_pool.rst @@ -16,6 +16,9 @@ psycopg_pool 3.2.0 (unreleased) - Add support for async `!reconnect_failed` callbacks in `AsyncConnectionPool` (:ticket:`#520`). - Make connection pool classes generic on the connection type (:ticket:`#559`). +- Raise a warning if a pool is used relying on an implicit `!open=True` and the + pool context is not used. In the future the default will become `!False` + (:ticket:`#659`). psycopg_pool 3.1.9 (unreleased) diff --git a/psycopg_pool/psycopg_pool/base.py b/psycopg_pool/psycopg_pool/base.py index b0e308136..a14f7a2d6 100644 --- a/psycopg_pool/psycopg_pool/base.py +++ b/psycopg_pool/psycopg_pool/base.py @@ -7,6 +7,7 @@ psycopg connection pool base class and functionalities. from time import monotonic from random import random from typing import Any, Dict, Optional, Tuple, TYPE_CHECKING +import warnings from psycopg import errors as e @@ -47,7 +48,6 @@ class BasePool: kwargs: Optional[Dict[str, Any]], min_size: int, max_size: Optional[int], - open: bool, name: Optional[str], timeout: float, max_waiting: int, @@ -96,6 +96,7 @@ class BasePool: self._opened = False self._closed = True + self._open_implicit = False def __repr__(self) -> str: return ( @@ -141,6 +142,13 @@ class BasePool: raise PoolClosed(f"the pool {self.name!r} is already closed") else: raise PoolClosed(f"the pool {self.name!r} is not open yet") + elif self._open_implicit: + warnings.warn( + f"the default for the {type(self).__name__} 'open' parameter will" + " become 'False' in a future release; please use open={True|False}" + " explicitly or use the pool as context manager", + DeprecationWarning, + ) def _check_pool_putconn(self, conn: "BaseConnection[Any]") -> None: pool = getattr(conn, "_pool", None) diff --git a/psycopg_pool/psycopg_pool/null_pool.py b/psycopg_pool/psycopg_pool/null_pool.py index 47643bb10..60c5dff34 100644 --- a/psycopg_pool/psycopg_pool/null_pool.py +++ b/psycopg_pool/psycopg_pool/null_pool.py @@ -32,7 +32,7 @@ class NullConnectionPool(_BaseNullConnectionPool, ConnectionPool[CT]): self: NullConnectionPool[Connection[TupleRow]], conninfo: str = "", *, - open: bool = ..., + open: bool | None = ..., configure: Optional[ConnectionCB[CT]] = ..., reset: Optional[ConnectionCB[CT]] = ..., kwargs: Optional[Dict[str, Any]] = ..., @@ -54,7 +54,7 @@ class NullConnectionPool(_BaseNullConnectionPool, ConnectionPool[CT]): self: NullConnectionPool[CT], conninfo: str = "", *, - open: bool = ..., + open: bool | None = ..., connection_class: Type[CT], configure: Optional[ConnectionCB[CT]] = ..., reset: Optional[ConnectionCB[CT]] = ..., @@ -76,7 +76,7 @@ class NullConnectionPool(_BaseNullConnectionPool, ConnectionPool[CT]): self, conninfo: str = "", *, - open: bool = True, + open: bool | None = None, connection_class: Type[CT] = cast(Type[CT], Connection), configure: Optional[ConnectionCB[CT]] = None, reset: Optional[ConnectionCB[CT]] = None, diff --git a/psycopg_pool/psycopg_pool/null_pool_async.py b/psycopg_pool/psycopg_pool/null_pool_async.py index 8db9ca7ae..a4a33e46a 100644 --- a/psycopg_pool/psycopg_pool/null_pool_async.py +++ b/psycopg_pool/psycopg_pool/null_pool_async.py @@ -29,7 +29,7 @@ class AsyncNullConnectionPool(_BaseNullConnectionPool, AsyncConnectionPool[ACT]) self: AsyncNullConnectionPool[AsyncConnection[TupleRow]], conninfo: str = "", *, - open: bool = ..., + open: bool | None = ..., configure: Optional[AsyncConnectionCB[ACT]] = ..., reset: Optional[AsyncConnectionCB[ACT]] = ..., kwargs: Optional[Dict[str, Any]] = ..., @@ -51,7 +51,7 @@ class AsyncNullConnectionPool(_BaseNullConnectionPool, AsyncConnectionPool[ACT]) self: AsyncNullConnectionPool[ACT], conninfo: str = "", *, - open: bool = ..., + open: bool | None = ..., connection_class: Type[ACT], configure: Optional[AsyncConnectionCB[ACT]] = ..., reset: Optional[AsyncConnectionCB[ACT]] = ..., @@ -73,7 +73,7 @@ class AsyncNullConnectionPool(_BaseNullConnectionPool, AsyncConnectionPool[ACT]) self, conninfo: str = "", *, - open: bool = True, + open: bool | None = None, connection_class: Type[ACT] = cast(Type[ACT], AsyncConnection), configure: Optional[AsyncConnectionCB[ACT]] = None, reset: Optional[AsyncConnectionCB[ACT]] = None, diff --git a/psycopg_pool/psycopg_pool/pool.py b/psycopg_pool/psycopg_pool/pool.py index 54125fcdb..8abecd6ed 100644 --- a/psycopg_pool/psycopg_pool/pool.py +++ b/psycopg_pool/psycopg_pool/pool.py @@ -44,7 +44,7 @@ class ConnectionPool(Generic[CT], BasePool): self: ConnectionPool[Connection[TupleRow]], conninfo: str = "", *, - open: bool = ..., + open: bool | None = ..., configure: Optional[ConnectionCB[CT]] = ..., reset: Optional[ConnectionCB[CT]] = ..., kwargs: Optional[Dict[str, Any]] = ..., @@ -66,7 +66,7 @@ class ConnectionPool(Generic[CT], BasePool): self: ConnectionPool[CT], conninfo: str = "", *, - open: bool = ..., + open: bool | None = ..., connection_class: Type[CT], configure: Optional[ConnectionCB[CT]] = ..., reset: Optional[ConnectionCB[CT]] = ..., @@ -88,7 +88,7 @@ class ConnectionPool(Generic[CT], BasePool): self, conninfo: str = "", *, - open: bool = True, + open: bool | None = None, connection_class: Type[CT] = cast(Type[CT], Connection), configure: Optional[ConnectionCB[CT]] = None, reset: Optional[ConnectionCB[CT]] = None, @@ -129,7 +129,6 @@ class ConnectionPool(Generic[CT], BasePool): kwargs=kwargs, min_size=min_size, max_size=max_size, - open=open, name=name, timeout=timeout, max_waiting=max_waiting, @@ -139,6 +138,9 @@ class ConnectionPool(Generic[CT], BasePool): num_workers=num_workers, ) + if open is None: + open = self._open_implicit = True + if open: self._open() @@ -439,6 +441,7 @@ class ConnectionPool(Generic[CT], BasePool): gather(sched_runner, *workers, timeout=timeout) def __enter__(self: _Self) -> _Self: + self._open_implicit = False self.open() return self diff --git a/psycopg_pool/psycopg_pool/pool_async.py b/psycopg_pool/psycopg_pool/pool_async.py index ab8cfa4d4..2077b425b 100644 --- a/psycopg_pool/psycopg_pool/pool_async.py +++ b/psycopg_pool/psycopg_pool/pool_async.py @@ -43,7 +43,7 @@ class AsyncConnectionPool(Generic[ACT], BasePool): self: AsyncConnectionPool[AsyncConnection[TupleRow]], conninfo: str = "", *, - open: bool = ..., + open: bool | None = ..., configure: Optional[AsyncConnectionCB[ACT]] = ..., reset: Optional[AsyncConnectionCB[ACT]] = ..., kwargs: Optional[Dict[str, Any]] = ..., @@ -65,7 +65,7 @@ class AsyncConnectionPool(Generic[ACT], BasePool): self: AsyncConnectionPool[ACT], conninfo: str = "", *, - open: bool = ..., + open: bool | None = ..., connection_class: Type[ACT], configure: Optional[AsyncConnectionCB[ACT]] = ..., reset: Optional[AsyncConnectionCB[ACT]] = ..., @@ -87,7 +87,7 @@ class AsyncConnectionPool(Generic[ACT], BasePool): self, conninfo: str = "", *, - open: bool = True, + open: bool | None = None, connection_class: Type[ACT] = cast(Type[ACT], AsyncConnection), configure: Optional[AsyncConnectionCB[ACT]] = None, reset: Optional[AsyncConnectionCB[ACT]] = None, @@ -128,7 +128,6 @@ class AsyncConnectionPool(Generic[ACT], BasePool): kwargs=kwargs, min_size=min_size, max_size=max_size, - open=open, name=name, timeout=timeout, max_waiting=max_waiting, @@ -138,6 +137,9 @@ class AsyncConnectionPool(Generic[ACT], BasePool): num_workers=num_workers, ) + if open is None: + open = self._open_implicit = True + if open: self._open() @@ -447,6 +449,7 @@ class AsyncConnectionPool(Generic[ACT], BasePool): await agather(sched_runner, *workers, timeout=timeout) async def __aenter__(self: _Self) -> _Self: + self._open_implicit = False await self.open() return self diff --git a/tests/pool/test_pool.py b/tests/pool/test_pool.py index 9ab1b01d2..f16d5480b 100644 --- a/tests/pool/test_pool.py +++ b/tests/pool/test_pool.py @@ -359,7 +359,7 @@ def test_fail_rollback_close(dsn, caplog, monkeypatch): def test_del_no_warning(dsn, recwarn): - p = pool.ConnectionPool(dsn, min_size=2) + p = pool.ConnectionPool(dsn, min_size=2, open=True) with p.connection() as conn: conn.execute("select 1") diff --git a/tests/pool/test_pool_async.py b/tests/pool/test_pool_async.py index 27b55bbe1..a49622062 100644 --- a/tests/pool/test_pool_async.py +++ b/tests/pool/test_pool_async.py @@ -363,7 +363,7 @@ async def test_fail_rollback_close(dsn, caplog, monkeypatch): async def test_del_no_warning(dsn, recwarn): - p = pool.AsyncConnectionPool(dsn, min_size=2) + p = pool.AsyncConnectionPool(dsn, min_size=2, open=True) async with p.connection() as conn: await conn.execute("select 1") diff --git a/tests/pool/test_pool_common.py b/tests/pool/test_pool_common.py index a6b152b64..e12433786 100644 --- a/tests/pool/test_pool_common.py +++ b/tests/pool/test_pool_common.py @@ -25,6 +25,8 @@ def pool_cls(request): def test_defaults(pool_cls, dsn): with pool_cls(dsn) as p: + assert p.open + assert not p.closed assert p.timeout == 30 assert p.max_idle == 10 * 60 assert p.max_lifetime == 60 * 60 @@ -52,6 +54,49 @@ def test_context(pool_cls, dsn): assert p.closed +def test_create_warning(pool_cls, dsn): + # No warning on explicit open + p = pool_cls(dsn, open=True) + try: + with p.connection(): + pass + finally: + p.close() + + # No warning on explicit close + p = pool_cls(dsn, open=False) + p.open() + try: + with p.connection(): + pass + finally: + p.close() + + # No warning on context manager + with pool_cls(dsn) as p: + with p.connection(): + pass + + # Warning on open not specified + with pytest.warns(DeprecationWarning): + p = pool_cls(dsn) + try: + with p.connection(): + pass + finally: + p.close() + + # Warning also if open is called explicitly on already implicitly open + with pytest.warns(DeprecationWarning): + p = pool_cls(dsn) + p.open() + try: + with p.connection(): + pass + finally: + p.close() + + def test_wait_closed(pool_cls, dsn): with pool_cls(dsn) as p: pass @@ -312,7 +357,7 @@ def test_del_stops_threads(pool_cls, dsn): def test_closed_getconn(pool_cls, dsn): - p = pool_cls(dsn, min_size=min_size(pool_cls)) + p = pool_cls(dsn, min_size=min_size(pool_cls), open=True) assert not p.closed with p.connection(): pass @@ -326,7 +371,7 @@ def test_closed_getconn(pool_cls, dsn): def test_close_connection_on_pool_close(pool_cls, dsn): - p = pool_cls(dsn, min_size=min_size(pool_cls)) + p = pool_cls(dsn, min_size=min_size(pool_cls), open=True) with p.connection() as conn: p.close() assert conn.closed @@ -351,7 +396,7 @@ def test_closed_queue(pool_cls, dsn): e1 = Event() e2 = Event() - p = pool_cls(dsn, min_size=min_size(pool_cls), max_size=1) + p = pool_cls(dsn, min_size=min_size(pool_cls), max_size=1, open=True) p.wait() success: List[str] = [] @@ -409,7 +454,7 @@ def test_open_context(pool_cls, dsn): def test_open_no_op(pool_cls, dsn): - p = pool_cls(dsn) + p = pool_cls(dsn, open=True) try: assert not p.closed p.open() @@ -423,7 +468,7 @@ def test_open_no_op(pool_cls, dsn): def test_reopen(pool_cls, dsn): - p = pool_cls(dsn) + p = pool_cls(dsn, open=True) with p.connection() as conn: conn.execute("select 1") p.close() diff --git a/tests/pool/test_pool_common_async.py b/tests/pool/test_pool_common_async.py index ff6dcc0bc..b368fab54 100644 --- a/tests/pool/test_pool_common_async.py +++ b/tests/pool/test_pool_common_async.py @@ -25,6 +25,8 @@ def pool_cls(request): async def test_defaults(pool_cls, dsn): async with pool_cls(dsn) as p: + assert p.open + assert not p.closed assert p.timeout == 30 assert p.max_idle == 10 * 60 assert p.max_lifetime == 60 * 60 @@ -54,6 +56,49 @@ async def test_context(pool_cls, dsn): assert p.closed +async def test_create_warning(pool_cls, dsn): + # No warning on explicit open + p = pool_cls(dsn, open=True) + try: + async with p.connection(): + pass + finally: + await p.close() + + # No warning on explicit close + p = pool_cls(dsn, open=False) + await p.open() + try: + async with p.connection(): + pass + finally: + await p.close() + + # No warning on context manager + async with pool_cls(dsn) as p: + async with p.connection(): + pass + + # Warning on open not specified + with pytest.warns(DeprecationWarning): + p = pool_cls(dsn) + try: + async with p.connection(): + pass + finally: + await p.close() + + # Warning also if open is called explicitly on already implicitly open + with pytest.warns(DeprecationWarning): + p = pool_cls(dsn) + await p.open() + try: + async with p.connection(): + pass + finally: + await p.close() + + async def test_wait_closed(pool_cls, dsn): async with pool_cls(dsn) as p: pass @@ -322,7 +367,7 @@ async def test_del_stops_threads(pool_cls, dsn): async def test_closed_getconn(pool_cls, dsn): - p = pool_cls(dsn, min_size=min_size(pool_cls)) + p = pool_cls(dsn, min_size=min_size(pool_cls), open=True) assert not p.closed async with p.connection(): pass @@ -336,7 +381,7 @@ async def test_closed_getconn(pool_cls, dsn): async def test_close_connection_on_pool_close(pool_cls, dsn): - p = pool_cls(dsn, min_size=min_size(pool_cls)) + p = pool_cls(dsn, min_size=min_size(pool_cls), open=True) async with p.connection() as conn: await p.close() assert conn.closed @@ -361,7 +406,7 @@ async def test_closed_queue(pool_cls, dsn): e1 = AEvent() e2 = AEvent() - p = pool_cls(dsn, min_size=min_size(pool_cls), max_size=1) + p = pool_cls(dsn, min_size=min_size(pool_cls), max_size=1, open=True) await p.wait() success: List[str] = [] @@ -420,7 +465,7 @@ async def test_open_context(pool_cls, dsn): async def test_open_no_op(pool_cls, dsn): - p = pool_cls(dsn) + p = pool_cls(dsn, open=True) try: assert not p.closed await p.open() @@ -435,7 +480,7 @@ async def test_open_no_op(pool_cls, dsn): async def test_reopen(pool_cls, dsn): - p = pool_cls(dsn) + p = pool_cls(dsn, open=True) async with p.connection() as conn: await conn.execute("select 1") await p.close()