StreamResponse write() does not raise an exception if client disconnects

[erik-moqvist]

Describe the bug

Hi!

I have an application that uses a StreamResponse to continuously write data to the client by calling the write() method. The server stops sending data when the client disconnects. In version 3.8.1 the write() method raises an exception when the client disconnects, but in later version it does not. The commit that introduces the bug is 20c9365.

I've tested on Python 3.7, 3.8 and 3.10.

To Reproduce

Add later if really needed.

Expected behavior

An exception is raised when the client disconnects.

Logs/tracebacks

-

Python Version

$ python --version
Python 3.10.7

aiohttp Version

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.8.3
...

multidict Version

-

yarl Version

-

OS

Linux

Related component

Server

Additional context

No response

Code of Conduct

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

[Dreamsorcerer]

What exception are you expecting? That code only removes the task cancellation. So, if you're expecting a CancelledError, then yes, that's the expected behaviour, it won't be cancelled anymore.

There was some confusion with the change being made in 3.7, but accidentally getting reverted somewhere. In 3.9 the behaviour is configurable. You can see the old and new behaviour (and how to configure them in 3.9) documented at:
https://docs.aiohttp.org/en/latest/web_advanced.html#peer-disconnection

The behaviour is properly tested now, so mistaken changes shouldn't happen again.

[erik-moqvist]

Further testing shows that the problem only exists when using SSL/TLS. Without ssl_context the ConnectionResetError exception is raised when the client disconnects, which is what I expect to happen when using SSL/TLS as well.

Here is a program that has the problem:

import ssl
import asyncio
from aiohttp import web


async def handle_stream(request):
    print('request')
    response = web.StreamResponse()
    await response.prepare(request)

    try:
        while True:
            await response.write(b"1")
            await asyncio.sleep(0.1)
    finally:
        print('done')


app = web.Application()
app.add_routes([web.get('/', handle_stream)])

ssl_context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
ssl_context.load_cert_chain('cert.pem', 'key.pem')
web.run_app(app, ssl_context=ssl_context)

# web.run_app(app)

Run it in one terminal. In another terminal, run wget --no-check-certificate https://localhost:8443 and then press CTRL-C disconnect the client. The ConnectionResetError exception is not raised.

Create cert.pem and key.pem with:

openssl req -x509 -newkey rsa:4096 -keyout key.pem -out cert.pem -days 365 -nodes -subj "/CN=example.com"

[Dreamsorcerer]

OK, that's definitely a problem. Will try to find some time soon to take a look, thanks for the reproducer.

[Dreamsorcerer]

Although we generally recommend deploying behind a proxy, so we expect most installs to not be using TLS on their application.

[balloob]

This report explains why we had users run into this, but I was unable to reproduce it locally. Some of our users use SSL, others don't. Thanks for the ping @Dreamsorcerer

[Dreamsorcerer]

Don't have time to look properly yet, but I think the issue may be in asyncio.

Where the exception should be raised:
https://github.com/aio-libs/aiohttp/blob/master/aiohttp/http_writer.py#L75

The http transport is _SelectorSocketTransport and .is_closing() returns True when the connection is severed:
https://github.com/python/cpython/blob/main/Lib/asyncio/selector_events.py#L907
The https transport is _SSLProtocolTransport and .is_closing() continues to return False after the connection is severed:
https://github.com/python/cpython/blob/main/Lib/asyncio/sslproto.py#L78

It's not obvious to me why the SSL one is not marked as closing.

[bdraco]

@erik-moqvist can you try #7180 ?

[Dreamsorcerer]

I'll have to come back to this at the end of the week to try out, but I don't see how that matches up with the information I posted above. Have you tested it with the reproducer above? #7172 (comment)

When I tested it, transport was not None, it was a _SSLProtocolTransport, and is_closing() kept returning False.

[bdraco]

I'll have to come back to this at the end of the week to try out, but I don't see how that matches up with the information I posted above. Have you tested it with the reproducer above? #7172 (comment)

Yes

When I tested it, transport was not None, it was a _SSLProtocolTransport, and is_closing() kept returning False.

self._transport will not be None when the connection is closed but self._protocol.transport will be None

drain is checking self._protocol.transport as well:

aiohttp/aiohttp/http_writer.py

Line 172 in 4635161

if self._protocol.transport is not None:

StreamWriter's self._protocol.transport set to None on the BaseProtocol when the connection is lost here:
https://github.com/aio-libs/aiohttp/blob/master/aiohttp/base_protocol.py#L64

self._transport is only ever set to None in write_eof

aiohttp/aiohttp/http_writer.py

Line 162 in 4635161

self._transport = None

Maybe that should be changed to:

diff --git a/aiohttp/http_writer.py b/aiohttp/http_writer.py
index 16b28cc7..4af237a2 100644
--- a/aiohttp/http_writer.py
+++ b/aiohttp/http_writer.py
@@ -35,7 +35,6 @@ class StreamWriter(AbstractStreamWriter):
         on_headers_sent: _T_OnHeadersSent = None,
     ) -> None:
         self._protocol = protocol
-        self._transport = protocol.transport
 
         self.loop = loop
         self.length = None
@@ -52,7 +51,7 @@ class StreamWriter(AbstractStreamWriter):
 
     @property
     def transport(self) -> Optional[asyncio.Transport]:
-        return self._transport
+        return self._protocol.transport
 
     @property
     def protocol(self) -> BaseProtocol:
@@ -166,7 +165,6 @@ class StreamWriter(AbstractStreamWriter):
         await self.drain()
 
         self._eof = True
-        self._transport = None
 
     async def drain(self) -> None:
         """Flush the write buffer.

... but I was trying to keep this as small as possible

[erik-moqvist]

@erik-moqvist can you try #7180 ?

Yes, this fixes the problem for me.