Replace tcp_sockopts with socket_factory (#10520) #10534

TimMenninger · 2025-03-10T15:45:21Z

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 issue #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.

codspeed-hq · 2025-03-10T15:51:43Z

CodSpeed Performance Report

Merging #10534 will not alter performance

_{Comparing TimMenninger:tcp-socket-factory (09ece48) with master (4399a6c)}

Summary

✅ 46 untouched benchmarks

codecov · 2025-03-10T19:28:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@4399a6c). Learn more about missing BASE report.

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #10534   +/-   ##
=========================================
  Coverage          ?   98.69%           
=========================================
  Files             ?      122           
  Lines             ?    37235           
  Branches          ?     2062           
=========================================
  Hits              ?    36750           
  Misses            ?      338           
  Partials          ?      147

Flag	Coverage Δ
CI-GHA	`98.57% <100.00%> (?)`
OS-Linux	`98.24% <100.00%> (?)`
OS-Windows	`96.18% <100.00%> (?)`
OS-macOS	`97.36% <100.00%> (?)`
Py-3.10.11	`97.26% <100.00%> (?)`
Py-3.10.16	`97.81% <100.00%> (?)`
Py-3.11.11	`97.89% <100.00%> (?)`
Py-3.11.9	`97.35% <100.00%> (?)`
Py-3.12.9	`98.35% <100.00%> (?)`
Py-3.13.2	`98.34% <100.00%> (?)`
Py-3.9.13	`97.15% <100.00%> (?)`
Py-3.9.21	`97.68% <100.00%> (?)`
Py-pypy7.3.16	`87.47% <100.00%> (?)`
VM-macos	`97.36% <100.00%> (?)`
VM-ubuntu	`98.24% <100.00%> (?)`
VM-windows	`96.18% <100.00%> (?)`

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.

CHANGES/10520.feature.rst

bdraco · 2025-03-11T00:34:44Z

aiohttp/connector.py

@@ -826,8 +826,9 @@ class TCPConnector(BaseConnector):
                           the happy eyeballs algorithm, set to None.
    interleave - “First Address Family Count” as defined in RFC 8305
    loop - Optional event loop.
-    tcp_sockopts - List of tuples of sockopts applied to underlying
-                   socket
+    socket_factory - An aiohappyeyeballs.SocketFactoryType function


While aiohappyeyeballs is an aio-libs library, it might make sense to document this as an aiohttp type instead since aiohappyeyeballs is an implementation detail of happy eyeballs support.

Right, hopefully aiohappyeyeballs will be upstreamed to cpython in future, in which case we'd stop using it here.

I'm not sure I understand the suggestion. Are you thinking I should alias (or redefine) it in aiohttp and document it here?

I notice the timing of these comments is before your first activity in aio-libs/aiohappyeyeballs#149, would it be plausible to let aiohappyeyeballs run its course and keep aiohttp as-is once the docs are updated?

It's not about documentation, but backwards compatibility. I think an alias in aiohttp is probably fine, and then if we drop aiohappyeyeballs in a future release the type can be copied into the codebase. This would ensure users don't need to depend on aiohappyeyeballs directly at any point.

kjander0 · 2025-03-11T05:09:40Z

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

TimMenninger · 2025-03-11T14:00:20Z

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 · 2025-03-11T14:28:37Z

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 · 2025-03-11T15:15:15Z

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 · 2025-03-11T17:07:56Z

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

docs/conf.py

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.

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

TimMenninger force-pushed the tcp-socket-factory branch from e07d4e8 to dd9f264 Compare March 10, 2025 15:51

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

Dreamsorcerer reviewed Mar 11, 2025

View reviewed changes

CHANGES/10520.feature.rst Outdated Show resolved Hide resolved

Dreamsorcerer reviewed Mar 11, 2025

View reviewed changes

CHANGES/10520.feature.rst Show resolved Hide resolved

Dreamsorcerer approved these changes Mar 11, 2025

View reviewed changes

bdraco reviewed Mar 11, 2025

View reviewed changes

TimMenninger force-pushed the tcp-socket-factory branch from d9a7943 to 2e147ff Compare March 11, 2025 13:57

TimMenninger force-pushed the tcp-socket-factory branch from 1b467a3 to eb17499 Compare March 11, 2025 14:11

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

TimMenninger commented Mar 11, 2025

View reviewed changes

docs/conf.py Outdated Show resolved Hide resolved

TimMenninger force-pushed the tcp-socket-factory branch 2 times, most recently from 56fc83d to 209d036 Compare March 11, 2025 22:28

Replace tcp_sockopts with socket_factory (aio-libs#10520)

09ece48

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.

TimMenninger force-pushed the tcp-socket-factory branch from cdaf8ba to 09ece48 Compare March 11, 2025 22:29

TimMenninger marked this pull request as ready for review March 11, 2025 22:34

TimMenninger requested a review from webknjaz as a code owner March 11, 2025 22:34

TimMenninger requested a review from asvetlov as a code owner March 11, 2025 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace tcp_sockopts with socket_factory (#10520) #10534

Replace tcp_sockopts with socket_factory (#10520) #10534

TimMenninger commented Mar 10, 2025 •

edited

Loading

codspeed-hq bot commented Mar 10, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading

bdraco Mar 11, 2025

Dreamsorcerer Mar 11, 2025

TimMenninger Mar 11, 2025

Dreamsorcerer Mar 11, 2025

kjander0 commented Mar 11, 2025

TimMenninger commented Mar 11, 2025 •

edited

Loading

Dreamsorcerer commented Mar 11, 2025

TimMenninger commented Mar 11, 2025

TimMenninger commented Mar 11, 2025 •

edited

Loading

Replace tcp_sockopts with socket_factory (#10520) #10534

Are you sure you want to change the base?

Replace tcp_sockopts with socket_factory (#10520) #10534

Conversation

TimMenninger commented Mar 10, 2025 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

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

Related issue number

Checklist

codspeed-hq bot commented Mar 10, 2025 • edited Loading

CodSpeed Performance Report

Merging #10534 will not alter performance

Summary

codecov bot commented Mar 10, 2025 • edited Loading

Codecov Report

bdraco Mar 11, 2025

Choose a reason for hiding this comment

Dreamsorcerer Mar 11, 2025

Choose a reason for hiding this comment

TimMenninger Mar 11, 2025

Choose a reason for hiding this comment

Dreamsorcerer Mar 11, 2025

Choose a reason for hiding this comment

kjander0 commented Mar 11, 2025

TimMenninger commented Mar 11, 2025 • edited Loading

Dreamsorcerer commented Mar 11, 2025

TimMenninger commented Mar 11, 2025

TimMenninger commented Mar 11, 2025 • edited Loading

TimMenninger commented Mar 10, 2025 •

edited

Loading

codspeed-hq bot commented Mar 10, 2025 •

edited

Loading

codecov bot commented Mar 10, 2025 •

edited

Loading

TimMenninger commented Mar 11, 2025 •

edited

Loading

TimMenninger commented Mar 11, 2025 •

edited

Loading