Skip to content

Refactor: plain socket objects instead of wrapper classes#3454

Open
pajod wants to merge 10 commits intobenoitc:masterfrom
pajod:unwrapped-sockets-originally-pr3217
Open

Refactor: plain socket objects instead of wrapper classes#3454
pajod wants to merge 10 commits intobenoitc:masterfrom
pajod:unwrapped-sockets-originally-pr3217

Conversation

@pajod
Copy link
Contributor

@pajod pajod commented Jan 23, 2026

So, here is a rebased version with the fixup I commented earlier.

tilgovi and others added 3 commits January 23, 2026 20:57
Refactor socket creation to remove the socket wrapper classes so that
these objects have less surprising behavior when used in worker hooks,
worker classes, and custom applications.

Rebased from benoitc#3127

Co-authored-by: Paul J. Dorn <pajod@users.noreply.github.com>
@pajod
Copy link
Contributor Author

pajod commented Jan 23, 2026

Whoops. There is no regression test for --enable-backlog-metric!

Copy link
Owner

@benoitc benoitc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the refactor, code is cleaner :)

Few things to address:

ASGI Worker needs updating

The ASGI worker (gunicorn/workers/gasgi.py) still accesses sock.sock (line 181) and uses str(sock) for logging (line 187) - won't work with plain sockets.

listen() for fd-passed sockets

Old code called sock.listen(self.conf.backlog) including for systemd-passed fds. New set_socket_options() doesn't call it. Could this cause issues with systemd socket activation?

SSL validation timing

Changing to validate_file_exists at config time might break setups where certs are generated dynamically before start. Maybe keep runtime validation?

Tests

As you noted, need regression test for --enable-backlog-metric. Also missing tests for get_uri(), get_backlog(), set_socket_options().

Otherwise looks good - the close_sockets() error handling improvement is nice.

@pajod
Copy link
Contributor Author

pajod commented Jan 24, 2026

ASGI Worker needs updating

Copied the changes there, but did not check beyond running tests.

listen() for fd-passed sockets

Right. I dont think it does anything, now that I dropped the fromfd duplication. Its the same socket, so why would the kernel mess with the backlog hint? But I'll put it back to avoid unforeseen consequences. I don't see any pressing need to change that right now.

SSL validation timing - Maybe keep runtime validation?

Fair point. Reverted.

Tests

I pushed some more unit tests (and the whole thing still passes my nginx test).


I have one more commit to fix edge cases like --bind="[fe80::1%enp21s0]:80" but that is fairly verbose with new socket.getaddrinfo/socket.getnameinfo calls (and also known to be broken for eventlet) so I will leave this PR as-is and submit that separately.


Things I attempt to not touch here, but would like to follow up with:

  • Make reuse_port behaviour clear and consistent on non-supporting platforms
  • Make LISTEN_FDS= compatible with --bind being used simultaneously (and fix reload/re-exec for all resulting edge cases)

@pajod pajod marked this pull request as ready for review January 26, 2026 03:35
@pajod
Copy link
Contributor Author

pajod commented Jan 28, 2026

I would be interested to receive clear confirmation from someone using the uvicorn-worker package and receiving superfluous "Bad file descriptor" messages on whether these changes resolve the issue, as suggested regarding the original PR this is a rebase for.
(tagging @qubydev @dtrasboIMG separately so to not bump everyone that commented on the issue some years ago)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants