Add dns lookup error

[euri10]

continuation of #1008 and #1827
fixes #1003

WIP, looking mostly for some CI love at the moment

[codecov]

Codecov Report

Merging #2305 (d004d1c) into main (6622092) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #2305   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         2439      2446    +7     
=========================================
+ Hits          2439      2446    +7     

Impacted Files	Coverage Δ
src/urllib3/connection.py	100.00% <100.00%> (ø)
src/urllib3/connectionpool.py	100.00% <100.00%> (ø)
src/urllib3/exceptions.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6622092...d004d1c. Read the comment docs.

[pquentin]

Thank you for tackling this! Please let us know when you'd like a review

[euri10]

it seems raising socket.gaierror does not yield the same message on various platforms, so I had to remove the specific match in with pytest.raises(socket.gaierror), match=......): in the test.test_util.TestUtil.test_dnsresolver_error because the CI failed on this test

windows: [Errno 11001] getaddrinfo failed in windows
linux: [Errno -2] Name or service not known
macos: [Errno 8] nodename nor servname provided, or not known

The docs say about the error that:

The numeric error value will match one of the EAI_* constants defined in this module

so given the source here I'm not sure how to interpret the 11001 win error, I'm not familiar enough with how windows errors are "translated", would like some pointers here

#define EAI_ADDRFAMILY   1      /* address family for hostname not supported */
#define EAI_AGAIN        2      /* temporary failure in name resolution */
#define EAI_BADFLAGS     3      /* invalid value for ai_flags */
#define EAI_FAIL         4      /* non-recoverable failure in name resolution */
#define EAI_FAMILY       5      /* ai_family not supported */
#define EAI_MEMORY       6      /* memory allocation failure */
#define EAI_NODATA       7      /* no address associated with hostname */
#define EAI_NONAME       8      /* hostname nor servname provided, or not known */
#define EAI_SERVICE      9      /* servname not supported for ai_socktype */
#define EAI_SOCKTYPE    10      /* ai_socktype not supported */
#define EAI_SYSTEM      11      /* system error returned in errno */
#define EAI_BADHINTS    12
#define EAI_PROTOCOL    13
#define EAI_MAX         14

[pquentin]

I'm pretty sure EAI_* is Unix or Linux-only, Windows uses completely different errors, as you've seen. See https://docs.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2 which tells us that 11001 is WSAHOST_NOT_FOUND

[euri10]

I think it's OK for a review

I would have liked to add a more fine-grained test (with a match in the test.test_util.TestUtil.test_dnsresolver_error) which checks for the gaierror reason, but it's too OS specific, and I'm sure it adds lots of value.

I also tried to take into account this comment from a previous attempt, but I found that if you do that, you'll get the NameResolutionError switched to a ProtocolError in here because gaierror is a sublass of OSError (not sure I get this correctly though, I begin so I have the exception layout not quite well in mind !)

[pquentin]

This looks good to me! But I would like to know what bothered @sigmavirus24 in the previous PR

[pquentin]

@sigmavirus24 Do you remember what you concern was in #1827 (review)?

[sigmavirus24]

I think I'm confused as to why we're re-raising an exception with no other handling. Is it to shorten the traceback stack? What is the purpose of catching and then raising? (I'm on mobile right now so correct me if I'm missing something)

[pquentin]

You're right, this change itself seems useless. We could reraise as a NameResolutionError here instead or drop this change

[pquentin]

Thanks!

[sethmlarson]

I think we also need to add NameResolutionError to Retry._is_connection_error()?

[pquentin]

@euri10 Can you please look into this? We indeed want to retry name resolution errors

[euri10]

Sure, I'll try come up with a test for this and resolution later today !