Adjust read_timeout Behavior for Bot.get_updates (#3963)

This commit is contained in:
Bibo-Joshi 2023-11-27 18:24:21 +01:00 committed by GitHub
parent 354a8e0854
commit da11561f87
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 334 additions and 40 deletions

View file

@ -20,9 +20,9 @@ import inspect
keyword_args = [
"Keyword Arguments:",
(
" read_timeout ({read_timeout_type}, optional): Value to pass to "
" read_timeout (:obj:`float` | :obj:`None`, optional): Value to pass to "
" :paramref:`telegram.request.BaseRequest.post.read_timeout`. Defaults to "
" {read_timeout}."
" :attr:`~telegram.request.BaseRequest.DEFAULT_NONE`. "
),
(
" write_timeout (:obj:`float` | :obj:`None`, optional): Value to pass to "
@ -73,11 +73,14 @@ media_write_timeout_deprecation = [
"",
"",
]
read_timeout_sub = [
":attr:`~telegram.request.BaseRequest.DEFAULT_NONE`",
"``2``. :paramref:`timeout` will be added to this value",
get_updates_read_timeout_addition = [
" :paramref:`timeout` will be added to this value.",
"",
"",
" .. versionchanged:: NEXT.VERSION",
" Defaults to :attr:`~telegram.request.BaseRequest.DEFAULT_NONE` instead of ",
" ``2``.",
]
read_timeout_type = [":obj:`float` | :obj:`None`", ":obj:`float`"]
def find_insert_pos_for_kwargs(lines: list[str]) -> int:

View file

@ -29,11 +29,10 @@ from docs.auxil.admonition_inserter import AdmonitionInserter
from docs.auxil.kwargs_insertion import (
check_timeout_and_api_kwargs_presence,
find_insert_pos_for_kwargs,
get_updates_read_timeout_addition,
keyword_args,
media_write_timeout_deprecation,
media_write_timeout_deprecation_methods,
read_timeout_sub,
read_timeout_type,
)
from docs.auxil.link_code import LINE_NUMBERS
@ -107,7 +106,7 @@ def autodoc_process_docstring(
f"Couldn't find the correct position to insert the keyword args for {obj}."
)
get_updates_sub = 1 if (method_name == "get_updates") else 0
get_updates: bool = method_name == "get_updates"
# The below can be done in 1 line with itertools.chain, but this must be modified in-place
insert_idx = insert_index
for i in range(insert_index, insert_index + len(keyword_args)):
@ -118,18 +117,11 @@ def autodoc_process_docstring(
and method_name in media_write_timeout_deprecation_methods
):
effective_insert: list[str] = media_write_timeout_deprecation
elif get_updates and to_insert.lstrip().startswith("read_timeout"):
effective_insert = [to_insert] + get_updates_read_timeout_addition
else:
effective_insert = [to_insert]
effective_insert = [
entry.format(
method=method_name,
read_timeout=read_timeout_sub[get_updates_sub],
read_timeout_type=read_timeout_type[get_updates_sub],
)
for entry in effective_insert
]
lines[insert_idx:insert_idx] = effective_insert
insert_idx += len(effective_insert)

View file

@ -101,7 +101,7 @@ from telegram.error import InvalidToken
from telegram.request import BaseRequest, RequestData
from telegram.request._httpxrequest import HTTPXRequest
from telegram.request._requestparameter import RequestParameter
from telegram.warnings import PTBUserWarning
from telegram.warnings import PTBDeprecationWarning, PTBUserWarning
if TYPE_CHECKING:
from telegram import (
@ -3496,7 +3496,7 @@ class Bot(TelegramObject, AsyncContextManager["Bot"]):
timeout: Optional[int] = None,
allowed_updates: Optional[Sequence[str]] = None,
*,
read_timeout: float = 2,
read_timeout: ODVInput[float] = DEFAULT_NONE,
write_timeout: ODVInput[float] = DEFAULT_NONE,
connect_timeout: ODVInput[float] = DEFAULT_NONE,
pool_timeout: ODVInput[float] = DEFAULT_NONE,
@ -3558,6 +3558,22 @@ class Bot(TelegramObject, AsyncContextManager["Bot"]):
"allowed_updates": allowed_updates,
}
# The "or 0" is needed for the case where read_timeout is None.
if not isinstance(read_timeout, DefaultValue):
arg_read_timeout: float = read_timeout or 0
else:
try:
arg_read_timeout = self._request[0].read_timeout or 0
except NotImplementedError:
arg_read_timeout = 2
self._warn(
f"The class {self._request[0].__class__.__name__} does not override "
"the property `read_timeout`. Overriding this property will be mandatory in "
"future versions. Using 2 seconds as fallback.",
PTBDeprecationWarning,
stacklevel=3,
)
# Ideally we'd use an aggressive read timeout for the polling. However,
# * Short polling should return within 2 seconds.
# * Long polling poses a different problem: the connection might have been dropped while
@ -3568,7 +3584,7 @@ class Bot(TelegramObject, AsyncContextManager["Bot"]):
await self._post(
"getUpdates",
data,
read_timeout=read_timeout + timeout if timeout else read_timeout,
read_timeout=arg_read_timeout + timeout if timeout else arg_read_timeout,
write_timeout=write_timeout,
connect_timeout=connect_timeout,
pool_timeout=pool_timeout,

View file

@ -700,7 +700,7 @@ class Application(Generic[BT, CCT, UD, CD, BD, JQ], AsyncContextManager["Applica
poll_interval: float = 0.0,
timeout: int = 10,
bootstrap_retries: int = -1,
read_timeout: float = 2,
read_timeout: ODVInput[float] = DEFAULT_NONE,
write_timeout: ODVInput[float] = DEFAULT_NONE,
connect_timeout: ODVInput[float] = DEFAULT_NONE,
pool_timeout: ODVInput[float] = DEFAULT_NONE,
@ -745,16 +745,37 @@ class Application(Generic[BT, CCT, UD, CD, BD, JQ], AsyncContextManager["Applica
* > 0 - retry up to X times
read_timeout (:obj:`float`, optional): Value to pass to
:paramref:`telegram.Bot.get_updates.read_timeout`. Defaults to ``2``.
:paramref:`telegram.Bot.get_updates.read_timeout`. Defaults to
:attr:`~telegram.request.BaseRequest.DEFAULT_NONE`.
.. versionchanged:: NEXT.VERSION
Defaults to :attr:`~telegram.request.BaseRequest.DEFAULT_NONE` instead of
``2``.
.. deprecated:: NEXT.VERSION
Deprecated in favor of setting the timeout via
:meth:`telegram.ext.ApplicationBuilder.get_updates_read_timeout`.
write_timeout (:obj:`float` | :obj:`None`, optional): Value to pass to
:paramref:`telegram.Bot.get_updates.write_timeout`. Defaults to
:attr:`~telegram.request.BaseRequest.DEFAULT_NONE`.
.. deprecated:: NEXT.VERSION
Deprecated in favor of setting the timeout via
:meth:`telegram.ext.ApplicationBuilder.get_updates_write_timeout`.
connect_timeout (:obj:`float` | :obj:`None`, optional): Value to pass to
:paramref:`telegram.Bot.get_updates.connect_timeout`. Defaults to
:attr:`~telegram.request.BaseRequest.DEFAULT_NONE`.
.. deprecated:: NEXT.VERSION
Deprecated in favor of setting the timeout via
:meth:`telegram.ext.ApplicationBuilder.get_updates_connect_timeout`.
pool_timeout (:obj:`float` | :obj:`None`, optional): Value to pass to
:paramref:`telegram.Bot.get_updates.pool_timeout`. Defaults to
:attr:`~telegram.request.BaseRequest.DEFAULT_NONE`.
.. deprecated:: NEXT.VERSION
Deprecated in favor of setting the timeout via
:meth:`telegram.ext.ApplicationBuilder.get_updates_pool_timeout`.
drop_pending_updates (:obj:`bool`, optional): Whether to clean any pending updates on
Telegram servers before actually starting to poll. Default is :obj:`False`.
allowed_updates (List[:obj:`str`], optional): Passed to
@ -783,6 +804,14 @@ class Application(Generic[BT, CCT, UD, CD, BD, JQ], AsyncContextManager["Applica
"Application.run_polling is only available if the application has an Updater."
)
if (read_timeout, write_timeout, connect_timeout, pool_timeout) != ((DEFAULT_NONE,) * 4):
warn(
"Setting timeouts via `Application.run_polling` is deprecated. "
"Please use `ApplicationBuilder.get_updates_*_timeout` instead.",
PTBDeprecationWarning,
stacklevel=2,
)
def error_callback(exc: TelegramError) -> None:
self.create_task(self.process_error(error=exc, update=None))

View file

@ -549,7 +549,7 @@ class ExtBot(Bot, Generic[RLARGS]):
timeout: Optional[int] = None,
allowed_updates: Optional[Sequence[str]] = None,
*,
read_timeout: float = 2,
read_timeout: ODVInput[float] = DEFAULT_NONE,
write_timeout: ODVInput[float] = DEFAULT_NONE,
connect_timeout: ODVInput[float] = DEFAULT_NONE,
pool_timeout: ODVInput[float] = DEFAULT_NONE,

View file

@ -211,7 +211,7 @@ class Updater(AsyncContextManager["Updater"]):
poll_interval: float = 0.0,
timeout: int = 10,
bootstrap_retries: int = -1,
read_timeout: float = 2,
read_timeout: ODVInput[float] = DEFAULT_NONE,
write_timeout: ODVInput[float] = DEFAULT_NONE,
connect_timeout: ODVInput[float] = DEFAULT_NONE,
pool_timeout: ODVInput[float] = DEFAULT_NONE,
@ -236,16 +236,40 @@ class Updater(AsyncContextManager["Updater"]):
* 0 - no retries
* > 0 - retry up to X times
read_timeout (:obj:`float`, optional): Value to pass to
:paramref:`telegram.Bot.get_updates.read_timeout`. Defaults to ``2``.
:paramref:`telegram.Bot.get_updates.read_timeout`. Defaults to
:attr:`~telegram.request.BaseRequest.DEFAULT_NONE`.
.. versionchanged:: NEXT.VERSION
Defaults to :attr:`~telegram.request.BaseRequest.DEFAULT_NONE` instead of
``2``.
.. deprecated:: NEXT.VERSION
Deprecated in favor of setting the timeout via
:meth:`telegram.ext.ApplicationBuilder.get_updates_read_timeout` or
:paramref:`telegram.Bot.get_updates_request`.
write_timeout (:obj:`float` | :obj:`None`, optional): Value to pass to
:paramref:`telegram.Bot.get_updates.write_timeout`. Defaults to
:attr:`~telegram.request.BaseRequest.DEFAULT_NONE`.
.. deprecated:: NEXT.VERSION
Deprecated in favor of setting the timeout via
:meth:`telegram.ext.ApplicationBuilder.get_updates_write_timeout` or
:paramref:`telegram.Bot.get_updates_request`.
connect_timeout (:obj:`float` | :obj:`None`, optional): Value to pass to
:paramref:`telegram.Bot.get_updates.connect_timeout`. Defaults to
:attr:`~telegram.request.BaseRequest.DEFAULT_NONE`.
.. deprecated:: NEXT.VERSION
Deprecated in favor of setting the timeout via
:meth:`telegram.ext.ApplicationBuilder.get_updates_connect_timeout` or
:paramref:`telegram.Bot.get_updates_request`.
pool_timeout (:obj:`float` | :obj:`None`, optional): Value to pass to
:paramref:`telegram.Bot.get_updates.pool_timeout`. Defaults to
:attr:`~telegram.request.BaseRequest.DEFAULT_NONE`.
.. deprecated:: NEXT.VERSION
Deprecated in favor of setting the timeout via
:meth:`telegram.ext.ApplicationBuilder.get_updates_pool_timeout` or
:paramref:`telegram.Bot.get_updates_request`.
allowed_updates (List[:obj:`str`], optional): Passed to
:meth:`telegram.Bot.get_updates`.
drop_pending_updates (:obj:`bool`, optional): Whether to clean any pending updates on
@ -271,6 +295,10 @@ class Updater(AsyncContextManager["Updater"]):
:exc:`RuntimeError`: If the updater is already running or was not initialized.
"""
# We refrain from issuing deprecation warnings for the timeout parameters here, as we
# already issue them in `Application`. This means that there are no warnings when using
# `Updater` without `Application`, but this is a rather special use case.
if error_callback and asyncio.iscoroutinefunction(error_callback):
raise TypeError(
"The `error_callback` must not be a coroutine function! Use an ordinary function "
@ -316,7 +344,7 @@ class Updater(AsyncContextManager["Updater"]):
self,
poll_interval: float,
timeout: int,
read_timeout: float,
read_timeout: ODVInput[float],
write_timeout: ODVInput[float],
connect_timeout: ODVInput[float],
pool_timeout: ODVInput[float],
@ -401,16 +429,25 @@ class Updater(AsyncContextManager["Updater"]):
_LOGGER.debug(
"Calling `get_updates` one more time to mark all fetched updates as read."
)
await self.bot.get_updates(
offset=self._last_update_id,
# We don't want to do long polling here!
timeout=0,
read_timeout=read_timeout,
connect_timeout=connect_timeout,
write_timeout=write_timeout,
pool_timeout=pool_timeout,
allowed_updates=allowed_updates,
)
try:
await self.bot.get_updates(
offset=self._last_update_id,
# We don't want to do long polling here!
timeout=0,
read_timeout=read_timeout,
connect_timeout=connect_timeout,
write_timeout=write_timeout,
pool_timeout=pool_timeout,
allowed_updates=allowed_updates,
)
except TelegramError as exc:
_LOGGER.error(
"Error while calling `get_updates` one more time to mark all fetched updates "
"as read: %s. Suppressing error to ensure graceful shutdown. When polling for "
"updates is restarted, updates may be fetched again. Please adjust timeouts "
"via `ApplicationBuilder` or the parameter `get_updates_request` of `Bot`.",
exc_info=exc,
)
self.__polling_cleanup_cb = _get_updates_cleanup

View file

@ -130,6 +130,24 @@ class BaseRequest(
# https://docs.python.org/3/reference/datamodel.html?#object.__aexit__
await self.shutdown()
@property
def read_timeout(self) -> Optional[float]:
"""This property must return the default read timeout in seconds used by this class.
More precisely, the returned value should be the one used when
:paramref:`post.read_timeout` of :meth:post` is not passed/equal to :attr:`DEFAULT_NONE`.
.. versionadded:: NEXT.VERSION
Warning:
For now this property does not need to be implemented by subclasses and will raise
:exc:`NotImplementedError` if accessed without being overridden. However, in future
versions, this property will be abstract and must be implemented by subclasses.
Returns:
:obj:`float` | :obj:`None`: The read timeout in seconds.
"""
raise NotImplementedError
@abc.abstractmethod
async def initialize(self) -> None:
"""Initialize resources used by this class. Must be implemented by a subclass."""

View file

@ -198,6 +198,16 @@ class HTTPXRequest(BaseRequest):
"""
return self._http_version
@property
def read_timeout(self) -> Optional[float]:
"""See :attr:`BaseRequest.read_timeout`.
Returns:
:obj:`float` | :obj:`None`: The default read timeout in seconds as passed to
:paramref:`HTTPXRequest.read_timeout`.
"""
return self._client.timeout.read
def _build_client(self) -> httpx.AsyncClient:
return httpx.AsyncClient(**self._client_kwargs) # type: ignore[arg-type]

View file

@ -1498,6 +1498,54 @@ class TestApplication:
found_log = True
assert found_log
@pytest.mark.parametrize(
"timeout_name",
["read_timeout", "connect_timeout", "write_timeout", "pool_timeout", "poll_interval"],
)
@pytest.mark.skipif(
platform.system() == "Windows",
reason="Can't send signals without stopping whole process on windows",
)
def test_run_polling_timeout_deprecation_warnings(
self, timeout_name, monkeypatch, recwarn, app
):
async def get_updates(*args, **kwargs):
# This makes sure that other coroutines have a chance of running as well
await asyncio.sleep(0)
return []
def thread_target():
waited = 0
while not app.running:
time.sleep(0.05)
waited += 0.05
if waited > 5:
pytest.fail("App apparently won't start")
time.sleep(0.05)
os.kill(os.getpid(), signal.SIGINT)
monkeypatch.setattr(app.bot, "get_updates", get_updates)
thread = Thread(target=thread_target)
thread.start()
kwargs = {timeout_name: 42}
app.run_polling(drop_pending_updates=True, close_loop=False, **kwargs)
thread.join()
if timeout_name == "poll_interval":
assert len(recwarn) == 0
return
assert len(recwarn) == 1
assert "Setting timeouts via `Application.run_polling` is deprecated." in str(
recwarn[0].message
)
assert recwarn[0].category is PTBDeprecationWarning
assert recwarn[0].filename == __file__, "wrong stacklevel"
@pytest.mark.skipif(
platform.system() == "Windows",
reason="Can't send signals without stopping whole process on windows",
@ -2210,7 +2258,8 @@ class TestApplication:
else:
app.run_webhook(close_loop=False, stop_signals=None)
assert len(recwarn) == 0
for record in recwarn:
assert not str(record.message).startswith("Could not add signal handlers for the stop")
@pytest.mark.flaky(3, 1) # loop.call_later will error the test when a flood error is received
def test_signal_handlers(self, app, monkeypatch):

View file

@ -19,11 +19,13 @@
import asyncio
import inspect
from dataclasses import dataclass
from http import HTTPStatus
import httpx
import pytest
from telegram import Bot
from telegram._utils.defaultvalue import DEFAULT_NONE
from telegram.ext import (
AIORateLimiter,
Application,
@ -581,3 +583,32 @@ class TestApplicationBuilder:
)
assert recwarn[0].category is PTBDeprecationWarning
assert recwarn[0].filename == __file__, "wrong stacklevel"
@pytest.mark.parametrize(
("read_timeout", "timeout", "expected"),
[
(None, None, 0),
(1, None, 1),
(None, 1, 1),
(DEFAULT_NONE, None, 10),
(DEFAULT_NONE, 1, 11),
(1, 2, 3),
],
)
async def test_get_updates_read_timeout_value_passing(
self, bot, read_timeout, timeout, expected, monkeypatch, builder
):
# This test is a double check that ApplicationBuilder respects the changes of #3963 just
# like `Bot` does - see also the corresponding test in test_bot.py (same name)
caught_read_timeout = None
async def catch_timeouts(*args, **kwargs):
nonlocal caught_read_timeout
caught_read_timeout = kwargs.get("read_timeout")
return HTTPStatus.OK, b'{"ok": "True", "result": {}}'
monkeypatch.setattr(HTTPXRequest, "do_request", catch_timeouts)
bot = builder.get_updates_read_timeout(10).token(bot.token).build().bot
await bot.get_updates(read_timeout=read_timeout, timeout=timeout)
assert caught_read_timeout == expected

View file

@ -331,6 +331,39 @@ class TestUpdater:
assert log_found
async def test_polling_mark_updates_as_read_timeout(self, monkeypatch, updater, caplog):
timeout_event = asyncio.Event()
async def get_updates(*args, **kwargs):
await asyncio.sleep(0)
if timeout_event.is_set():
raise TimedOut("TestMessage")
return []
monkeypatch.setattr(updater.bot, "get_updates", get_updates)
async with updater:
await updater.start_polling()
with caplog.at_level(logging.ERROR):
timeout_event.set()
await updater.stop()
assert len(caplog.records) >= 1
log_found = False
for record in caplog.records:
if not record.getMessage().startswith(
"Error while calling `get_updates` one more time"
):
continue
assert record.name == "telegram.ext.Updater"
assert record.exc_info[0] is TimedOut
assert str(record.exc_info[1]) == "TestMessage"
log_found = True
break
assert log_found
async def test_polling_mark_updates_as_read_failure(self, monkeypatch, updater, caplog):
async def get_updates(*args, **kwargs):
await asyncio.sleep(0)
@ -376,7 +409,7 @@ class TestUpdater:
expected = {
"timeout": 10,
"read_timeout": 2,
"read_timeout": DEFAULT_NONE,
"write_timeout": DEFAULT_NONE,
"connect_timeout": DEFAULT_NONE,
"pool_timeout": DEFAULT_NONE,

View file

@ -353,6 +353,20 @@ class TestRequestWithoutRequest:
)
assert self.test_flag == (1, 2, 3, 4)
def test_read_timeout_not_implemented(self):
class SimpleRequest(BaseRequest):
async def do_request(self, *args, **kwargs):
raise httpx.ReadTimeout("read timeout")
async def initialize(self) -> None:
pass
async def shutdown(self) -> None:
pass
with pytest.raises(NotImplementedError):
SimpleRequest().read_timeout
@pytest.mark.parametrize("media", [True, False])
async def test_timeout_propagation_write_timeout(
self, monkeypatch, media, input_media_photo, recwarn # noqa: F811
@ -739,6 +753,10 @@ class TestHTTPXRequestWithoutRequest:
HTTPXRequest(socket_options=((1, 2, 3),))
assert transport_kwargs["socket_options"] == ((1, 2, 3),)
@pytest.mark.parametrize("read_timeout", [None, 1, 2, 3])
async def test_read_timeout_property(self, read_timeout):
assert HTTPXRequest(read_timeout=read_timeout).read_timeout == read_timeout
@pytest.mark.skipif(not TEST_WITH_OPT_DEPS, reason="No need to run this twice")
class TestHTTPXRequestWithRequest:

View file

@ -26,6 +26,7 @@ import re
import socket
import time
from collections import defaultdict
from http import HTTPStatus
import httpx
import pytest
@ -79,7 +80,7 @@ from telegram.error import BadRequest, InvalidToken, NetworkError
from telegram.ext import ExtBot, InvalidCallbackData
from telegram.helpers import escape_markdown
from telegram.request import BaseRequest, HTTPXRequest, RequestData
from telegram.warnings import PTBUserWarning
from telegram.warnings import PTBDeprecationWarning, PTBUserWarning
from tests.auxil.bot_method_checks import check_defaults_handling
from tests.auxil.ci_bots import FALLBACKS
from tests.auxil.envvars import GITHUB_ACTION, TEST_WITH_OPT_DEPS
@ -2447,6 +2448,63 @@ class TestBotWithRequest:
if updates:
assert isinstance(updates[0], Update)
@pytest.mark.parametrize("bot_class", [Bot, ExtBot])
async def test_get_updates_read_timeout_deprecation_warning(
self, bot, recwarn, monkeypatch, bot_class
):
# Using the normal HTTPXRequest should not issue any warnings
await bot.get_updates()
assert len(recwarn) == 0
# Now let's test deprecation warning when using get_updates for other BaseRequest
# subclasses (we just monkeypatch the existing HTTPXRequest for this)
read_timeout = None
async def catch_timeouts(*args, **kwargs):
nonlocal read_timeout
read_timeout = kwargs.get("read_timeout")
return HTTPStatus.OK, b'{"ok": "True", "result": {}}'
monkeypatch.setattr(HTTPXRequest, "read_timeout", BaseRequest.read_timeout)
monkeypatch.setattr(HTTPXRequest, "do_request", catch_timeouts)
bot = bot_class(get_updates_request=HTTPXRequest(), token=bot.token)
await bot.get_updates()
assert len(recwarn) == 1
assert "does not override the property `read_timeout`" in str(recwarn[0].message)
assert recwarn[0].category is PTBDeprecationWarning
assert recwarn[0].filename == __file__, "wrong stacklevel"
assert read_timeout == 2
@pytest.mark.parametrize(
("read_timeout", "timeout", "expected"),
[
(None, None, 0),
(1, None, 1),
(None, 1, 1),
(DEFAULT_NONE, None, 10),
(DEFAULT_NONE, 1, 11),
(1, 2, 3),
],
)
async def test_get_updates_read_timeout_value_passing(
self, bot, read_timeout, timeout, expected, monkeypatch
):
caught_read_timeout = None
async def catch_timeouts(*args, **kwargs):
nonlocal caught_read_timeout
caught_read_timeout = kwargs.get("read_timeout")
return HTTPStatus.OK, b'{"ok": "True", "result": {}}'
monkeypatch.setattr(HTTPXRequest, "do_request", catch_timeouts)
bot = Bot(get_updates_request=HTTPXRequest(read_timeout=10), token=bot.token)
await bot.get_updates(read_timeout=read_timeout, timeout=timeout)
assert caught_read_timeout == expected
@pytest.mark.xdist_group("getUpdates_and_webhook")
@pytest.mark.parametrize("use_ip", [True, False])
# local file path as file_input is tested below in test_set_webhook_params