Repository navigation
_SSLProtocolTransport.close() cannot close the connection until timeout or EOF #471
Description
Activity
- added a commit that references this issue
on May 21, 2022 - changed the title
[-]aiohttp-client connections cannot be really closed when uvloop installed[/-][+]aiohttp-client connections cannot be really closed before EOF when uvloop enabled[/+]on May 21, 2022 Some statistics in my production environment. (In my use case only the response headers and/or the beginning 4KB of the response body is needed.)
I'd done some deep-dive and found that the bug is a TLS/SSL-relevant bug. Non-TLS connections can be closed even if
uvloopis enabled.- await fetch('https://sgp-ping.vultr.com/vultr.com.1000MB.bin') + await fetch('http://sgp-ping.vultr.com/vultr.com.1000MB.bin')
I also analyzed the web traffic with Wireshark. With
asyncio,RSTis immediately sent once the connection was closed; but withuvloop, closing the connection won't change anything as if is it not closed,RSTis only sent after the event loop is closed.Another deep-dive:
async def fetch(url): async with aiohttp.ClientSession() as session: async with session.get(url) as response: logging.debug(response) logging.debug(response.content) + await asyncio.sleep(5) # at this point, only a little part of the response is read, then pends + return # after the connection is "closed", uvloop will resume reading the responseA deeper dive shows the direct cause of the bug.
async def reproduce(wait): await fetch('https://sgp-ping.vultr.com/vultr.com.1000MB.bin') - await asyncio.sleep(wait) # now you will see that the file is still being downloaded + await asyncio.sleep(35) # the download will last for exactly 30s, then abort
aiohttpclosing its high-level connection causesuvloopto call_SSLProtocolTransport.close().Lines 42 to 51 in 3e71ddc
Lines 559 to 574 in 3e71ddc
We can easily verify that the low-level connection is closed 30 seconds after
aiohttphave "closed" its high-level connection (as long as the response body is big enough). This complies withSSL_SHUTDOWN_TIMEOUT. That is to say, it isSSLProtocol._ssl_shutdown_timeout()that eventually closes the connection.uvloop/uvloop/includes/consts.pxi
Line 24 in 3e71ddc
Now that
SSLProtocol._ssl_shutdown_timeout()is called, the conclusion is evident thatSSLProtocol._do_flush()causes the reading to be resumed but meanwhile is stuck in something until EOF or timeout is reached, causing the reading sustained.Lines 600 to 618 in 3e71ddc
Here's a simple hotfix to stop
uvloopfrom reading the response happily and not willing to close the connection.class AiohttpUvloopTransportHotfix(contextlib.AbstractAsyncContextManager): def __init__(self, response: aiohttp.ClientResponse): self.response = response async def __aexit__(self, exc_type, exc_val, exc_tb): if UVLOOP_ENABLED: self.response.connection.transport.abort() async def fetch(url): async with aiohttp.ClientSession() as session: async with session.get(url) as response: async with AiohttpUvloopTransportHotfix(response): ...
Reacted by Marvin Wichmann- added a commit that references this issue
on May 25, 2022 i have the same problem with httpS server using uvloop, but with asyncio it works as expected.
After close() call i cant see new data_received calls associated with "closed" connection, but incoming traffic persists for about 10 seconds after transport.close(). switching to abort() didnt helpHi, @rojamit. I don't really so much familiar with aio servers. Could you please figure out some code to reproduce that?
(I wonder, since it is a TLS/SSL issue, why aio servers are affected? Typically speaking a server itself should not deal with TLS/SSL itself as it is the business of reverse proxies, e.g. Nginx)@Rongronggg9 right, its a https reverse proxy implemented with asyncio and httptools. problem occurs only with uvloop, and looks like 1:1 as yours, but on the server-side
about code:
- server-side: attaching client sockets using loop.connect_accepted_socket with ssl=ssl_context
- client-side: connect to server then while True: write data at maximum speed
- server-side: transport.close()
and... traffic still persist for about 10-30 seconds AFTER close()
without uvloop - no problems like this
notice: no new data_received calls after close() occurs, but traffic and CPU load
- changed the title
[-]aiohttp-client connections cannot be really closed before EOF when uvloop enabled[/-][+]`_SSLProtocolTransport.close()` cannot close the connection until timeout or EOF[/+]on Jun 15, 2022 @Rongronggg9 hmm, is uvloop abandoned?...
Hopefully not. Historically speaking, some of the recent versions were released right after a long-time radio silence. If someone figures out a PR to fix this, it could make the process faster. I am not really familiar with C-binding libraries so I can do little.
Sorry for the late reply! With a quick look through the details (thanks @Rongronggg9 for the debugging!), this is probably because the remote peer didn't reply a proper TLS
close_notify.More specifically,
transport.close()is similar tosocket.close(), they don't imply an immediate shutdown on the low-level resources. Instead, they are supposed to do a graceful shutdown, with all the write buffer flushed and nicely closes the connection - in TLS 1.3 this means each end sends aclose_notifymessage. Which TLS version are you using btw?6 remaining items
So here locates the buggy code:
cdef _do_flush(self, object context=None): try: self._do_read_into_void(context) self._do_write() self._process_outgoing() self._control_ssl_reading() except Exception as ex: self._on_shutdown_complete(ex) else: if not self._get_write_buffer_size(): self._set_state(SHUTDOWN) self._do_shutdown(context) cdef _do_shutdown(self, object context=None): try: # we must skip all application data (if any) before unwrap self._do_read_into_void(context) # <--- I am so sad because I am buggy try: self._sslobj.unwrap() except ssl_SSLAgainErrors as exc: self._process_outgoing() else: self._process_outgoing() if not self._get_write_buffer_size(): self._on_shutdown_complete(None) except Exception as ex: self._on_shutdown_complete(ex)
Being commented out,
uvloopshows 100% the same behavior as vanilla asyncio in CPython 3.11 (closing the connection as expected and logging the fatal error):DEBUG:asyncio:<uvloop.loop.SSLProtocol object at 0x7f9cbfbcf5e0>: Error occurred during shutdown Traceback (most recent call last): File "uvloop/sslproto.pyx", line 624, in uvloop.loop.SSLProtocol._do_shutdown File "/usr/lib/python3.11/ssl.py", line 983, in unwrap return self._sslobj.shutdown() ^^^^^^^^^^^^^^^^^^^^^^^ ssl.SSLError: [SSL: APPLICATION_DATA_AFTER_CLOSE_NOTIFY] application data after close notify (_ssl.c:2672)
You may also inspect the source code of both uvloop and asyncio yourself. Vanilla asyncio in CPython 3.11 indeed does NOT read before unwrap in the shutdown stage.
def _do_flush(self): self._do_read() self._set_state(SSLProtocolState.SHUTDOWN) self._do_shutdown() def _do_shutdown(self): try: if not self._eof_received: self._sslobj.unwrap() except SSLAgainErrors: self._process_outgoing() except ssl.SSLError as exc: self._on_shutdown_complete(exc) else: self._process_outgoing() self._call_eof_received() self._on_shutdown_complete(None)
https://github.com/python/cpython/blob/3.11/Lib/asyncio/sslproto.py#L643
this is probably because the remote peer didn't reply a proper TLS close_notify.
After reading more refs, I finally realized that there do be such a probability that the "weird" behavior is caused by misbehaved servers (my case) or clients (#471 (comment)).
Indeed, according to the spec (e.g. https://www.rfc-editor.org/rfc/rfc5246), waiting for
close_notify("two-way shutdown") is considered more secure (to get rid of truncation attacks). However, there are tons of server/client implementations never reply toclose_notify, making the "secure" behavior "leak" bandwidth. In all use cases of TLS, HTTPS is absolutely the most widespread, and HTTP/1.0 or newer is not vulnerable to truncation attacks no matter whether the TLS shutdown is one-way or two-way.So there are three solutions:
- just act like how CPython asyncio does, ignore the
APPLICATION_DATA_AFTER_CLOSE_NOTIFYerror, and no longer wait forclose_notifyreply from the other side. - keep the current behavior, but add more procedures to determine if the other side will never reply to
close_notify, if so, drop the connection early (the current 30s timeout insanely leaks bandwidth). - (?) add an option to switch between the two behavior.
The violation of spec is so widespread that until now no other one realized that it is caused by a violation. My preference is 1.
- just act like how CPython asyncio does, ignore the
Thanks for the very detailed reply and sorry for the delay!
I think we should do (1) for TLS 1.2 and lower to "conform with widespread implementation practice", and cut down the
SSL_SHUTDOWN_TIMEOUTto maybe 10s to reduce the impact in TLS 1.3.Python 3.11 behaves differently because #385 wasn't merged in, I'll probably create a PR to do #385 there, together with the proposed fix here.
- added a commit that references this issue
on Oct 1, 2022 cut down the
SSL_SHUTDOWN_TIMEOUTto maybe 10s to reduce the impact in TLS 1.3.At first, I consider it is OK. After doing some tests, I was so sad to find that Cloudflare still violates the spec even in TLS 1.3... It means that the situation of the violation does not become much better in the world of TLS 1.3.
https://speed.cloudflare.com/__down?bytes=500000000For servers, they maintain a connection pool and recycle connections immediately (which relies on client-side one-way shutdowns), not willing to "waste" any bandwidth to reply to
close_notify. For client developers, they don't know much and hardly care about the spec, but worry about bandwidth a lot.I think we should reconsider Option 3 ("add an option to switch between the two behavior"), both for uvloop and CPython asyncio. If already known that the
SSLProtocolis used by HTTP/1.0 or later, libraries on top of uvloop or asyncio can simply force one-way shutdowns, and vice versa.@Dreamsorcerer @asvetlov @webknjaz Sorry for disturbing you. What do you think about the issue?
I think we should reconsider Option 3 ("add an option to switch between the two behavior"), both for uvloop and CPython asyncio. If already known that the
SSLProtocolis used by HTTP/1.0 or later, libraries on top of uvloop or asyncio can simply force one-way shutdowns, and vice versa.@Dreamsorcerer @asvetlov @webknjaz Sorry for disturbing you. What do you think about the issue?
I'm no expert with TLS, so I'm just relying on your investigations. :P
My only question if we add a switch, who changes the switch? If it's the user, when would they know which option to use?
Reacted by 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко)My only question if we add a switch, who changes the switch? If it's the user, when would they know which option to use?
That could be a question to be discussed. What's your preference for that? My preference is that everyone can change it, but HTTP libraries should have two-way shutdowns turned off by default. Option 3 is just my personal advice, not a result of the discussion. You may have other choices :)
Even if both CPython asyncio and uvloop adopted #507, libraries on top of them could still call
.abort()to roughly simulate a one-way shutdown.In fact, I am more concerned about aiohttp maintainers' opinion about the question that "comfort to the spec or save users' bandwidth?" At least for HTTP/1.0 or later, doing a two-way shutdown does not show additional advantages according to my research, but wastes a lot of bandwidth if the other side only supports one-way shutdowns. Most users do not expect such behavior.
Actually I got a new idea. We can implement the
SSLTransport.write_eof()method and move the two-way shutdown there, while changing the behavior ofSSLTransport.close()to one-way shutdown to meet the general need, as the RFC 8446 does say:Both parties need not wait to receive a "close_notify" alert before closing their read side of the connection, though doing so would introduce the possibility of truncation.
If the status quo (of CPython, both <= 3.10 and == 3.11) is already taking this risk of truncation, I think it is okay to keep it that way. Users who do care about this risk could just
write_eof()and explicitly wait untileof_received(), and then close the transport safely. This also opens doors to implementing "TLS downgrade" - the counter-version ofloop.start_tls().Proposed shutdown flow:
So basically user could now 1) call
write_eof()(and expect to receive more TLS data), or 2) return different values ineof_received(). Specifically in (2), if the returned value is a false value, we just close the underlying transport; or else if it's a true value:- We keep the underlying transport open. User could still send TLS data if
write_eof()wasn't called, and eventually close the transport when the user chooses to do so. - If the "true value" is an asyncio protocol object, we downgrade the TLS connection to a plaintext connection using the given protocol object, by calling
set_protocol()on the underlying transport. TheSSLProtocolwill be discarded, and the given protocol will receive the underlying transport throughconnection_made().
- We keep the underlying transport open. User could still send TLS data if
@fantix Wow, your flow graph is so clear. I second your proposal!
There is a slight question, however. How can a user "return different values in
eof_received()"? Shouldn't it be a method ofSSLProtocol? Or users are expected to subclassSSLProtocolto change the behavior?哈哈，哪里哪里🤓谢谢
You may notice 2
eof_received()in the diagram, the green one is the one you mentioned inSSLProtocol, while the red one is a user-provided protocol implementation inheriting directly fromasyncio.Protocol, a.k.a. “app protocol”. And that’s the one user would return different values from.You may notice 2
eof_received()in the diagram, the green one is the one you mentioned inSSLProtocol, while the red one is a user-provided protocol implementation inheriting directly fromasyncio.Protocol, a.k.a. “app protocol”. And that’s the one user would return different values from.Understood. Then I have no more questions and thanks for your explanation :)
- added 2 commits that reference this issue
on Jun 17, 2024
PYTHONASYNCIODEBUGin env?: yesDesciption
If
uvloopis enabled, as long as EOF is not reached, even if the response, connection, and session are all closed, the server from which aiohttp requested resources, will still keep sending packets to the aiohttp-client for some time. However, the bug is only reproducible with uvloop enabled.To reproduce
Log and screenshot
The bug has also been reported to
aiohttp: aio-libs/aiohttp#6762