Binding to network interfaces

[Muno459]

What do these changes do?

Finally added support to bind directly to network interfaces.

Are there changes in behavior for the user?

No, a optional keyword argument has been added. This will allow end-users to specify the network interface, a feature many related projects lacks without monkey-patching.

Related issue number

#6383

Checklist

• [X] I think the code is well written
• [ ] Unit tests for the changes exist
• [X] Documentation reflects the changes
• [X] 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.
• [x] 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."

[codecov[bot]]

Codecov Report

Merging #7132 (2e50c16) into master (d51013d) will decrease coverage by 0.43%. The diff coverage is 27.77%.

:exclamation: Current head 2e50c16 differs from pull request most recent head 5fb6f4b. Consider uploading reports for the commit 5fb6f4b to get more accurate results

@@            Coverage Diff             @@
##           master    #7132      +/-   ##
==========================================
- Coverage   97.38%   96.95%   -0.44%     
==========================================
  Files         106      106              
  Lines       31093    31110      +17     
  Branches     3875     3878       +3     
==========================================
- Hits        30280    30162     -118     
- Misses        612      735     +123     
- Partials      201      213      +12     

Flag	Coverage Δ
CI-GHA	96.87% <27.77%> (-0.41%)	:arrow_down:
OS-Linux	96.84% <27.77%> (-0.10%)	:arrow_down:
OS-Windows	?
OS-macOS	96.46% <27.77%> (-0.05%)	:arrow_down:
Py-3.10.8	?
Py-3.10.9	96.55% <27.77%> (-0.48%)	:arrow_down:
Py-3.11.0	95.55% <27.77%> (-0.05%)	:arrow_down:
Py-3.7.15	96.71% <27.77%> (-0.05%)	:arrow_down:
Py-3.7.9	?
Py-3.8.10	?
Py-3.8.15	96.61% <27.77%> (-0.04%)	:arrow_down:
Py-3.9.13	?
Py-3.9.14	?
Py-3.9.15	?
Py-3.9.16	96.62% <27.77%> (-0.04%)	:arrow_down:
Py-pypy7.3.10	?
Py-pypy7.3.9	?
VM-macos	96.46% <27.77%> (-0.05%)	:arrow_down:
VM-ubuntu	96.84% <27.77%> (-0.10%)	:arrow_down:
VM-windows	?

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

Impacted Files	Coverage Δ
aiohttp/connector.py	91.88% <22.22%> (-2.74%)	:arrow_down:
aiohttp/helpers.py	94.37% <33.33%> (-1.02%)	:arrow_down:
tests/autobahn/client/client.py	46.42% <0.00%> (-50.00%)	:arrow_down:
tests/conftest.py	86.84% <0.00%> (-9.65%)	:arrow_down:
aiohttp/__init__.py	85.71% <0.00%> (-7.15%)	:arrow_down:
aiohttp/web_runner.py	92.37% <0.00%> (-5.39%)	:arrow_down:
aiohttp/tcp_helpers.py	94.73% <0.00%> (-5.27%)	:arrow_down:
tests/test_web_runner.py	93.78% <0.00%> (-5.09%)	:arrow_down:
tests/autobahn/test_autobahn.py	95.00% <0.00%> (-5.00%)	:arrow_down:
aiohttp/pytest_plugin.py	94.33% <0.00%> (-3.15%)	:arrow_down:
... and 9 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[Dreamsorcerer]

Looks good to me, though I'm not too familiar with it. @webknjaz Want to double check it?

[webknjaz]

In general, this seems mostly acceptable, except for the comments I've made and missing tests. Also, the CI must pass before it's mergeable, including linting.

[Muno459]

I looked through your comments, and will create a new commit. In regards to tests, I'm not sure how I would implement that as it's highly specific for each system. In regards to the CI check failing, the check is failing from other files that hasn't been changed in this PR.

[Muno459]

I have futhermore fixed the socket family to be used from the request and not be fixed to AF_INET which should add support for IPv6, futhermore I removed the connect_ex call and replaced it with async loop.sock_connect. I'm not sure how it worked before, but i tested the socket.connect_ex function just for fun on macOS, and it was throwing errors that the socket wasn't connected unless i set it blocking, but on my Ubuntu machine it was working fine. Anyways, after awaiting, it worked out just fine.

[Muno459]

I noticed my latest commit has introduced a linting issue, however i'm unsure what caused it. The typing used for local_addr is the same one defined in the built-in library socket.

[webknjaz]

i'm unsure what caused it.

You used lowercase tuple instead of the imported capitalized version. That would only work on Python 3.10+ IIRC. To make it work the same in the earlier versions, you could add from __future__ import annotations and it should satisfy the linter because mypy runs in a separate runtime but the future import will turn the annotations into regular strings making the regular runtime ignore what's inside.

[webknjaz]

Also, there are typing problems present on master, they need to be addressed separately so they don't pollute the checks in this PR with confusing unrelated things.

[Dreamsorcerer]

you could add from __future__ import annotations and it should satisfy the linter because mypy runs in a separate runtime but the future import will turn the annotations into regular strings making the regular runtime ignore what's inside.

My understanding is that this merely delays evaluation of annotations. If someone wants the type hints at runtime (e.g. with get_type_hints()) then you'd still hit an error at that point. So, I think it's best we stick to Tuple (pyupgrade will presumably replace all instances once we drop support for 3.8).

[Muno459]

I made it uppercase now, but it's still causing a linting issue.

[webknjaz]

My understanding is that this merely delays evaluation of annotations.

Not exactly. What's happening here is a combination of factors. So let me try to explain various aspects a bit deeper.

