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

gh-111246: Remove listening Unix socket on close #111483

Merged
merged 6 commits into from
Nov 8, 2023

Conversation

CendioOssman
Copy link
Contributor

@CendioOssman CendioOssman commented Oct 30, 2023

Relieve developers from the burden of manually cleaning up their Unix sockets by having asyncio take care of that internally.

Fixes #111246.


📚 Documentation preview 📚: https://cpython-previews--111483.org.readthedocs.build/

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Oct 30, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Oct 30, 2023

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Try to clean up the socket file we create so we don't add unused noise
to the file system.
We only want to clean up *our* socket, so try to determine if we still
own this address or if something else has replaced it.
Allow the implicit cleanup to be disabled in case there are some odd
corner cases where this causes issues.
@gvanrossum
Copy link
Member

Once review has started (now) please don't force-push -- just add additional commits, otherwise review will be more work. We'll squash merge the final approved PR anyway. Also, merging main is usually not needed (GitHub is a bit pushy around that).

@CendioOssman
Copy link
Contributor Author

Ah, sorry. I'm used to the Linux kernel model where you churn the patch set until you have a history everyone is happy with. :)

I guess that means my careful splitting into distinct features wasn't necessary either. :)

FYI, the CLA is in progress via the slower organization route.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LG, two nits. Thanks for helping out!

Lib/asyncio/unix_events.py Show resolved Hide resolved
@@ -147,6 +149,74 @@ async def serve(*args):
await task2


class UnixServerCleanupTests(unittest.IsolatedAsyncioTestCase):
@socket_helper.skip_unless_bind_unix_socket
async def test_unix_server_addr_cleanup(self):
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding one-line comments to the new tests explaining what each test checks? They look so similar I first thought you had accidentally copied the test a few times. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. A commit with comments have been pushed.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks!

@gvanrossum
Copy link
Member

@CendioOssman Could you sign the CLA please so we can merge?

@CendioOssman
Copy link
Contributor Author

It was signed last Tuesday, so it should hopefully be in someone's queue. I got a confirmation from Adobe at least.

@gvanrossum
Copy link
Member

It was signed last Tuesday, so it should hopefully be in someone's queue. I got a confirmation from Adobe at least.

Okay, I'll wait a bit for our side to update the database then.

@ambv
Copy link
Contributor

ambv commented Nov 7, 2023

@CendioOssman, in the case of corporate CLAs we still need you to click through the personal CLA signing route to mark your email as green. We don't use blanket email domain approvals.

@CendioOssman
Copy link
Contributor Author

Ah. The link said I should use the manual form "instead", so I never proceeded with the GitHub form.

I've clicked through the personal approval as well now.

@gvanrossum gvanrossum merged commit 74b868f into python:main Nov 8, 2023
24 checks passed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Try to clean up the socket file we create so we don't add unused noise to the file system.
@CendioOssman CendioOssman deleted the unix_cleanup branch February 28, 2024 10:51
fantix added a commit to MagicStack/uvloop that referenced this pull request Aug 28, 2024
This is derived from python/cpython#111483 but available on
all Python versions with uvloop, only that it's only enabled
by default for Python 3.13 and above to be consistent with
CPython behavior.
fantix added a commit to edgarrmondragon/uvloop that referenced this pull request Aug 28, 2024
This is derived from python/cpython#111483 but available on
all Python versions with uvloop, only that it's only enabled
by default for Python 3.13 and above to be consistent with
CPython behavior.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Try to clean up the socket file we create so we don't add unused noise to the file system.
fantix added a commit to MagicStack/uvloop that referenced this pull request Sep 2, 2024
This is derived from python/cpython#111483 but available on
all Python versions with uvloop, only that it's only enabled
by default for Python 3.13 and above to be consistent with
CPython behavior.

* Also add Python 3.13 to CI (#610)
* Enable CI in debug mode
* Fix CI to include [dev]

---------

Co-authored-by: Fantix King <fantix.king@gmail.com>
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.

Listening asyncio UNIX socket isn't removed on close
4 participants