Add ClientConnectorDNSError for differentiating DNS errors from others #8456
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
tests/test_client_functional.py
Dismissed
| with pytest.raises(aiohttp.ClientConnectorError) as excinfo: | ||
| async with aiohttp.request("GET", "http://localhost:1/"): | ||
| assert False, "never executed" # pragma: no cover | ||
| assert not isinstance(excinfo.value, aiohttp.ClientConnectorDNSError) |
Check warning
Code scanning / CodeQL
Unreachable code Warning
There was a problem hiding this comment.
Is there a comment I should add to ignore this, or is it fine to proceed with the warning?
There was a problem hiding this comment.
There used to be comments back when the tool used to be called LGTM. I'm not sure anymore. But I can click the "dismiss alert" button since I can see that coverage is actually reported on this line.
OTOH, why don't you put the exception into pytest.raises(). That's what's supposed to be checked anyway.
I think CodeQL understands that assert False interrupts the control flow of a test function so it naturally thinks the outer line wouldn't be hit. It'd probably be better to call pytest.fail() there instead. I wonder if wrapping such a call into assert_never() would let us drop the coverage ignore comment as well...
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8456 +/- ##
==========================================
- Coverage 97.64% 97.62% -0.03%
==========================================
Files 107 107
Lines 33067 33075 +8
Branches 3885 3887 +2
==========================================
Hits 32288 32288
- Misses 562 568 +6
- Partials 217 219 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -0,0 +1 @@ | |||
| Added `ClientConnectorDNSError` for differentiating DNS resolution errors from other connector errors. | |||
There was a problem hiding this comment.
|
We need to think the DNS-related exception hierarchy through — perhaps it needs a base exception with subtypes. Similar to #6722 (comment). Additionally, the documentation would need to be updated to list the new exceptions with explanations. |
| @@ -166,6 +167,14 @@ def __str__(self) -> str: | |||
| __reduce__ = BaseException.__reduce__ | |||
|
|
|||
|
|
|||
| class ClientConnectorDNSError(ClientConnectorError): | |||
There was a problem hiding this comment.
Not sure if this should be a connector error, though. Any reasoning?
There was a problem hiding this comment.
It's currently a ConnectorError so it would allow existing code that tries to catch ConnectorErrors to keep working. I'm open to taking a different approach though.
There was a problem hiding this comment.
Yeah, I understand. If you read the PR I linked, there were similar concerns, and I know that some bits of this might be breaking. In that instance, we ended up using multiple inheritance and intermediate exceptions in the hierarchy to maximize the compatibility while enabling flexibility and accuracy.
What do these changes do?
Adds a ClientConnectorError that's specific to DNS resolution errors. See #8455
Are there changes in behavior for the user?
Existing usages should still keep working the same as before since this extends from ClientConnectorError.
Users can now catch ClientConnectorDNSError if they're looking for DNS errors specifically.
Is it a substantial burden for the maintainers to support this?
I can't see this being difficult to maintain since the Resolver abstraction already exists to determine when errors are related to DNS or some other part of the connection.
Related issue number
Fixes #8455
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.