GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-111246: Remove listening Unix socket on close by CendioOssman · Pull Request #111483 · python/cpython

Conversation

@CendioOssman

@CendioOssman CendioOssman commented Oct 30, 2023 •
edited by github-actions Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Fixes #111246.

  • Issue: Listening asyncio UNIX socket isn't removed on close #111246

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@CendioOssman
CendioOssman requested review from 1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners October 30, 2023 09:37
@ghost

ghost commented Oct 30, 2023 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot mentioned this pull request Oct 27, 2023
Listening asyncio UNIX socket isn't removed on close #111246
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Oct 30, 2023
@bedevere-app

bedevere-app Bot commented Oct 30, 2023

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
CendioOssman added 4 commits October 30, 2023 14:47
@CendioOssman
pythongh-111246: Remove listening Unix socket on close (python#111246)
21ff6be 
Try to clean up the socket file we create so we don't add unused noise
to the file system.
@CendioOssman
pythongh-111246: Don't log on redundant socket cleanup (python#111246)
21e9d8c
@CendioOssman
pythongh-111246: Don't remove stolen Unix socket address (python#111246)
df3b74e 
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.
@CendioOssman
pythongh-111246: Add opt-out of Unix socket cleanup (python#111246)
93571fe 
Allow the implicit cleanup to be disabled in case there are some odd
corner cases where this causes issues.
@CendioOssman
CendioOssman force-pushed the unix_cleanup branch from 14a46a9 to 93571fe Compare October 30, 2023 13:47
@gvanrossum

gvanrossum commented Oct 30, 2023

Copy link
Copy Markdown
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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CendioOssman

CendioOssman commented Oct 31, 2023

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gvanrossum
gvanrossum reviewed Nov 1, 2023
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/asyncio/unix_events.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_asyncio/test_server.py

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

@gvanrossum gvanrossum Nov 1, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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. :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@CendioOssman CendioOssman Nov 6, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
kumaraditya303
kumaraditya303 reviewed Nov 2, 2023
View reviewed changes
Comment thread Doc/whatsnew/3.13.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
CendioOssman added 2 commits November 6, 2023 16:36
@CendioOssman
Mention gh issue in whatsnew
acadacb
@CendioOssman
Add comments to new unix server cleanup tests
1db87b7
gvanrossum
gvanrossum approved these changes Nov 6, 2023
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Nov 6, 2023
@gvanrossum

gvanrossum commented Nov 6, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CendioOssman

CendioOssman commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Nov 7, 2023

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ambv

ambv commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@CendioOssman

CendioOssman commented Nov 8, 2023

Copy link
Copy Markdown
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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
gvanrossum merged commit 74b868f into python:main Nov 8, 2023
@bedevere-app bedevere-app Bot removed the awaiting merge label Nov 8, 2023
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@CendioOssman @aisk
pythongh-111246: Remove listening Unix socket on close (python#111483)
2c03e74 
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
@fantix
Add cleanup_socket param on create_unix_server()
c3b04c9 
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 fantix mentioned this pull request Aug 28, 2024
Add cleanup_socket param on create_unix_server() MagicStack/uvloop#623
Closed
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@CendioOssman @Glyphack
pythongh-111246: Remove listening Unix socket on close (python#111483)
6ed0fa2 
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
@edgarrmondragon @fantix
Add cleanup_socket param on create_unix_server() (#623)
fb5a139 
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 <[email protected]>
@Hawk777 Hawk777 mentioned this pull request May 4, 2025
asyncio.start_unix_server maybe shouldn’t default to cleanup_socket=True when sock parameter is passed #133354
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 left review comments
@gvanrossum gvanrossum gvanrossum approved these changes
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
@willingc willingc Awaiting requested review from willingc willingc is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Listening asyncio UNIX socket isn't removed on close

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@CendioOssman @gvanrossum @ambv @kumaraditya303
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.