From e49bfdbc2b4dbd8bc2d7e54cd3c2b1073cae3c76 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 11 Feb 2023 17:31:13 +0000 Subject: [PATCH] client: fix upload timeouts with sock_read set (#7150) (#7206) Prevent the `sock_read` timeout callback from firing by only scheduling it afterthe payload (if any) has been fully written. No Fixes #7149 - [x] I think the code is well written - [x] Unit tests for the changes exist - [x] Documentation reflects the changes - [x] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` * The format is <Name> <Surname>. * Please keep alphabetical order, the file is sorted by names. - [x] Add a new news fragment into the `CHANGES` folder * name it `.` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." --------- Co-authored-by: Sam Bull (cherry picked from commit fecbe999c7a110fbeba8aa6ba269497435b2870d) ## What do these changes do? ## Are there changes in behavior for the user? ## Related issue number ## Checklist - [ ] I think the code is well written - [ ] Unit tests for the changes exist - [ ] Documentation reflects the changes - [ ] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` * The format is <Name> <Surname>. * Please keep alphabetical order, the file is sorted by names. - [ ] Add a new news fragment into the `CHANGES` folder * name it `.` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." Co-authored-by: daniele <36171005+dtrifiro@users.noreply.github.com> --- CHANGES/7149.bugfix | 1 + aiohttp/client_proto.py | 4 +++- aiohttp/client_reqrep.py | 2 ++ tests/test_client_functional.py | 20 ++++++++++++++++++++ tests/test_client_proto.py | 5 +++++ 5 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 CHANGES/7149.bugfix diff --git a/CHANGES/7149.bugfix b/CHANGES/7149.bugfix new file mode 100644 index 00000000000..dc3ac798d7c --- /dev/null +++ b/CHANGES/7149.bugfix @@ -0,0 +1 @@ +changed ``sock_read`` timeout to start after writing has finished, to avoid read timeouts caused by an unfinished write. -- by :user:`dtrifiro` diff --git a/aiohttp/client_proto.py b/aiohttp/client_proto.py index 07d09e11922..31df934c33d 100644 --- a/aiohttp/client_proto.py +++ b/aiohttp/client_proto.py @@ -150,7 +150,6 @@ def set_response_params( self._skip_payload = skip_payload self._read_timeout = read_timeout - self._reschedule_timeout() self._timeout_ceil_threshold = timeout_ceil_threshold @@ -186,6 +185,9 @@ def _reschedule_timeout(self) -> None: else: self._read_timeout_handle = None + def start_timeout(self) -> None: + self._reschedule_timeout() + def _on_read_timeout(self) -> None: exc = ServerTimeoutError("Timeout on reading data from socket") self.set_exception(exc) diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index e31b5085f7f..e31a466a0a4 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -606,6 +606,8 @@ async def write_bytes( protocol.set_exception(exc) except Exception as exc: protocol.set_exception(exc) + else: + protocol.start_timeout() finally: self._writer = None diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index b9bf7d72bd7..f57b9be3ee6 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -8,6 +8,7 @@ import pathlib import socket import ssl +from typing import AsyncIterator from unittest import mock import pytest @@ -673,6 +674,25 @@ async def handler(request): await resp.content.read() +async def test_read_timeout_on_write(aiohttp_client) -> None: + async def gen_payload() -> AsyncIterator[str]: + # Delay writing to ensure read timeout isn't triggered before writing completes. + await asyncio.sleep(0.5) + yield b"foo" + + async def handler(request: web.Request) -> web.Response: + return web.Response(body=await request.read()) + + app = web.Application() + app.router.add_put("/", handler) + + timeout = aiohttp.ClientTimeout(total=None, sock_read=0.1) + client = await aiohttp_client(app) + async with client.put("/", data=gen_payload(), timeout=timeout) as resp: + result = await resp.read() # Should not trigger a read timeout. + assert result == b"foo" + + async def test_timeout_on_reading_data(aiohttp_client, mocker) -> None: loop = asyncio.get_event_loop() diff --git a/tests/test_client_proto.py b/tests/test_client_proto.py index eea2830246a..d8ffac0059c 100644 --- a/tests/test_client_proto.py +++ b/tests/test_client_proto.py @@ -107,12 +107,15 @@ async def test_empty_data(loop) -> None: async def test_schedule_timeout(loop) -> None: proto = ResponseHandler(loop=loop) proto.set_response_params(read_timeout=1) + assert proto._read_timeout_handle is None + proto.start_timeout() assert proto._read_timeout_handle is not None async def test_drop_timeout(loop) -> None: proto = ResponseHandler(loop=loop) proto.set_response_params(read_timeout=1) + proto.start_timeout() assert proto._read_timeout_handle is not None proto._drop_timeout() assert proto._read_timeout_handle is None @@ -121,6 +124,7 @@ async def test_drop_timeout(loop) -> None: async def test_reschedule_timeout(loop) -> None: proto = ResponseHandler(loop=loop) proto.set_response_params(read_timeout=1) + proto.start_timeout() assert proto._read_timeout_handle is not None h = proto._read_timeout_handle proto._reschedule_timeout() @@ -131,6 +135,7 @@ async def test_reschedule_timeout(loop) -> None: async def test_eof_received(loop) -> None: proto = ResponseHandler(loop=loop) proto.set_response_params(read_timeout=1) + proto.start_timeout() assert proto._read_timeout_handle is not None proto.eof_received() assert proto._read_timeout_handle is None