Fix TCPConnector.close() resolver race on non-cached path (#12497)#12498
Fix TCPConnector.close() resolver race on non-cached path (#12497)#12498masonwilde wants to merge 7 commits into
Conversation
psf-chronographer
Bot
added
the
bot:chronographer:provided
for more information, see https://pre-commit.ci
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| await closed_event.wait() | ||
|
|
||
| token: ResolveResult = { | ||
| "hostname": "localhost", |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12498 +/- ##
==========================================
+ Coverage 98.55% 98.94% +0.39%
==========================================
Files 131 131
Lines 46558 46583 +25
Branches 2409 2410 +1
==========================================
+ Hits 45883 46092 +209
+ Misses 528 368 -160
+ Partials 147 123 -24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Merging this PR will improve performance by 8.79%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
for more information, see https://pre-commit.ci
| def __init__(self) -> None: | ||
| """Dummy""" | ||
|
|
There was a problem hiding this comment.
Probably makes sense to remove this and pass the necessary arguments just to be sure we don't end up with any other errors in future caused by the missing attributes.
| with mock.patch.object(connector._resolver, "resolve", fake_resolve): | ||
| with mock.patch.object(connector._resolver, "close", mock.AsyncMock()): |
There was a problem hiding this comment.
We should use autospec/spec_set here. Something like:
| with mock.patch.object(connector._resolver, "resolve", fake_resolve): | |
| with mock.patch.object(connector._resolver, "close", mock.AsyncMock()): | |
| with mock.patch.object(connector._resolver, "resolve", autospec=True, spec_set=True) as m: | |
| m.return_value = [token] | |
| with mock.patch.object(connector._resolver, "close", autospec=True, spec_set=True): |
Dreamsorcerer
added
the
pr-unfinished
3 participants
What do these changes do?
Fix a race condition in
TCPConnector.close()where theAsyncResolver's internal_resolverreference is nullified before the connector is marked closed (_closed = True). When an in-flight connection is suspended in a trace callback at the time of close, it resumes and callsself._resolver.resolve()onNone, producingAttributeError: 'NoneType' object has no attribute 'getaddrinfo'.Two changes:
TCPConnector.close()— callsuper().close()beforeself._resolver.close()so_closedisTruebefore the resolver is torn down._closedguard in_resolve_host()— on the non-cached path (use_dns_cache=False), checkself._closedafter trace callbacks yield and before callingself._resolver.resolve(). The cached path is already handled by_close_immediately()which cancels tracked_resolve_host_tasks.Are there changes in behavior for the user?
Connections that previously crashed with
AttributeErrorduring a concurrentsession.close()now raiseClientConnectionError("Connector is closed").Is it a substantial burden for the maintainers to support this?
No. The close reorder is a two-line swap with no new API surface. The
_closedguard is a single conditional in an existing method. Both follow patterns already used elsewhere in the connector (e.g._close_immediatelycancelling tracked resolve tasks on the cached path).Related issue number
Fixes #12497
Checklist
CONTRIBUTORS.txtCHANGES/foldername it
<issue_or_pr_num>.<type>.rst(e.g.588.bugfix.rst)if you don't have an issue number, change it to the pull request
number after creating the PR
.bugfix: A bug fix for something the maintainers deemed animproper undesired behavior that got corrected to match
pre-agreed expectations.
.feature: A new behavior, public APIs. That sort of stuff..deprecation: A declaration of future API removals and breakingchanges in behavior.
.breaking: When something public is removed in a breaking way.Could be deprecated in an earlier release.
.doc: Notable updates to the documentation structure or buildprocess.
.packaging: Notes for downstreams about unobvious side effectsand tooling. Changes in the test invocation considerations and
runtime assumptions.
.contrib: Stuff that affects the contributor experience. e.g.Running tests, building the docs, setting up the development
environment.
.misc: Changes that are hard to assign to any of the abovecategories.
Make sure to use full sentences with correct case and punctuation,
for example:
Use the past tense or the present tense a non-imperative mood,
referring to what's changed compared to the last released version
of this project.