Applying review comments

Author: Poolitzer

ptbcontrib/aiohttp_request/aiohttprequest.py

@@ -18,16 +18,17 @@
 # along with this program.  If not, see [http://www.gnu.org/licenses/].
 """This module contains methods to make POST and GET requests using the aiohttp library."""
 import asyncio
+import logging
 from typing import Any, Optional, Union
 
 import aiohttp
 import yarl
-from telegram._utils.logging import get_logger
+from telegram._utils.defaultvalue import DefaultValue
 from telegram._utils.types import ODVInput
 from telegram.error import NetworkError, TimedOut
 from telegram.request import BaseRequest, RequestData
 
-_LOGGER = get_logger(__name__, "AiohttpRequest")
+_LOGGER = logging.getLogger("AiohttpRequest")
 
 
 class AiohttpRequest(BaseRequest):
@@ -43,13 +44,13 @@ class AiohttpRequest(BaseRequest):
             Note:
                 :paramref:`media_total_timeout` will still be applied if a file is send, so be sure
                 to also set it to an appropriate value.
-                PylintDoesntAllowMeToWriteTODO Should I warn about this?
         media_total_timeout (:obj:`float` | :obj:`None`, optional): This overrides the total
             timeout with requests that upload media/files. Defaults to ``20`` seconds.
         proxy (:obj:`str` | `yarl.URL``, optional): The URL to a proxy server, aiohttp supports
             plain HTTP proxies and HTTP proxies that can be upgraded to HTTPS via the HTTP
             CONNECT method. See the docs of aiohttp: https://docs.aiohttp.org/en/stable/
             client_advanced.html#aiohttp-client-proxy-support.
+        proxy_auth (``aiohttp.BasicAuth``, optional): Proxy authorization, see :paramref:`proxy`.
         trust_env (:obj:`bool`, optional): In order to read proxy environmental variables, see the
             docs of aiohttp: https://docs.aiohttp.org/en/stable/client_advanced.html
             #aiohttp-client-proxy-support.
@@ -61,25 +62,28 @@ class AiohttpRequest(BaseRequest):
                 This parameter is intended for advanced users that want to fine-tune the behavior
                 of the underlying ``aiohttp`` clientSession. The values passed here will override
                 all the defaults set by ``python-telegram-bot`` and all other parameters passed to
-                :class:`ClientSession`. The only exception is the :paramref:`media_write_timeout`
+                :class:`ClientSession`. The only exception is the :paramref:`media_total_timeout`
                 parameter, which is not passed to the client constructor.
                 No runtime warnings will be issued about parameters that are overridden in this
                 way.
 
     """
 
-    __slots__ = ("_session", "_session_kwargs", "_media_total_timeout")
+    __slots__ = ("_session", "_session_kwargs", "_media_total_timeout", "_connection_pool_size")
 
     def __init__(  # pylint: disable=too-many-arguments,too-many-positional-arguments
         self,
         connection_pool_size: int = 1,
         client_timeout: Optional[aiohttp.ClientTimeout] = None,
-        media_total_timeout: Optional[float] = 20.0,
+        media_total_timeout: Optional[float] = 30.0,
         proxy: Optional[Union[str, yarl.URL]] = None,
+        proxy_auth: Optional[aiohttp.BasicAuth] = None,
         trust_env: Optional[bool] = None,
         aiohttp_kwargs: Optional[dict[str, Any]] = None,
     ):
         self._media_total_timeout = media_total_timeout
+        # this needs to be saved in case of initialize gets a closed session
+        self._connection_pool_size = connection_pool_size
         timeout = (
             client_timeout
             if client_timeout
@@ -103,6 +107,7 @@ def __init__(  # pylint: disable=too-many-arguments,too-many-positional-argument
             "timeout": timeout,
             "connector": conn,
             "proxy": proxy,
+            "proxy_auth": proxy_auth,
             "trust_env": trust_env,
             **(aiohttp_kwargs or {}),
         }
@@ -113,8 +118,8 @@ def __init__(  # pylint: disable=too-many-arguments,too-many-positional-argument
     def read_timeout(self) -> Optional[float]:
         """See :attr:`BaseRequest.read_timeout`.
 
-        This makes not a lot of sense to implement since there is no actual read_timeout.
-        But what can I do.
+        aiohttp does not have a read timeout. Instead the total timeout for a request (including
+        connection establishment, request sending and response reading) is returned.
         """
         return self._session.timeout.total
 
@@ -124,6 +129,14 @@ def _build_client(self) -> aiohttp.ClientSession:
     async def initialize(self) -> None:
         """See :meth:`BaseRequest.initialize`."""
         if self._session.closed:
+            # this means the TCPConnector has been closed, so we need to recreate it
+            try:
+                loop = asyncio.get_running_loop()
+            except RuntimeError:
+                loop = asyncio.get_event_loop()
+
+            conn = aiohttp.TCPConnector(limit=self._connection_pool_size, loop=loop)
+            self._session_kwargs["connector"] = conn
             self._session = self._build_client()
 
     async def shutdown(self) -> None:
@@ -134,7 +147,6 @@ async def shutdown(self) -> None:
 
         await self._session.close()
 
-    # pylint: disable=unused-argument
     async def do_request(  # pylint: disable=too-many-arguments,too-many-positional-arguments
         self,
         url: str,
@@ -145,7 +157,18 @@ async def do_request(  # pylint: disable=too-many-arguments,too-many-positional-
         connect_timeout: ODVInput[float] = BaseRequest.DEFAULT_NONE,
         pool_timeout: ODVInput[float] = BaseRequest.DEFAULT_NONE,
     ) -> tuple[int, bytes]:
-        """See :meth:`BaseRequest.do_request`."""
+        """See :meth:`BaseRequest.do_request`.
+
+        Since aiohttp has differen't timeouts, the params were mapped.
+
+        * :paramref:`pool_timeout` is mapped to :attr`~aiohttp.ClientTimeout.connect`
+        * :paramref:`connect_timeout` is mapped to :attr`~aiohttp.ClientTimeout.sock_connect`
+        * :paramref:`read_timeout` is mapped to :attr`~aiohttp.ClientTimeout.sock_read`
+        * :paramref:`write_timeout` is mapped to :attr`~aiohttp.ClientTimeout.ceil_threshold`
+
+        The :attr`~aiohttp.ClientTimeout.total` timeout is not changed since it also includes
+        response reading. You can only change them when initializing the class.
+        """
         if self._session.closed:
             raise RuntimeError("This AiohttpRequest is not initialized!")
 
@@ -161,20 +184,27 @@ async def do_request(  # pylint: disable=too-many-arguments,too-many-positional-
                     filename=request_data.multipart_data[field_name][0],
                 )
 
-        # I dont think it makes sense to support the timeout params.
-        # PylintDoesntAllowMeToWriteTOLiDO if no one complains in initial PR
-        # raise warnings if they are passed
-
-        timeout = (
-            aiohttp.ClientTimeout(
-                total=self._media_total_timeout,
-                connect=self._session_kwargs["timeout"].connect,
-                sock_read=self._session_kwargs["timeout"].sock_read,
-                sock_connect=self._session_kwargs["timeout"].sock_connect,
-                ceil_threshold=self._session_kwargs["timeout"].ceil_threshold,
-            )
-            if request_data and request_data.contains_files
-            else self._session_kwargs["timeout"]
+        # If user did not specify timeouts (for e.g. in a bot method), use the default ones when we
+        # created this instance.
+        if isinstance(read_timeout, DefaultValue):
+            read_timeout = self._session_kwargs["timeout"].sock_read
+        if isinstance(connect_timeout, DefaultValue):
+            connect_timeout = self._session_kwargs["timeout"].sock_connect
+        if isinstance(pool_timeout, DefaultValue):
+            pool_timeout = self._session_kwargs["timeout"].connect
+        if isinstance(write_timeout, DefaultValue):
+            write_timeout = self._session_kwargs["timeout"].ceil_threshold
+
+        timeout = aiohttp.ClientTimeout(
+            total=(
+                self._media_total_timeout
+                if (request_data and request_data.contains_files)
+                else self._session_kwargs["timeout"].total
+            ),
+            connect=pool_timeout,
+            sock_read=read_timeout,
+            sock_connect=connect_timeout,
+            ceil_threshold=write_timeout,
         )
 
         try:

ptbcontrib/aiohttp_request/requirements.txt

@@ -1,2 +1,2 @@
-python-telegram-bot>=21.1
+python-telegram-bot>=22.1
 aiohttp[speedups]>=3.11

tests/test_aiohttp_request.py

@@ -112,35 +112,28 @@ class TestRequest:
     def _reset(self):
         self.test_flag = None
 
-    # def test_aiohttp_kwargs(self, monkeypatch):
-    #     self.test_flag = {}
-    #
-    #     orig_init = aiohttp.ClientSession.__init__
-    #
-    #     class Session(aiohttp.ClientSession):
-    #         def __init__(*args, **kwargs):
-    #             orig_init(*args, **kwargs)
-    #             self.test_flag["args"] = args
-    #             self.test_flag["kwargs"] = kwargs
-    #
-    #     monkeypatch.setattr(aiohttp, "ClientSession", Session)
-    #
-    #     AiohttpRequest(
-    #         client_timeout=aiohttp.ClientTimeout(total=1.0),
-    #         connection_pool_size=42,
-    #         httpx_kwargs={
-    #             "timeout": httpx.Timeout(7),
-    #             "limits": httpx.Limits(max_connections=7),
-    #             "http1": True,
-    #             "verify": False,
-    #         },
-    #     )
-    #     kwargs = self.test_flag["kwargs"]
-    #
-    #     assert kwargs["timeout"].connect == 7
-    #     assert kwargs["limits"].max_connections == 7
-    #     assert kwargs["http1"] is True
-    #     assert kwargs["verify"] is False
+    def test_aiohttp_kwargs(self, monkeypatch):
+        self.test_flag = {}
+
+        orig_init = aiohttp.ClientSession.__init__
+
+        class Session(aiohttp.ClientSession):
+            def __init__(*args, **kwargs):
+                orig_init(*args, **kwargs)
+                self.test_flag["args"] = args
+                self.test_flag["kwargs"] = kwargs
+
+        monkeypatch.setattr(aiohttp, "ClientSession", Session)
+
+        AiohttpRequest(
+            client_timeout=aiohttp.ClientTimeout(total=1.0),
+            aiohttp_kwargs={
+                "timeout": aiohttp.ClientTimeout(total=40.0),
+            },
+        )
+        kwargs = self.test_flag["kwargs"]
+
+        assert kwargs["timeout"].total == 40
 
     async def test_context_manager(self, monkeypatch):
         async def initialize():
@@ -386,41 +379,26 @@ class TestAiohttpRequest:
     def _reset(self):
         self.test_flag = None
 
-    # def test_init(self, monkeypatch):
-    #     @dataclass
-    #     class Session:
-    #         timeout: object
-    #         proxy: object
-    #         limits: object
-    #         http1: object
-    #         http2: object
-    #         transport: object = None
-    #
-    #     monkeypatch.setattr(aiohttp, "ClientSession", Session)
-    #
-    #     request = AiohttpRequest()
-    #     assert request._client.timeout ==
-    #     httpx.Timeout(connect=5.0, read=5.0, write=5.0, pool=1.0)
-    #     assert request._client.proxy is None
-    #     assert request._client.limits == httpx.Limits(
-    #         max_connections=1, max_keepalive_connections=1
-    #     )
-    #     assert request._client.http1 is True
-    #     assert not request._client.http2
-    #
-    #     request = AiohttpRequest(
-    #         connection_pool_size=42,
-    #         proxy="proxy",
-    #         connect_timeout=43,
-    #         read_timeout=44,
-    #         write_timeout=45,
-    #         pool_timeout=46,
-    #     )
-    #     assert request._client.proxy == "proxy"
-    #     assert request._client.limits == httpx.Limits(
-    #         max_connections=42, max_keepalive_connections=42
-    #     )
-    #     assert request._client.timeout == httpx.Timeout(connect=43, read=44, write=45, pool=46)
+    def test_init(self, monkeypatch):
+
+        request = AiohttpRequest()
+        assert request._session.timeout == aiohttp.ClientTimeout(total=15)
+        assert request._session._default_proxy is None
+        assert request._session._version is aiohttp.HttpVersion11
+
+        request = AiohttpRequest(
+            connection_pool_size=42,
+            client_timeout=aiohttp.ClientTimeout(total=25),
+            media_total_timeout=200,
+            proxy="proxy",
+            proxy_auth=aiohttp.BasicAuth("user", "pass"),
+            trust_env=True,
+        )
+        assert request._session._default_proxy == "proxy"
+        assert request._session._default_proxy_auth == aiohttp.BasicAuth("user", "pass")
+        assert request._session.timeout == aiohttp.ClientTimeout(total=25)
+        assert request._media_total_timeout == 200
+        assert request._session._trust_env
 
     async def test_multiple_inits_and_shutdowns(self, monkeypatch):
         self.test_flag = defaultdict(int)
@@ -507,31 +485,31 @@ async def make_assertion(_, **kwargs):
 
         assert self.test_flag
 
-    # async def test_do_request_manual_timeouts(self, monkeypatch, aiohttp_request):
-    #     default_timeouts = aiohttp.ClientTimeout(total=42, connect=43,
-    #     sock_read=44, sock_connect=45, ceil_threshold=46)
-    #     manual_timeouts = aiohttp.ClientTimeout(total=52, connect=53,
-    #     sock_read=54, sock_connect=55, ceil_threshold=56)
-    #
-    #     async def make_assertion(_, **kwargs):
-    #         print(kwargs.get("timeout"))
-    #         self.test_flag = kwargs.get("timeout") == manual_timeouts
-    #         return Response()
-    #
-    #     async with AiohttpRequest(
-    #             client_timeout=default_timeouts
-    #     ) as aiohttp_request_ctx:
-    #         monkeypatch.setattr(aiohttp.ClientSession, "request", make_assertion)
-    #         await aiohttp_request_ctx.do_request(
-    #             method="GET",
-    #             url="URL",
-    #             connect_timeout=manual_timeouts.connect,
-    #             read_timeout=manual_timeouts.total,
-    #             write_timeout=manual_timeouts.sock_read,
-    #             pool_timeout=manual_timeouts.sock_connect,
-    #         )
-    #
-    #     assert self.test_flag
+    async def test_do_request_manual_timeouts(self, monkeypatch, aiohttp_request):
+        default_timeouts = aiohttp.ClientTimeout(
+            total=42, connect=43, sock_read=44, sock_connect=45, ceil_threshold=46
+        )
+        manual_timeouts = aiohttp.ClientTimeout(
+            total=42, connect=53, sock_read=54, sock_connect=55, ceil_threshold=56
+        )
+
+        async def make_assertion(_, **kwargs):
+            print(kwargs.get("timeout"))
+            self.test_flag = kwargs.get("timeout") == manual_timeouts
+            return Response()
+
+        async with AiohttpRequest(client_timeout=default_timeouts) as aiohttp_request_ctx:
+            monkeypatch.setattr(aiohttp.ClientSession, "request", make_assertion)
+            await aiohttp_request_ctx.do_request(
+                method="GET",
+                url="URL",
+                connect_timeout=manual_timeouts.sock_connect,
+                read_timeout=manual_timeouts.sock_read,
+                write_timeout=manual_timeouts.ceil_threshold,
+                pool_timeout=manual_timeouts.connect,
+            )
+
+        assert self.test_flag
 
     async def test_do_request_params_no_data(self, monkeypatch, aiohttp_request):
         async def make_assertion(self, **kwargs):