Add timeout in aiohttp.close #41374

xiangyan99 · 2025-05-31T00:01:30Z

No description provided.

sdk/core/azure-core/CHANGELOG.md

Co-authored-by: Laurent Mazuel <[email protected]>

jabbera · 2025-06-02T16:06:43Z

This doesn't fix the performance issue but reduced it to 100ms from 30s for every connection close. It looks like aiohttp plans to release a 3.12.7 that moves the error to debug, but I really think all of this is a bad idea because it's just masking 100ms of latency on closing every connection. It sounds like the SDK should be aborting the transport instead of closing it, or the Azure TLS stack should be sending a close_notify back when it gets one from the client.

xiangyan99 · 2025-06-02T16:13:44Z

This work around the performance issue but still adds 100ms of latency to every connection close. It looks like aiohttp plans to release a 3.12.7 that moves the error to debug, but I really think all of this is a bad idea because it's just masking 100ms of latency on closing every connection. It sounds like the SDK should be aborting the transport instead of closing it, or the Azure TLS stack should be sending a close_notify back when it gets one from the client.

Agreed. This solution isn't perfect.

But TLS 1.3 does not seem to require sending one immediately.

The change sets a limit to ensure added latency doesn't exceed 100ms, which helps reduce risk.

jabbera · 2025-06-03T14:45:36Z

The change sets a limit to ensure added latency doesn't exceed 100ms, which helps reduce risk.

Since we know Azure is not sending the close_notify why not replace this with a call_soon or something that doesn't wait at all? The TLS spec 1.3 allows for this behavior explicitly. Adding 100ms to every close connection doesn't make any sense even though it's better than 30.

TLS 1.3 RFC

   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.

xiangyan99 · 2025-06-03T14:54:09Z

The change sets a limit to ensure added latency doesn't exceed 100ms, which helps reduce risk.

Since we know Azure is not sending the close_notify why not replace this with a call_soon or something that doesn't wait at all? The TLS spec 1.3 allows for this behavior explicitly. Adding 100ms to every close connection doesn't make any sense even though it's better than 30.

TLS 1.3 RFC
   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.

aiohttp is not happy if we close it immediately w/o waiting.

I saw

Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x0000024289BA3F50>
Unclosed connector
connections: ['deque([(<aiohttp.client_proto.ResponseHandler object at 0x000002428AD5A210>, 115033.39)])']
connector: <aiohttp.connector.TCPConnector object at 0x000002428AD84A70>

0.1s seems also the default timeout for aiohttp.

ssl_shutdown_timeout - Grace period for SSL shutdown handshake on TLS
connections. Default is 0.1 seconds. This usually
allows for a clean SSL shutdown by notifying the
remote peer of connection closure, while avoiding
excessive delays during connector cleanup.
Note: Only takes effect on Python 3.11+.

jabbera · 2025-06-03T15:16:18Z

ssl_shutdown_timeout - Grace period for SSL shutdown handshake on TLS
connections. Default is 0.1 seconds. This usually
allows for a clean SSL shutdown by notifying the
remote peer of connection closure, while avoiding
excessive delays during connector cleanup.
Note: Only takes effect on Python 3.11+.

That setting was added in 3.12.6 if you can take a minimum dependency on that you can add ssl_shutdown_timeout to the client session args here and set it to something comically small: .000001 for example. aio-libs/aiohttp#11091 (comment)

azure-sdk-for-python/sdk/core/azure-core/azure/core/pipeline/transport/_aiohttp.py

Line 154 in 06bd2c4

