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 `<issue_id>.<type>` 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 <[email protected]>
(cherry picked from commit fecbe99)

<!-- Thank you for your contribution! -->

## What do these changes do?

<!-- Please give a short brief about these changes. -->

## Are there changes in behavior for the user?

<!-- Outline any notable behaviour for the end users. -->

## Related issue number

<!-- Are there any issues opened that will be resolved by merging this
change? -->

## 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 &lt;Name&gt; &lt;Surname&gt;.
  * Please keep alphabetical order, the file is sorted by names.
- [ ] Add a new news fragment into the `CHANGES` folder
  * name it `<issue_id>.<type>` 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 <[email protected]>

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`

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)

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
 

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()
 

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