Peer cancellation policy documentation unclear

[Tinche]

Describe the bug

The docs (https://docs.aiohttp.org/en/stable/web_advanced.html#peer-disconnection) clearly state:

When a client peer is gone a subsequent reading or writing raises OSError or more specific exception like ConnectionResetError.

As far as I understand, this behavior replaces the old behavior, which was to cancel the task running the handler as soon as a peer disconnected.

But if you curl what I've pasted in the To Reproduce section and kill curl after 1 second, the request handler will actually be cancelled, which is the old behavior. So which is correct?

To Reproduce

from asyncio import sleep

from aiohttp import web


async def handle(request):
    await sleep(3)
    name = request.match_info.get("name", "Anonymous")
    text = "Hello, " + name
    print(text)
    return web.Response(text=text)


app = web.Application()
app.add_routes([web.get("/", handle), web.get("/{name}", handle)])

if __name__ == "__main__":
    web.run_app(app)

Expected behavior

Not sure.

Logs/tracebacks

None.

Python Version

$ python --version
Python 3.9.2

aiohttp Version

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.7.4.post0
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: Nikolay Kim
Author-email: fafhrd91@gmail.com
License: Apache 2
Location: /home/tin/hr/feed-service/.venv/lib/python3.9/site-packages
Requires: async-timeout, attrs, chardet, typing-extensions, multidict, yarl
Required-by: aiojobs, aioelasticsearch, aiobotocore

multidict Version

$ python -m pip show multidict
Name: multidict
Version: 5.1.0
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /home/tin/hr/feed-service/.venv/lib/python3.9/site-packages
Requires: 
Required-by: yarl, aiohttp

yarl Version

$ python -m pip show yarl
Name: yarl
Version: 1.6.3
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl/
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /home/tin/hr/feed-service/.venv/lib/python3.9/site-packages
Requires: idna, multidict
Required-by: feed-service, aiohttp

OS

Ubuntu Linux

Related component

Server

Additional context

No response

Code of Conduct

• I agree to follow the aio-libs Code of Conduct

[Hanaasagi]

asyncio.exceptions.CancelledError was raised from here:

aiohttp/aiohttp/web_protocol.py

Lines 524 to 528 in 88d1f80

	try:
	resp, reset = await task
	except (asyncio.CancelledError, ConnectionError):
	self.log_debug("Ignored premature client disconnection")
	break

But I think the reason is the handle will sleep 3 seconds, and client(curl) is be canceled after 1 second. sleep will create a internal futuer in asyncio, and set the result after N seconds by the loop.call_later method. At that time(stop the curl), it actually canceled this future.

[Dreamsorcerer]

Indeed, the example provided never tries to read anything, it is cancelled in the sleep.
This is the expected behaviour.

[Hanaasagi]

You can try following code:

import itertools

from aiohttp import web


async def handle(request):
    response = web.StreamResponse(
        status=200,
        reason='OK',
        headers={'Content-Type': 'text/plain'},
    )
    await response.prepare(request)

    for line in itertools.cycle('abc'):
        await response.write(line.encode('utf-8'))

    await response.write_eof()
    return response


app = web.Application()
app.add_routes([web.get("/", handle), web.get("/{name}", handle)])

if __name__ == "__main__":
    web.run_app(app)

It's a dead loop, send the abc to the client. It will be easy to trigger a IO operation.

Cancel the curl will get a ConnectionResetError exception.

======== Running on http://0.0.0.0:8080 ========
(Press CTRL+C to quit)
Error handling request
Traceback (most recent call last):
  File "/root/aiohttp/aiohttp/web_protocol.py", line 422, in _handle_request
    resp = await self._request_handler(request)
  File "/root/aiohttp/aiohttp/web_app.py", line 499, in _handle
    resp = await handler(request)
  File "/root/t/t.py", line 15, in handle
    await response.write(line.encode('utf-8'))
  File "/root/aiohttp/aiohttp/web_response.py", line 470, in write
    await self._payload_writer.write(data)
  File "/root/aiohttp/aiohttp/http_writer.py", line 107, in write
    self._write(chunk)
  File "/root/aiohttp/aiohttp/http_writer.py", line 67, in _write
    raise ConnectionResetError("Cannot write to closing transport")
ConnectionResetError: Cannot write to closing transport

[Dreamsorcerer]

Also worth noting that the OSError mentioned in the documentation would never be raised in the original example, because it does not read from the response at all, it only uses information from request.match_info which has already been read (otherwise you'd need an await to get it).

[Tinche]

Hm, ok. This is the commit that changed the docs: b74306d#diff-af5cbca68a75093b494497c857162462f30fc2f981c4b1104c57bd6a99d8db6c

Notice the new documentation doesn't say anything about cancellation. What's very likely to happen is that the peer will drop the connection when the handler is busy doing something else, like reading from a database or a different API. In that case the handler task will still be thrown a CancelledError, but the documentation stating this was explicitly removed for some reason.

[Dreamsorcerer]

Hmm, interesting. I think that documentation should probably be put back in. @webknjaz Thoughts?

[webknjaz]

I think v4.0 changes something wrt autocancelation. So compare master vs 3.8 branch.

[Dreamsorcerer]

Looks like the same change was made to 3.7:
b8ba3d0

Testing against master and 3.8, I get the same cancellation result. Looking at the tests in that commit, it seems that they all test that a ConnectionError occurs on reading, but none of the tests check what happens when awaiting on something else.

If the desired behaviour is that the code continues without cancelling, then we'll need to look at stopping this cancellation. I'm not sure where the cancellation is actually coming from though, as a traceback is useless in this scenario. The code that @Hanaasagi pointed to is catching a CancelledError, not throwing it. So, I don't think it's there.

[Tinche]

My opinion is that the cancellation was actually a good thing, and useful. However, when upgrading aiohttp for one of my work projects I noticed the changelog entry and assumed aiohttp simply stopped cancelling (due to the documentation changes and the changelog itself), and was surprised when I realized aiohttp is still cancelling.

Even if you refactor to remove cancelling tasks of disconnecting peers, please leave a switch to reenable the behavior because there's no way to get this functionality without aiohttp cooperation. On the other hand, if aiohttp continues cancelling and you want to ignore it, it's actually easy (for example, aiojobs.atomic).

[Hanaasagi]

I'm not sure where the cancellation is actually coming from though, as a traceback is useless in this scenario. The code that @Hanaasagi pointed to is catching a CancelledError, not throwing it. So, I don't think it's there.

It's coming from here(_task_handler). The connection is lost, all coroutine in this request chain will be canceled from from outside to inside.

aiohttp/aiohttp/web_protocol.py

Lines 322 to 327 in 10995e8

	if self._task_handler is not None:
	self._task_handler.cancel()
	if self._waiter is not None:
	self._waiter.cancel()
	self._task_handler = None

[Dreamsorcerer]

It's coming from here(_task_handler).

Thanks.

So, the question is what is the intended approach? The new documentation suggests we shouldn't be cancelling at all, although this seems less optimal. The old approach would be to cancel always. But, currently, it's a mix of OSError if you are awaiting on a read/write and cancelling if you awaiting on something else, which seems a little confusing to me.

Curiously, I can't actually get the OSError to appear in simple testing. If I use request.text() after disconnection, it seems to return the text immediately, it must have already loaded the body and can return it without raising an exception or allowing the cancel to happen (I tested with several KBs in the body).

e.g. If the handler in the example is changed to:

time.sleep(3)
print(await request.text())

This always prints the body for me, even if the client is already disconnected. If you add await asyncio.sleep(0.1) after that print, then it will get cancelled in that sleep.

[pjknkda]

It seems to be fixed by #6727. The server's behavior now follows what the documentation says.

[webknjaz]

Okay, sounds like it's solved.