clientsession_kwargs = {

xiangyan99 · 2025-06-03T15:18:06Z

ssl_shutdown_timeout - Grace period for SSL shutdown handshake on TLS
connections. Default is 0.1 seconds. This usually
allows for a clean SSL shutdown by notifying the
remote peer of connection closure, while avoiding
excessive delays during connector cleanup.
Note: Only takes effect on Python 3.11+.

That setting was added in 3.12.6 if you can take a minimum dependency on that you can add ssl_shutdown_timeout to the client session args here and set it to something comically small: .000001 for example. aio-libs/aiohttp#11091 (comment)

azure-sdk-for-python/sdk/core/azure-core/azure/core/pipeline/transport/_aiohttp.py

Line 154 in 06bd2c4

clientsession_kwargs = {

But unfortunately, this parameter does not work for py3.11- and our library needs to support py3.9 & 3.10.

jabbera · 2025-06-03T15:27:54Z

But unfortunately, this parameter does not work for py3.11- and our library needs to support py3.9 & 3.10.

That's fine, but even the aiohttp people are saying you want something smaller then 100ms: aio-libs/aiohttp#11091 (comment)

100ms is an astronomical amount of time to wait for something in network land you know will never happen. We have pinned to 3.12.3 so I'm going to drop this issue for now, but I don't know any other way to say that I think this timeout is too long given that you know it will always be triggered.

xiangyan99 · 2025-06-03T15:37:55Z

But unfortunately, this parameter does not work for py3.11- and our library needs to support py3.9 & 3.10.

That's fine, but even the aiohttp people are saying you want something smaller then 100ms: aio-libs/aiohttp#11091 (comment)

100ms is an astronomical amount of time to wait for something in network land you know will never happen. We have pinned to 3.12.3 so I'm going to drop this issue for now, but I don't know any other way to say that I think this timeout is too long given that you know it will always be triggered.

changed to 1ms.

jabbera · 2025-06-03T15:43:26Z

changed to 1ms.

Thanks! Here is the perf difference between 100ms and 1ms with raw aiohttp in a loop using the new parameter. I really think this change makes more sense now.

sdk/core/azure-core/CHANGELOG.md

sdk/core/azure-core/tests/async_tests/test_aiohttp_blob_download_live_async.py

kristapratico · 2025-06-03T17:36:53Z

sdk/core/azure-core/tests/async_tests/test_aiohttp_blob_download_live_async.py

+
+
+@pytest.mark.skipif(
+    sys.version_info < (3, 11), reason="ssl_shutdown_timeout in aiohttp only takes effect on Python 3.11+"


Do we need to skip 3.9/3.10? Ideally, we see no warnings logged for those versions, too.

I see errors when using aiohttp 3.12.7 with py 3.9.

What errors? Things should still work with Python 3.9+ since we are only adding a timeout to asyncio.wait_for and not using ssl_shutdown_timeout.

Also, let's @pytest.mark.live_test_only this test. Ideally, we use the storage account that's provisioned in test-resources.bicep.

aiohttp hangs in Python 3.9 (compared to a 30-second delay in 3.11). In 3.9, we have to forcefully stop aiohttp, and an error from aiohttp appeared during my test on Python 3.9.

I wasn't able to reproduce an error locally with 3.9/3.10. Did you see this on CI?

I see this on py3.9 w/ timeout in azure-core.aiohttp added:

Exception ignored in: <function _ProactorBasePipeTransport.del at 0x000002255D531670>
Traceback (most recent call last):
File "C:\Users\xiangyan\AppData\Local\Programs\Python\Python39\lib\asyncio\proactor_events.py", line 116, in del
self.close()
File "C:\Users\xiangyan\AppData\Local\Programs\Python\Python39\lib\asyncio\proactor_events.py", line 108, in close
self._loop.call_soon(self._call_connection_lost, None)
File "C:\Users\xiangyan\AppData\Local\Programs\Python\Python39\lib\asyncio\base_events.py", line 751, in call_soon
self._check_closed()
File "C:\Users\xiangyan\AppData\Local\Programs\Python\Python39\lib\asyncio\base_events.py", line 515, in _check_closed
raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed

Oh, I think I'm not seeing it because I'm running on WSL. Looks like that ^ only shows up in windows.

I see that runtime exception show up in windows/py3.9 even without the timeout changes in this PR.

Right. aiohttp 3.12.7 does not solve the problem on py 3.9. (I did not test py 3.10)

sdk/core/azure-core/tests/async_tests/test_aiohttp_close_timeout.py

sdk/core/azure-core/azure/core/pipeline/transport/_aiohttp.py

pvaneck · 2025-06-03T22:05:34Z

sdk/core/azure-core/tests/async_tests/test_aiohttp_blob_download_live_async.py

+
+
+@pytest.mark.skipif(
+    sys.version_info < (3, 11), reason="ssl_shutdown_timeout in aiohttp only takes effect on Python 3.11+"


What errors? Things should still work with Python 3.9+ since we are only adding a timeout to asyncio.wait_for and not using ssl_shutdown_timeout.

Also, let's @pytest.mark.live_test_only this test. Ideally, we use the storage account that's provisioned in test-resources.bicep.

Add timeout in aiohttp.close

dc0c874

lmazuel reviewed May 31, 2025

View reviewed changes

sdk/core/azure-core/CHANGELOG.md Outdated Show resolved Hide resolved

xiangyan99 and others added 3 commits May 30, 2025 17:04

update

b1d0564

Update sdk/core/azure-core/CHANGELOG.md

99049de

Co-authored-by: Laurent Mazuel <[email protected]>

update

8cd2024

xiangyan99 marked this pull request as ready for review June 2, 2025 15:17

xiangyan99 requested review from annatisch, johanste, iscai-msft, kashifkhan and pvaneck as code owners June 2, 2025 15:17

xiangyan99 added 3 commits June 2, 2025 13:52

add test

31361f3

update

ca97c90

update

a626796

xiangyan99 requested review from scbedd, mccoyp, benbp and weshaggard as code owners June 2, 2025 21:26

add test

17eb33d

change timeout to 0.001

b8a1a25

kristapratico reviewed Jun 3, 2025

View reviewed changes

johanste reviewed Jun 3, 2025

View reviewed changes

sdk/core/azure-core/azure/core/pipeline/transport/_aiohttp.py Outdated Show resolved Hide resolved

updates

60e2d08

xiangyan99 requested review from johanste and kristapratico June 3, 2025 21:26

pvaneck reviewed Jun 3, 2025

View reviewed changes

xiangyan99 added 2 commits June 3, 2025 15:58

update timeout

1027157

update

bc155e9

xiangyan99 requested a review from pvaneck June 3, 2025 23:56

update release date

4f0cb37



		@pytest.mark.skipif(
		sys.version_info < (3, 11), reason="ssl_shutdown_timeout in aiohttp only takes effect on Python 3.11+"

Add timeout in aiohttp.close #41374

Are you sure you want to change the base?

Add timeout in aiohttp.close #41374

Uh oh!

Conversation

xiangyan99 commented May 31, 2025

Uh oh!

Uh oh!

jabbera commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiangyan99 commented Jun 2, 2025

Uh oh!

jabbera commented Jun 3, 2025

Uh oh!

xiangyan99 commented Jun 3, 2025

Uh oh!

jabbera commented Jun 3, 2025

Uh oh!

xiangyan99 commented Jun 3, 2025

Uh oh!

jabbera commented Jun 3, 2025

Uh oh!

xiangyan99 commented Jun 3, 2025

Uh oh!

jabbera commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jabbera commented Jun 2, 2025 •

edited

Loading

jabbera commented Jun 3, 2025 •

edited

Loading