Fixed failure to try next host after single-host connection timeout

[brettdh]

What do these changes do?

See subject. Also updated the default ClientTimeout params to include sock_connect so that this correct behavior happens by default.

Are there changes in behavior for the user?

Yes; the default socket connect timeout changes from None (no timeout) to 30 seconds.

Related issue number

Closes #7342.

Checklist

• I think the code is well written
• Unit tests for the changes exist
  • Observed test failing before code change in connector.py; passing after.
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[brettdh]

test_secure_https_proxy_absolute_path is failing on my fork, but only on 3.9 and only on Windows. No idea why, and I'm on a mac so I cannot repro. Please advise?

[Dreamsorcerer]

test_secure_https_proxy_absolute_path is failing on my fork, but only on 3.9 and only on Windows. No idea why, and I'm on a mac so I cannot repro. Please advise?

It's failing on all Windows tests. The stderr output looks like there might be a problem with proxy.py, but not really sure. @webknjaz has more experience with the proxy stuff, maybe he has an idea.

Traceback (most recent call last):
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\proxy\core\work\threadless.py", line 380, in _run_forever
    if await self._run_once():
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\proxy\core\work\threadless.py", line 341, in _run_once
    work_by_ids, new_work_available = await self._selected_events()
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\proxy\core\work\threadless.py", line 265, in _selected_events
    events = self.selector.select(
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\selectors.py", line 324, in select
    r, w, _ = self._select(self._readers, self._writers, [], timeout)
  File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\selectors.py", line 315, in _select
    r, w, x = select.select(r, w, w, timeout)
OSError: [WinError 10022] An invalid argument was supplied

[brettdh]

Ah, I misread the Github Actions results; only one windows job reports failuire; the rest just appear "cancelled" until I drill into any of them.

Screenshot

[Dreamsorcerer]

Might also be worth testing what specific change causes the test failure. Does the error happen if we change the default timeout, without the TimeoutError change?

[brettdh]

The error goes away if I temporarily revert the change to the default timeout.

[Dreamsorcerer]

Yeah, I was wondering the other way. Maybe it's caused by the sock_connect timeout being reached, or maybe it's caused by the connection retry..

[brettdh]

Whoops, sorry, I misread your earlier message. The same tests fail when the default timeout has sock_connect=30 set but the loop doesn't catch TimeoutError.

[Dreamsorcerer]

OK, I guess it's an existing issue with sock_connect timeout then.

[brettdh]

Is there anything else you need from me on this PR? I wasn't really sure about the resolution (or lack thereof) of our last exchange.

[Dreamsorcerer]

I don't think we can change the default if it's going to cause errors for all Windows users. So, we'll need to figure out a fix for that. I've not had a chance to look into it yet, but if you have time to figure that out, it'd help us get this done.

[codecov]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (7911f1e) 97.27% compared to head (b9d776d) 96.93%.
Report is 478 commits behind head on master.

Files	Patch %	Lines
tests/test_connector.py	76.92%	8 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7368      +/-   ##
==========================================
- Coverage   97.27%   96.93%   -0.35%     
==========================================
  Files         106      106              
  Lines       31402    31441      +39     
  Branches     3925     3930       +5     
==========================================
- Hits        30547    30478      -69     
- Misses        650      751     +101     
- Partials      205      212       +7     

Flag	Coverage Δ
CI-GHA	96.90% <78.04%> (-0.33%)	⬇️
OS-Linux	96.87% <78.04%> (-0.02%)	⬇️
OS-Windows	?
OS-macOS	96.55% <78.04%> (-0.04%)	⬇️
Py-3.10.11	?
Py-3.10.12	96.76% <78.04%> (-0.04%)	⬇️
Py-3.11.0	96.51% <78.04%> (-0.02%)	⬇️
Py-3.8.10	?
Py-3.8.17	96.69% <78.04%> (-0.03%)	⬇️
Py-3.9.13	?
Py-3.9.17	96.72% <78.04%> (-0.03%)	⬇️
Py-pypy7.3.11	94.20% <78.04%> (-0.03%)	⬇️
VM-macos	96.55% <78.04%> (-0.04%)	⬇️
VM-ubuntu	96.87% <78.04%> (-0.02%)	⬇️
VM-windows	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[webknjaz]

@brettdh could you rebase the branch resolving the merge conflicts, to see if it'd pass? I think there were some fixes to the proxy tests last year.

[Dreamsorcerer]

@brettdh could you rebase the branch resolving the merge conflicts, to see if it'd pass? I think there were some fixes to the proxy tests last year.

Still failing. I'm not sure it's related to the proxy anyway, I think this is just regular connections.