Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust read_timeout behavior for get_updates #3963

Merged
merged 15 commits into from Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 9 additions & 6 deletions docs/auxil/kwargs_insertion.py
Expand Up @@ -20,9 +20,9 @@
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 "
Expand Down Expand Up @@ -73,11 +73,14 @@
"",
"",
]
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:
Expand Down
16 changes: 4 additions & 12 deletions docs/auxil/sphinx_hooks.py
Expand Up @@ -29,11 +29,10 @@
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

Expand Down Expand Up @@ -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)):
Expand All @@ -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)

Expand Down
22 changes: 19 additions & 3 deletions telegram/_bot.py
Expand Up @@ -101,7 +101,7 @@
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 (
Expand Down Expand Up @@ -3496,7 +3496,7 @@ async def get_updates(
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,
Expand Down Expand Up @@ -3558,6 +3558,22 @@ async def get_updates(
"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
harshil21 marked this conversation as resolved.
Show resolved Hide resolved
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
Expand All @@ -3568,7 +3584,7 @@ async def get_updates(
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,
Expand Down
33 changes: 31 additions & 2 deletions telegram/ext/_application.py
Expand Up @@ -700,7 +700,7 @@ def run_polling(
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,
Expand Down Expand Up @@ -745,16 +745,37 @@ def run_polling(
* > 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
Expand Down Expand Up @@ -783,6 +804,14 @@ def run_polling(
"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))

Expand Down
2 changes: 1 addition & 1 deletion telegram/ext/_extbot.py
Expand Up @@ -549,7 +549,7 @@ async def get_updates(
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,
Expand Down
63 changes: 50 additions & 13 deletions telegram/ext/_updater.py
Expand Up @@ -211,7 +211,7 @@ async def start_polling(
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,
Expand All @@ -236,16 +236,40 @@ async def start_polling(
* 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
Expand All @@ -271,6 +295,10 @@ def callback(error: telegram.error.TelegramError)
: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 "
Expand Down Expand Up @@ -316,7 +344,7 @@ async def _start_polling(
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],
Expand Down Expand Up @@ -401,16 +429,25 @@ async def _get_updates_cleanup() -> None:
_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

Expand Down
18 changes: 18 additions & 0 deletions telegram/request/_baserequest.py
Expand Up @@ -130,6 +130,24 @@ async def __aexit__(
# 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."""
Expand Down
10 changes: 10 additions & 0 deletions telegram/request/_httpxrequest.py
Expand Up @@ -198,6 +198,16 @@ def http_version(self) -> str:
"""
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
harshil21 marked this conversation as resolved.
Show resolved Hide resolved

def _build_client(self) -> httpx.AsyncClient:
return httpx.AsyncClient(**self._client_kwargs) # type: ignore[arg-type]

Expand Down