Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement happy eyeballs (RFC 8305) #7954

Merged
merged 58 commits into from
Jan 5, 2024
Merged

Implement happy eyeballs (RFC 8305) #7954

merged 58 commits into from
Jan 5, 2024

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Dec 9, 2023

What do these changes do?

Implements Happy Eyeballs support (https://datatracker.ietf.org/doc/html/rfc8305)

Are there changes in behavior for the user?

Dual stack connections where IPv6 is not working should now be successful.

Related issue number

fixes #4451

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • 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."

--

TODO

  • docs (params happy_eyeballs_delay and interleave function the same as cpython but we should point that out, and how to disable happy_eyeballs_delay by setting it to None)
  • more production and functional testing
  • tests for ipv4 fallback
  • tests for interleave
  • tests for family
  • timeline

This PR is a first step as I need to make sure this library works for the other projects as well. Its likely to sit for a bit while I test the library with all the projects that need it so there aren't any need to do breaking changes before all the other projects start consuming it:

--

aiohttp/connector.py Outdated Show resolved Hide resolved
aiohttp/connector.py Outdated Show resolved Hide resolved
aiohttp/connector.py Fixed Show fixed Hide fixed
@bdraco
Copy link
Member Author

bdraco commented Dec 10, 2023

functional tests seem to be ok

Anything that does this will fail since aiohappyeyeballs.start_connection isn't mocked

        self.loop.create_connection = make_mocked_coro((mock.Mock(), mock.Mock()))

aiohttp/connector.py Fixed Show fixed Hide fixed
aiohttp/connector.py Outdated Show resolved Hide resolved
requirements/constraints.txt Outdated Show resolved Hide resolved
requirements/dev.txt Outdated Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Dec 11, 2023

I'm still doing testing with this, and I think its ready to move to wider production testing.

I'm going to spend a few cycles on test_unsupported_upgrade in #7960 but its a bit difficult without a windows box

@bdraco
Copy link
Member Author

bdraco commented Dec 12, 2023

So far so good on production 3.9+backported.

Doing performance testing now

@bdraco
Copy link
Member Author

bdraco commented Dec 12, 2023

performance looks good. All the time is nearly loop.sock_connect so should have no significant change

Screenshot 2023-12-11 at 7 29 51 PM Screenshot 2023-12-11 at 7 31 03 PM

_ensure_fd_no_transport has a KeyError in the success path so it slows down establishing the connection but thats an existing problem similar to python/cpython#106664

submitted python/cpython#112989 and will PR cpython. This is not a new problem

@bdraco
Copy link
Member Author

bdraco commented Dec 12, 2023

Performance and production testing looks good. All the run time is in loop.sock_connect as before
start_connection

I've got reviews on the other use cases so I think this is good to go for a final review.

I want to sleep on it and look it all over in the morning end to end again though before I mark it ready

@bdraco
Copy link
Member Author

bdraco commented Dec 12, 2023

Still testing out great this morning

@bdraco bdraco marked this pull request as ready for review December 12, 2023 16:54
@bdraco bdraco changed the title Implement happy eyeballs Implement happy eyeballs (RFC 8305) Dec 12, 2023
@bdraco
Copy link
Member Author

bdraco commented Dec 16, 2023

I think this is good to merge at this point, but I'm slow rolling it because ESPHome is currently in beta which also uses the aiohappyeyeballs lib so no rush in merging as I'm happy to wait for the beta feedback in case anything has to change with the lib, but it now seems unlikely at this point.

@bdraco
Copy link
Member Author

bdraco commented Dec 20, 2023

ESPHome is releasing today with the new lib, HA is doing beta next week with it

@bdraco
Copy link
Member Author

bdraco commented Dec 21, 2023

This change in esphome release seems to be going swimmingly. Next week we get beta feedback on it in aiohomekit from Home Assistant

@bdraco
Copy link
Member Author

bdraco commented Dec 29, 2023

The changes in aiohomekit seem to be going well in the HA beta.

@bdraco
Copy link
Member Author

bdraco commented Jan 2, 2024

Ha beta has been without issues in this area. The full release happens tomorrow. As long as everything goes smoothly, I'll merge this shortly after

@bdraco
Copy link
Member Author

bdraco commented Jan 4, 2024

HA release shipped today. No issues so far. All looking good

@bdraco
Copy link
Member Author

bdraco commented Jan 5, 2024

Everything seems solid with the HA release related to this.

That's the last hurdle.

@bdraco bdraco merged commit c4ec3f1 into master Jan 5, 2024
31 of 34 checks passed
@bdraco bdraco deleted the happy_eye_balls branch January 5, 2024 00:40
Copy link
Contributor

patchback bot commented Jan 5, 2024

Backport to 3.10: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply c4ec3f1 on top of patchback/backports/3.10/c4ec3f130af3a53040b883aa2fab06fd215f9088/pr-7954

Backporting merged PR #7954 into master

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. Make sure you have an upstream repo added as a remote too. In these
    instructions you'll refer to it by the name upstream. If you don't
    have it, here's how you can add it:
    $ git remote add upstream https://github.com/aio-libs/aiohttp.git
  3. Ensure you have the latest copy of upstream and prepare a branch
    that will hold the backported code:
    $ git fetch upstream
    $ git checkout -b patchback/backports/3.10/c4ec3f130af3a53040b883aa2fab06fd215f9088/pr-7954 upstream/3.10
  4. Now, cherry-pick PR Implement happy eyeballs (RFC 8305) #7954 contents into that branch:
    $ git cherry-pick -x c4ec3f130af3a53040b883aa2fab06fd215f9088
    If it'll yell at you with something like fatal: Commit c4ec3f130af3a53040b883aa2fab06fd215f9088 is a merge but no -m option was given., add -m 1 as follows instead:
    $ git cherry-pick -m1 -x c4ec3f130af3a53040b883aa2fab06fd215f9088
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Implement happy eyeballs (RFC 8305) #7954 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/3.10/c4ec3f130af3a53040b883aa2fab06fd215f9088/pr-7954
  7. Create a PR, ensure that the CI is green. If it's not — update it so that
    the tests and any other checks pass. This is it!
    Now relax and wait for the maintainers to process your pull request
    when they have some cycles to do reviews. Don't worry — they'll tell you if
    any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Happy Eyeballs from Python 3.8
2 participants