1. Type annotations can be specified in two ways: var1: type1 — the AST node for type1 is an object that must be previously imported and available at runtime var1: 'type1' — the AST node for 'type1' is an 'str' literal For the second case, it won't get eval()ed at import time like the first one. Both cases will produce metadata available in runtime via the __annotations__ variable/attribute in the local scope.

   $ ipython3                                                                        
   Python 3.9.0 (default, Dec  9 2022, 20:45:53) 
   Type 'copyright', 'credits' or 'license' for more information
   IPython 7.19.0 -- An enhanced Interactive Python. Type '?' for help.
   
   In [1]: var: asdf
   ---------------------------------------------------------------------------
   NameError                                 Traceback (most recent call last)
   <ipython-input-1-71b1d907417d> in <module>
   ----> 1 var: asdf
   
   NameError: name 'asdf' is not defined
   
   In [2]: __annotations__
   Out[2]: {}
   
   In [3]: var: 'asdf'
   
   In [4]: __annotations__
   Out[4]: {'var': 'asdf'}
   
   In [5]: var: str
   
   In [6]: __annotations__
   Out[6]: {'var': str}
   

   So, as we see, what you type in ends up in __annotations__ exactly as you specify it. Thanks to tuple having the __slice__() method in Python 3.10+, you can use tuple[thing] in that runtime. In older runtimes, only typing.Tuple has it.
2. Now, there's PEP 563 that was supposed to become the default behavior in Python 3.10 but got a strong pushback from the Pydantic community very last-minute, about 3 weeks before the release. Turned out that even though it was planned like 4 years before that, many folks didn't realize the impact. It created a lot of noise on Twitter and GitHub, the SC had to get involved and ended up reverting the change: https://github.com/pydantic/pydantic/issues/2678#issuecomment-823569522 / https://mail.python.org/archives/list/[email protected]/thread/CLVXXPQ2T2LQ5MP2Y53VVQFCXYWQJHKZ/. PEP 649 which is currently a draft, was created to try to mitigate the concerns raised: https://dev.to/tiangolo/the-future-of-fastapi-and-pydantic-is-bright-3pbm#pep-649-deferred-evaluation-of-annotations-using-descriptors. What was it about? This feature, that is currently only enabled via from __future__ import annotations, makes it so that the parser turns anything you type in as an annotation into strings — that var: asdf example above would become an equivalent of var: 'asdf'. This is exactly what's happening with my proposal to add an import — it basically stops the interpreter from looking up any references you may have in your annotations, since they are all just strings in runtime now.
3. With that in mind, my 'tuple[thing]' does not become tuple[thing] in runtime and therefore, it won't cause any problems with linting and testing for us.
4. When typing.TYPE_CHECKING is True, though, it's a different story. MyPy does evaluate those strings to verify that the referenced objects exist. But it also includes a couple of shims that make it able to evaluate “syntax” from newer Pythons which is how tuple[thing] would work in MyPy regardless of the target Python version.

[webknjaz]

If someone wants the type hints at runtime (e.g. with get_type_hints()) then you'd still hit an error at that point.

You're right, that's a valid concern.

[webknjaz]

@Mostafa-Mahdi since you mentioned being a novice, here's a curated collection of materials related to FOSS contributions and Python you may like:

• https://gist.github.com/webknjaz/b623cb5eb941ed17617afa831f4a7fb0#idiomatic-python
• https://gist.github.com/webknjaz/a5a4fb374b7579de827e6bedb93a5220
• https://gist.github.com/webknjaz/cb7d7bf62c3dda4b1342d639d0e78d79
• https://gist.github.com/webknjaz/a7362787a80067af8621a85a71746ca1
• https://gist.github.com/webknjaz/2aa9c7a43b51c1385260ff87e0cbb9e3

(these are not specific to our project and can be applied pretty much universally)

[Muno459]

@Mostafa-Mahdi since you mentioned being a novice, here's a curated collection of materials related to FOSS contributions and Python you may like:

• https://gist.github.com/webknjaz/b623cb5eb941ed17617afa831f4a7fb0#idiomatic-python
• https://gist.github.com/webknjaz/a5a4fb374b7579de827e6bedb93a5220
• https://gist.github.com/webknjaz/cb7d7bf62c3dda4b1342d639d0e78d79
• https://gist.github.com/webknjaz/a7362787a80067af8621a85a71746ca1
• https://gist.github.com/webknjaz/2aa9c7a43b51c1385260ff87e0cbb9e3

(these are not specific to our project and can be applied pretty much universally)

I appreciate you taking the time to link these. I will definitely read up on these.

[Muno459]

I believe I have addressed all the issues, futhermore found a fix to the tuple assignment without using type ignore :)

[Muno459]

Changed it to static

[Dreamsorcerer]

To fix the typing error, I'd suggest renaming the first host variable to raw_host:

        raw_host = req.url.raw_host
        assert raw_host is not None
        port = req.port
        assert port is not None
        host_resolved = asyncio.ensure_future(
            self._resolve_host(raw_host, port, traces=traces), loop=self._loop
        )

https://github.com/aio-libs/aiohttp/blob/f0073afe3cc66f8cbc951522d3cacf46523bd818/aiohttp/connector.py#L1112-L1118

[bertybuttface]

Sorry to bump an old PR but is this stale / is there another way to select a particular network interface when using aiohttp.ClientSession?

[Dreamsorcerer]

Looks like the PR didn't get finished off. Feel free to fork it and create a new PR with the final changes.

[Muno459]

Hey @Dreamsorcerer Exactly, what is needed to finish this PR?

[Dreamsorcerer]

The tests are failing, there are conflicts, and there are unresolved conversations above.

[sheldygg]

When this PR will be merged?

[webknjaz]

When somebody addresses the requests above.