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

Replace tcp_sockopts with socket_factory #10534

Merged
merged 2 commits into from
Mar 15, 2025

Conversation

TimMenninger
Copy link
Contributor

@TimMenninger TimMenninger commented Mar 10, 2025

Instead of TCPConnector taking a list of sockopts to be applied sockets created, take a socket_factory callback that allows the caller to implement socket creation entirely.

Fixes #10520

What do these changes do?

Replace tcp_sockopts parameter with a socket_factory parameter that is a callback allowing the caller to own socket creation. If passed, all sockets created by TCPConnector are expected to come from the socket_factory callback.

Are there changes in behavior for the user?

The only users to experience a change in behavior are those who are using the un-released tcp_sockopts argument to TCPConnector. However, using unreleased code comes with caveat emptor, and is why I felt entitled to remove the option entirely without warning.

Is it a substantial burden for the maintainers to support this?

The burden will be minimal and would only arise if aiohappyeyeballs changes their interface.

Related issue number

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_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 an
        improper 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 breaking
        changes 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 build
        process.
      • .packaging: Notes for downstreams about unobvious side effects
        and 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 above
        categories.
    • Make sure to use full sentences with correct case and punctuation,
      for example:

      Fixed issue with non-ascii contents in doctest text files
      -- by :user:`contributor-gh-handle`.

      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.

Sorry, something went wrong.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Mar 10, 2025
Copy link

codspeed-hq bot commented Mar 10, 2025

CodSpeed Performance Report

Merging #10534 will not alter performance

Comparing TimMenninger:tcp-socket-factory (ea62f2f) with master (e507ba3)

Summary

✅ 46 untouched benchmarks

@TimMenninger TimMenninger force-pushed the tcp-socket-factory branch 6 times, most recently from 212bb6f to d9a7943 Compare March 10, 2025 17:58
Copy link

codecov bot commented Mar 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.69%. Comparing base (45b861f) to head (ea62f2f).
Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #10534   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files         122      122           
  Lines       37230    37235    +5     
  Branches     2064     2062    -2     
=======================================
+ Hits        36745    36750    +5     
  Misses        338      338           
  Partials      147      147           
Flag Coverage Δ
CI-GHA 98.57% <100.00%> (+<0.01%) ⬆️
OS-Linux 98.24% <100.00%> (+<0.01%) ⬆️
OS-Windows 96.18% <100.00%> (+<0.01%) ⬆️
OS-macOS 97.35% <100.00%> (-0.01%) ⬇️
Py-3.10.11 97.27% <100.00%> (+<0.01%) ⬆️
Py-3.10.16 97.81% <100.00%> (+<0.01%) ⬆️
Py-3.11.11 97.89% <100.00%> (+<0.01%) ⬆️
Py-3.11.9 97.35% <100.00%> (+<0.01%) ⬆️
Py-3.12.9 98.35% <100.00%> (+<0.01%) ⬆️
Py-3.13.2 98.34% <100.00%> (+<0.01%) ⬆️
Py-3.9.13 97.14% <100.00%> (-0.02%) ⬇️
Py-3.9.21 97.68% <100.00%> (+<0.01%) ⬆️
Py-pypy7.3.16 85.92% <100.00%> (-3.32%) ⬇️
VM-macos 97.35% <100.00%> (-0.01%) ⬇️
VM-ubuntu 98.24% <100.00%> (+<0.01%) ⬆️
VM-windows 96.18% <100.00%> (+<0.01%) ⬆️

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.

@kjander0
Copy link

Do you need to uprev the required version of aiohappyeyeballs in setup.cfg?

@TimMenninger
Copy link
Contributor Author

TimMenninger commented Mar 11, 2025

Do you need to uprev the required version of aiohappyeyeballs in setup.cfg?

I think that depends on the resolution to #10534 (comment) but if only for the documentation, yeah it'd probably have to go to whatever version if/when aio-libs/aiohappyeyeballs#149 merges and released.

