Add ClientResponse.discard_content() method for connection reuse in middleware #10976
Conversation
…iddleware
This PR adds a new `ClientResponse.discard_content()` method to help middleware properly handle connection reuse when retrying requests. The method reads and discards the response body, allowing the connection to be released back to the pool for reuse.
When client middleware performs retries based on response status codes, connections may not be reused if the response body hasn't been consumed. This is because aiohttp only releases connections back to the pool after:
1. The response body is fully read (EOF reached)
2. The response context manager exits
3. `response.release()` is explicitly called
This behavior caused flaky tests where middleware retries would create new connections instead of reusing existing ones.
The new `discard_content()` method provides a clean API for middleware to consume response bodies without processing them:
```python
async def discard_content(self) -> None:
"""Read and discard the response body to allow connection reuse."""
if self._released:
return
# Read and discard content until EOF
while True:
chunk = await self.content.readany()
if not chunk:
break
```
**When `discard_content()` is needed:**
- Only required if the response has a body that you're not going to read
- Most small responses (where headers and body arrive in the same packet) will automatically release connections
- Empty responses (like 204 No Content) automatically release connections without any action needed
**When to use it:**
- In retry middleware that checks status codes before deciding to retry
- When you only need response headers and want to ensure connection reuse
- To be 100% certain the connection gets a chance to be reused (though reuse is never guaranteed due to other factors like connection limits, timeouts, or server behavior)
1. **Added `ClientResponse.discard_content()` method** - Efficiently reads and discards response body using `readany()`
2. **Updated all client middleware to use `discard_content()`:**
- `aiohttp/client_middleware_digest_auth.py` - Digest auth middleware
- `examples/retry_middleware.py` - Retry middleware example
- `examples/combined_middleware.py` - Combined middleware example
- `examples/token_refresh_middleware.py` - Token refresh example
- Documentation examples in `docs/client_middleware_cookbook.rst` and `docs/client_advanced.rst`
3. **Added comprehensive tests:**
- Test for responses with bodies (requires calling `discard_content()`)
- Test for empty responses (204 No Content - automatic connection release)
- Both tests verify only 1 connection is created during retries
4. **Added documentation** explaining when and why to use `discard_content()`
The fix resolves the flaky test `test_client_middleware_retry_reuses_connection` that was intermittently failing due to connections not being reused during middleware retries.
psf-chronographer
bot
added
the
bot:chronographer:provided
CodSpeed Performance ReportMerging #10976 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10976 +/- ##
==========================================
+ Coverage 98.10% 98.70% +0.60%
==========================================
Files 129 129
Lines 39541 39635 +94
Branches 2185 2195 +10
==========================================
+ Hits 38792 39123 +331
+ Misses 591 357 -234
+ Partials 158 155 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| # Consume response body before retrying | ||
| await response.discard_content() |
There was a problem hiding this comment.
We can't do it here, as this middleware knows nothing about the request. If it's a streaming response, this is just an infinite loop.
There was a problem hiding this comment.
I don't think we can do it at all then as it gets very complex to figure out.
There was a problem hiding this comment.
I guess we can make discard_content only discard content if it has a content length.
There was a problem hiding this comment.
Maybe I put the warning back and tell them that connections may not be reused if there is a response body and its advisable to avoid using middleware in this case
There was a problem hiding this comment.
Not really sure how to handle this case
There was a problem hiding this comment.
I don't think we can do it at all then as it gets very complex to figure out.
Yeah, I think just adding this code into middlewares that make sense individually may be better, rather than making this a public feature. I suspect the digest auth is fine to do this in, but most/all of the retry middlewares are probably not.
There was a problem hiding this comment.
I guess we can make discard_content only discard content if it has a content length.
That still has the issue of how much content should we allow? If it's got a length of 100GB, probably going to be a lot better to close the connection.
There was a problem hiding this comment.
I think we can make discard_content() safe... more complexity but safe
| # Consume response body before retrying | ||
| await response.discard_content() |
There was a problem hiding this comment.
Probably this one too, though less likely to cause a problem.
| # Discard response content to release connection for reuse | ||
| await response.discard_content() |
There was a problem hiding this comment.
This one probably unsafe too.
| # Discard response content to release connection for reuse | ||
| await response.discard_content() |
aiohttp/client_reqrep.py
Outdated
| await self._wait_released() # Underlying connection released | ||
| return self._body | ||
|
|
||
| async def discard_content(self) -> None: |
There was a problem hiding this comment.
I'm thinking the potential for using this in the wrong circumstance may mean it's better not to put it in as a public API? Maybe just manually do this in a couple of places that should be optimised for this case?
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| # Read and discard content with timeout | ||
| try: | ||
| async with async_timeout.timeout(timeout): |
There was a problem hiding this comment.
If timeout is >100ms, then isn't creating a new connection going to be faster in most cases?
Also, this is used in so many different situations, I'm not sure I'd be comfortable with a size limit >4KB; a slow connection could take a long time.
I'm still a bit skeptical on this, as it feels like a minor optimisation that may actually lead to slower performance for many users. It seems too difficult to get it right. e.g. Another case could be a misbehaving server which never actually sends the body (or is waiting for a response from the client first or something), so it will always hit the timeout here and never reach the content length above..
There was a problem hiding this comment.
Do you want me to just take it out?
There was a problem hiding this comment.
I'm going to abandon this PR.
There was a problem hiding this comment.
I was also thinking overnight, what is the problem we're trying to solve? For something like the auth middleware, my expectation is that a sensible service sending a 401 will have a body <1KB. If the body is that small, then it seems highly likely that it has already been read and the connection released, while it was loading the headers.
If the body is more than that, then it may well be more efficient to just close the connection anyway.
There was a problem hiding this comment.
I'm thinking about single threaded IOT devices that have a high cost of making a new connection. I think in those cases it's still ok though. The test passes most of the time with a small body. aiohttp sends the headers and body in two separate writes which isn't the case for the use case I'm worried about where everything will be in a single packet for small payloads. It would nice if we could get aiohttp to behave the same way for small payloads as I know we have downstream users implementing custom writes to get that to work with some devices
There was a problem hiding this comment.
Thinking about it, it probably makes more sense to try to implement writing in a single syscall for small payloads as it makes this problem go away and fixes the other compatibility issue above. That's probably far less complex, and win all around.
2 participants
This PR adds a new
ClientResponse.discard_content()method to help middleware properly handle connection reuse when retrying requests. The method reads and discards the response body, allowing the connection to be released back to the pool for reuse.When client middleware performs retries based on response status codes, connections may not be reused if the response body hasn't been consumed. This is because aiohttp only releases connections back to the pool after:
response.release()is explicitly calledThis behavior caused flaky tests where middleware retries would create new connections instead of reusing existing ones.
The new
discard_content()method provides a clean API for middleware to consume response bodies without processing them:When
discard_content()is needed:When to use it:
Added
ClientResponse.discard_content()method - Efficiently reads and discards response body usingreadany()Updated all client middleware to use
discard_content():aiohttp/client_middleware_digest_auth.py- Digest auth middlewareexamples/retry_middleware.py- Retry middleware exampleexamples/combined_middleware.py- Combined middleware exampleexamples/token_refresh_middleware.py- Token refresh exampledocs/client_middleware_cookbook.rstanddocs/client_advanced.rstAdded comprehensive tests:
discard_content())Added documentation explaining when and why to use
discard_content()The fix resolves the flaky test
test_client_middleware_retry_reuses_connectionthat was intermittently failing due to connections not being reused during middleware retries.