Fix spurious "Future exception was never retrieved" warnings for connection lost errors #11100
Conversation
…ctions TimeoutError: SSL shutdown timed out The above exception was the direct cause of the following exception: aiohttp.client_exceptions.ClientConnectionError: Connection lost: SSL shutdown timed out
bdraco
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #11100 +/- ##
=======================================
Coverage 98.82% 98.82%
=======================================
Files 129 129
Lines 41493 41505 +12
Branches 2234 2234
=======================================
+ Hits 41007 41019 +12
Misses 337 337
Partials 149 149
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
psf-chronographer
bot
added
the
bot:chronographer:provided
bdraco
added
backport-3.12
CodSpeed Performance ReportMerging #11100 will not alter performanceComparing Summary
|
Backport to 3.12: 💚 backport PR created✅ Backport PR branch: Backported as #11101 🤖 @patchback |
Backport to 3.13: 💚 backport PR created✅ Backport PR branch: Backported as #11102 🤖 @patchback |
…s never retrieved" warnings for connection lost errors (#11101) Co-authored-by: J. Nick Koston <[email protected]>
…s never retrieved" warnings for connection lost errors (#11102) Co-authored-by: J. Nick Koston <[email protected]>
| # The exception is always passed on through | ||
| # other means, so this is safe | ||
| with suppress(Exception): | ||
| self.closed.exception() |
There was a problem hiding this comment.
Should this maybe be handled by the set_exception() helper?
I assume that function is the one responsible for sending the exception elsewhere, so seems to me that it'd make sense for it to also suppress the unretrieved exception that it's handling.
There was a problem hiding this comment.
In this case it's the waiter below that is responsible for propagating the exception. We effectively have it in two places. There are other places where we don't so we can't make set_exception responsible as it doesn't know if it should suppress the warning or not
What do these changes do?
This PR fixes spurious "Future exception was never retrieved" warnings that occur when connections are lost (e.g., SSL shutdown timeouts, network errors) and the connector is not immediately closed.
The fix marks the exception on the
ResponseHandler.closedfuture as retrieved after setting it, since the exception is always propagated through other means (via the waiter). This prevents Python from logging unnecessary warnings about unretrieved exceptions.Are there changes in behavior for the user?
No functional changes. Users will no longer see spurious "Future exception was never retrieved" warnings in their logs when connections are lost.
Related issue number
This issue was introduced by #3733 (backported as #11074), which added proper connection closure waiting but exposed these futures that may not be awaited until the connector is closed.