EDIT: Nevermind, I think as long as the latest docs are complete, which they seem to be (using https://aiohappyeyeballs--149.org.readthedocs.build/en/149/ for aiohappyeyeballs worked) the version should be fine, unless you see something I didn't.

@Dreamsorcerer
Copy link
Member

I think that depends on the resolution to #10534 (comment) but if only for the documentation, yeah it'd probably have to go to whatever version if/when aio-libs/aiohappyeyeballs#149 merges and released.

We're passing a kwarg that didn't exist on older releases, won't that break on older releases of aiohappyeyeballs?

@TimMenninger
Copy link
Contributor Author

We're passing a kwarg that didn't exist on older releases, won't that break on older releases of aiohappyeyeballs?

oh! good catch @kjander0 and @Dreamsorcerer, I didn't realize how new that feature is. will bump to 2.5.0 when socket_factory comes about

@TimMenninger TimMenninger force-pushed the tcp-socket-factory branch 2 times, most recently from 3a87c34 to 3adf43a Compare March 11, 2025 16:46
@TimMenninger
Copy link
Contributor Author

TimMenninger commented Mar 11, 2025

I'm at a total loss on how to fix this

/Users/tmenninger/Downloads/aiohttp/docs/client_reference.rst:1127: WARNING: py:class reference target not found: socket.SocketKind [ref.class]
/Users/tmenninger/Downloads/aiohttp/docs/client_reference.rst:1132: WARNING: py:class reference target not found: socket.SocketKind [ref.class]

Any ideas? It's complaining about these lines...

1127:   Refer to :py:data:`aiohappyeyeballs.AddrInfoType`
...
1132:   Refer to :py:data:`aiohappyeyeballs.SocketFactoryType`

EDIT: Found a workaround, sorry I'm new to this

@TimMenninger TimMenninger force-pushed the tcp-socket-factory branch 3 times, most recently from cdaf8ba to 09ece48 Compare March 11, 2025 22:29
@TimMenninger TimMenninger marked this pull request as ready for review March 11, 2025 22:34
@bdraco bdraco added the backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot label Mar 12, 2025
Instead of TCPConnector taking a list of sockopts to be applied sockets
created, take a socket_factory callback that allows the caller to
implement socket creation entirely.
@bdraco
Copy link
Member

bdraco commented Mar 12, 2025

Thanks @TimMenninger

@bdraco bdraco changed the title Replace tcp_sockopts with socket_factory (#10520) Replace tcp_sockopts with socket_factory Mar 15, 2025

Verified

This commit was signed with the committer’s verified signature.
bdraco J. Nick Koston
@bdraco bdraco merged commit 3b9bb1c into aio-libs:master Mar 15, 2025
40 checks passed
Copy link
Contributor

patchback bot commented Mar 15, 2025

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

❌ Failed to cleanly apply 3b9bb1c on top of patchback/backports/3.12/3b9bb1cd5677a8c8443d16184ed36856ae105cd7/pr-10534

Backporting merged PR #10534 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.12/3b9bb1cd5677a8c8443d16184ed36856ae105cd7/pr-10534 upstream/3.12
  4. Now, cherry-pick PR Replace tcp_sockopts with socket_factory #10534 contents into that branch:
    $ git cherry-pick -x 3b9bb1cd5677a8c8443d16184ed36856ae105cd7
    If it'll yell at you with something like fatal: Commit 3b9bb1cd5677a8c8443d16184ed36856ae105cd7 is a merge but no -m option was given., add -m 1 as follows instead:
    $ git cherry-pick -m1 -x 3b9bb1cd5677a8c8443d16184ed36856ae105cd7
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Replace tcp_sockopts with socket_factory #10534 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/3.12/3b9bb1cd5677a8c8443d16184ed36856ae105cd7/pr-10534
  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.

@bdraco
Copy link
Member

bdraco commented Mar 15, 2025

@TimMenninger Thanks for this PR. Can you take care of back-porting this PR to 3.12 using the instructions above?

@TimMenninger
Copy link
Contributor Author

@TimMenninger Thanks for this PR. Can you take care of back-porting this PR to 3.12 using the instructions above?

Yep will do. It might be till tomorrow or Monday morning that I get to it.

TimMenninger added a commit to TimMenninger/aiohttp that referenced this pull request Mar 16, 2025
@bdraco
Copy link
Member

bdraco commented Mar 16, 2025

Since I'm planning on doing a release. I'll get a clean backport done and than we can merge your docs fixes on top.

bdraco pushed a commit that referenced this pull request Mar 16, 2025

Verified

This commit was signed with the committer’s verified signature.
bdraco J. Nick Koston
Instead of TCPConnector taking a list of sockopts to be applied sockets
created, take a socket_factory callback that allows the caller to
implement socket creation entirely.

Fixes #10520

<!-- Thank you for your contribution! -->

Replace `tcp_sockopts` parameter with a `socket_factory` parameter that
is a callback allowing the caller to own socket creation. If passed, all
sockets created by `TCPConnector` are expected to come from the
`socket_factory` callback.

<!-- Please give a short brief about these changes. -->

The only users to experience a change in behavior are those who are
using the un-released `tcp_sockopts` argument to `TCPConnector`.
However, using unreleased code comes with caveat emptor, and is why I
felt entitled to remove the option entirely without warning.

<!-- Outline any notable behaviour for the end users. -->

The burden will be minimal and would only arise if `aiohappyeyeballs`
changes their interface.

<!--
Stop right there! Pause. Just for a minute... Can you think of anything
obvious that would complicate the ongoing development of this project?

Try to consider if you'd be able to maintain it throughout the next
5 years. Does it seem viable? Tell us your thoughts! We'd very much
love to hear what the consequences of merging this patch might be...

This will help us assess if your change is something we'd want to
entertain early in the review process. Thank you in advance!
-->

<!-- Are there any issues opened that will be resolved by merging this
change? -->
<!-- Remember to prefix with 'Fixes' if it should close the issue (e.g.
'Fixes #123'). -->

- [x] I think the code is well written
- [x] 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 &lt;Name&gt; &lt;Surname&gt;.
  * Please keep alphabetical order, the file is sorted by names.
- [x] Add a new news fragment into the `CHANGES/` folder
  * name 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 an
      improper 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 breaking
      changes 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 build
      process.
    * `.packaging`: Notes for downstreams about unobvious side effects
      and 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 above
      categories.
  * Make sure to use full sentences with correct case and punctuation,
    for example:
    ```rst
    Fixed issue with non-ascii contents in doctest text files
    -- by :user:`contributor-gh-handle`.
    ```

    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.

---------

Co-authored-by: J. Nick Koston <nick@koston.org>
(cherry picked from commit 3b9bb1c)
bdraco added a commit that referenced this pull request Mar 16, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…factory (#10574)

replaces and closes #10565

Instead of TCPConnector taking a list of sockopts to be applied sockets
created, take a socket_factory callback that allows the caller to
implement socket creation entirely.

Fixes #10520

<!-- Thank you for your contribution! -->

Replace `tcp_sockopts` parameter with a `socket_factory` parameter that
is a callback allowing the caller to own socket creation. If passed, all
sockets created by `TCPConnector` are expected to come from the
`socket_factory` callback.

<!-- Please give a short brief about these changes. -->

The only users to experience a change in behavior are those who are
using the un-released `tcp_sockopts` argument to `TCPConnector`.
However, using unreleased code comes with caveat emptor, and is why I
felt entitled to remove the option entirely without warning.

<!-- Outline any notable behaviour for the end users. -->

The burden will be minimal and would only arise if `aiohappyeyeballs`
changes their interface.

<!--
Stop right there! Pause. Just for a minute... Can you think of anything
obvious that would complicate the ongoing development of this project?

Try to consider if you'd be able to maintain it throughout the next 5
years. Does it seem viable? Tell us your thoughts! We'd very much love
to hear what the consequences of merging this patch might be...

This will help us assess if your change is something we'd want to
entertain early in the review process. Thank you in advance! -->

<!-- Are there any issues opened that will be resolved by merging this
change? -->
<!-- Remember to prefix with 'Fixes' if it should close the issue (e.g.
'Fixes #123'). -->

- [x] I think the code is well written
- [x] 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 &lt;Name&gt; &lt;Surname&gt;.
  * Please keep alphabetical order, the file is sorted by names.
- [x] Add a new news fragment into the `CHANGES/` folder
  * name 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 an improper
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 breaking
changes 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 build
process.
* `.packaging`: Notes for downstreams about unobvious side effects and
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 above
categories.
* Make sure to use full sentences with correct case and punctuation, for
example: ```rst Fixed issue with non-ascii contents in doctest text
files -- by :user:`contributor-gh-handle`. ```

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.

---------

Co-authored-by: J. Nick Koston <nick@koston.org>
(cherry picked from commit 3b9bb1c)

<!-- Thank you for your contribution! -->

## What do these changes do?

<!-- Please give a short brief about these changes. -->

## Are there changes in behavior for the user?

<!-- Outline any notable behaviour for the end users. -->

## Is it a substantial burden for the maintainers to support this?

<!--
Stop right there! Pause. Just for a minute... Can you think of anything
obvious that would complicate the ongoing development of this project?

Try to consider if you'd be able to maintain it throughout the next
5 years. Does it seem viable? Tell us your thoughts! We'd very much
love to hear what the consequences of merging this patch might be...

This will help us assess if your change is something we'd want to
entertain early in the review process. Thank you in advance!
-->

## Related issue number

<!-- Are there any issues opened that will be resolved by merging this
change? -->
<!-- Remember to prefix with 'Fixes' if it should close the issue (e.g.
'Fixes #123'). -->

## 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 &lt;Name&gt; &lt;Surname&gt;.
  * Please keep alphabetical order, the file is sorted by names.
- [ ] Add a new news fragment into the `CHANGES/` folder
  * name 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 an
      improper 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 breaking
      changes 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 build
      process.
    * `.packaging`: Notes for downstreams about unobvious side effects
      and 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 above
      categories.
  * Make sure to use full sentences with correct case and punctuation,
    for example:
    ```rst
    Fixed issue with non-ascii contents in doctest text files
    -- by :user:`contributor-gh-handle`.
    ```

    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.

Co-authored-by: Tim Menninger <tmenninger22@gmail.com>
TimMenninger added a commit to TimMenninger/aiohttp that referenced this pull request Mar 19, 2025
TimMenninger added a commit to TimMenninger/aiohttp that referenced this pull request Mar 19, 2025
TimMenninger added a commit to TimMenninger/aiohttp that referenced this pull request Mar 21, 2025
TimMenninger added a commit to TimMenninger/aiohttp that referenced this pull request Mar 22, 2025
bdraco pushed a commit that referenced this pull request Mar 30, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
patchback bot pushed a commit that referenced this pull request Mar 30, 2025
(cherry picked from commit 8ac4830)
bdraco pushed a commit that referenced this pull request Mar 30, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…ocket factory (#10534) (#10637)

**This is a backport of PR #10568 as merged into master
(8ac4830).**

Co-authored-by: Tim Menninger <tmenninger22@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot 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.

Setting client TCP socket options before connection
4 participants