Listening asyncio UNIX socket isn't removed on close #111246
Comments
|
@CendioOssman I'm pretty ignorant when it comes to such matters. For regular IP sockets this doesn't seem to be a problem (after closing one, the kernel sees to its proper disposal), so this seems unique to UNIX domain sockets. Can you point to some well-respected document explaining why it's best practice to explicitly remove a UNIX domain socket after closing it? I would also be concerned about removing the socket too soon, when multiple processes are listening on the same address (this is possible for IP sockets, I don't know if it works for UNIX sockets). |
I'm not sure if there is a forum where such a document could appear. This is more a de facto situation than de jure. There is this in the unix(7) man page though:
Some examples of very major software doing this, though: Xorg: https://gitlab.freedesktop.org/xorg/lib/libxtrans/-/blob/master/Xtranslcl.c#L1309 An example of a major software not doing it is dbus-daemon, but it normally uses abstract sockets or uses temporary directories, so it might not have considered much of a need.
I'm not sure, to be honest, but this thread suggests that it isn't: https://stackoverflow.com/questions/15716302/so-reuseaddr-and-af-unix It also contains another quote as to the responsibility of deleting the file:
|
|
A technical note here is that it is pretty easy to steal UNIX socket addresses, unlike TCP/IP addresses. You just None of the above examples consider this scenario, but there might be some odd cases that consider this normal and sane behaviour. For some handling of this, Python could do a A second technical issue if the server was created using the |
|
Okay, you've convinced me. Can you submit a PR? This would be new behavior in 3.13, require tests and docs, and a brief "what's new" entry. |
|
I can have a go. I've never run the tests for CPython though. I hope this is a good guide: https://devguide.python.org/testing/run-write-tests/index.html |
|
Yeah, that’s the ultimate guide. I often run python-m test test_asyncio or even python-m test test_asyncio. The further flags -v and -m are also handy. |
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.
|
I've now opened #111483. Please have a look and see what you think. |
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.
Try to clean up the socket file we create so we don't add unused noise to the file system.
|
Following up on my mention above, I'm a bit worried that this change will break backward compatibility with earlier versions of Python. Any code out there which creates UNIX domain sockets and manually cleans them up (which should be quite common) will now get an error back when trying to remove/unlink the socket. Unless the code intentionally ignores the failure or uses some other form of cleanup like TemporaryDirectory which doesn't reference the socket name explicitly, this change will lead to failures. |
This commit adds guards around code which cleans up UNIX domain sockets, to protect against a change proposed at python/cpython#111246 which would cause the socket to clean itself up on close.
|
Thanks for the feedback. It reminds me of https://xkcd.com/1172/. In the issue you mention only test were affected -- I agree it's annoying, but the fix is straightforward and can be backported with no problem. I think in this case it's better to Do The Right Thing (TM), i.e. remove the socket, which ideally this API should have done from the start, rather than forever being stuck in a slightly buggy state, where the API leaves behind random junk. |
|
Thanks, Guido. I agree that it's much cleaner to automatically remove the socket from the filesystem on close, and it would have been great if it had done so from the start. This change will eventually make things better, at least once people no longer need to support Python 3.12 and earlier. Is there any mechanism in the changelog to call out potentially breaking changes that need attention on upgrade? While in my case it only broke test code, any real code using UNIX domain sockets probably would likely have triggered the same kind of error my test code did. As you said, it's not hard to fix, but it will require a code change in the caller in most cases if the code was previously doing proper cleanup. |
I'm not aware of something like that. It might be a useful idea though -- you could start a thread on discuss.python.org about it. (Better wait until January, folks are checking out for the holidays.) |
|
Thanks - will do. Happy holidays! |
|
For reference, we ended up with the following to support all Python versions: server.close()
try:
os.unlink(addr)
except FileNotFoundError:
pass
except OSError as e:
log.error('Error removing socket %s: %s' % (addr, e)) If the |
|
Yeah - I did something similar here in asyncssh, to avoid the application code triggering a failure if it finds that the UNIX domain socket is already removed. |
Try to clean up the socket file we create so we don't add unused noise to the file system.
Bug report
Bug description:
After creating a UNIX socket server with
loop.create_unix_server()and then closing it usingServer.close(), then the UNIX socket file is still left on disk.Although asyncio itself can handle this on the next startup since #72585, it's still a bit rude (and confusing) to leave stale things around on the file system.
Callers can make sure to pair
Server.close()with anos.unlink(). But it would be a lot neater if Python could automatically do this, as basically everyone should do it anyway.CPython versions tested on:
3.9
Operating systems tested on:
Linux
Linked PRs
The text was updated successfully, but these errors were encountered: