Repository navigation
asyncio.start_unix_server maybe shouldn’t default to cleanup_socket=True when sock parameter is passed #133354
Description
Activity
Hey, I can take this issue.
Before creating a PR though, I'd want to confirm whether this was intended or not, because it seems like something that was intentional.
Reacted by Christopher HeadDoing cleanup by default when
pathis passed is a change in behaviour too, but it seems like a good idea IMO (in that case the server is responsible for creating the socket so it’s logical it should also be responsible for deleting it). Only whensockis passed does it seem like it probably shouldn’t be done by default.Reacted by dmitrin9Agreed that getting an opinion, ideally from whoever made the change initially, would be good.
Haha yea I was searching the github PRs tab, grepping, listing via gh-cli, and I genuinely couldn't find it. I was able to narrow it down to before python 3.13 and after python 3.12.
Is there a standardized way to find who initially made a change?
The discussion there mentions the use of the
sockparameter, but only in the sense of “it’s harder to get the name if someone uses that, but hey we can usegetsockname”; I don’t see any discussion of the idea that maybe it shouldn’t be removed at all in that case.@CendioOssman perhaps you have some thoughts on this, as the author of the original change?
(Side note, if you want to get the inode number for the listening socket, why are you doing
getsocknameplusstatrather thanfstat, or, equivalently,staton the descriptor rather than the name? That would fix a race if some other process replaces the socket after youbindbut before youstat.)The idea was that since this creates a server object, it implies a great deal of ownership. E.g. accepting and creating new sockets.
Secondly, removing all traces of the socket when it is closed is how most sockets behave, except for Unix sockets.
Hence, having the cleanup be enabled by default seemed like the most useful and least surprising option.
Admittedly, systemd socket activation was not considered.
(Side note, if you want to get the inode number for the listening socket, why are you doing
getsocknameplusstatrather thanfstat, or, equivalently,staton the descriptor rather than the name? That would fix a race if some other process replaces the socket after youbindbut before youstat.)I seem to recall that the socket's inode number was from the magical socket file system, and not from the file on disk. So this is a race, yes, but no alternative was found.
1 remaining item
Philosophically, I think the issue regarding ownership is that any socket (UNIX-domain or otherwise) ought to be cleaned up once the socket no longer exists. The problem is, with any socket (UNIX-domain or otherwise), the socket no longer exists once the last file descriptor referring to it is closed, not once this particular file descriptor referring to it is closed. With non-UNIX-in-filesystem sockets (i.e. non-UNIX sockets or UNIX sockets outside the filesystem), that’s trivial: the kernel cleans up the socket once the last descriptor is closed. With UNIX-in-filesystem sockets, if you get the
pathand create the descriptor yourself, you can be reasonably certain nobody else has a copy of the descriptor (at least, code would have to work quite hard to get it out intentionally and share it with someone); on the other hand, if you get the descriptor viasock, there’s no such guarantee (even if you take ownership of, and responsibility for closing, that descriptor, you can’t assume that nobody else has another descript0r>Reacted by dmitrin9Indeed, the library code cannot know for sure and has to be told by the application. So the question was really what the most useful default should be.
I made the assumption that the vast majority of cases (but not all) would have the asyncio server as the sole owner of the socket. Hence, that cleanup should be on by default. That would reduce the risk of developers overlooking the setting. And it keeps application code cleaner, as they don't need to explicitly specify this.
I think it would be a big mistake if the default was changed when
pathis used, but that doesn't seem to be in question. Forsock, I think the usage might be more mixed and the default could be questioned. I'm not convinced the majority of those users have shared socket, though.Given that the usage of
sockis unclear, I would still vote for keeping the current default for the simple reason that it keeps the API understandable. The default ofcleanup_socketwould be clear and not conditional on other details.What would be the issue with making cleanup_socket a required argument as opposed to having a default?
Requiring the argument would arguably make the function more clear IMO.
I made the assumption that the vast majority of cases (but not all) would have the asyncio server as the sole owner of the socket. Hence, that cleanup should be on by default. That would reduce the risk of developers overlooking the setting. And it keeps application code cleaner, as they don't need to explicitly specify this.
If this were new, I’d agree with you more strongly. I’m mostly bothered because it’s a compatibility break from Python 3.12. And not even one that’s nice to work around, given that I have to write this ugliness:
if sys.hexversion >= 0x030D00F0: async def start_unix_server_from_socket(sock: socket.socket) -> asyncio.Server: return await asyncio.start_unix_server(handle_connection, sock=sock, cleanup_socket=False) else: async def start_unix_server_from_socket(sock: socket.socket) -> asyncio.Server: return await asyncio.start_unix_server(handle_connection, sock=sock)
What would be the issue with making cleanup_socket a required argument as opposed to having a default?
That would arguably be nice, but would break backwards compatibility with 3.12 even more.
I understand where Mr. Ossman is coming from and I think his defaults were sane. I am a little uneasy about the compatibility break and the problem with socket activation, but a tradeoff needed to be made and IMHO it's a fair trade off, given you can always undo it with an additional argument, albiet with additional code.
Python, at least to my knowledge, is very philosophically implicit, and a lot of the features of the language will abstract lower level logic or do things implicitly (the type system, pointers, large amounts of metadata that you sometimes can't control) that can sometimes make the language easier or more difficult to work in, but will usually provide a very fast and seamless experience developing in the language with the tradeoff of taking away control. That's why it's so widely used in prototyping and research. So I feel like the discussion earlier about giving the programmer less flexibility by default is largely irrelivent. The main issue at this point is the compatibility break, but I've already spoken on that.
I definitely do think we need to open a separate issue to deal with the problem with the documentation. I can do that if you guys want.
Reacted by dmitrin9I was planning on making a PR for the docs myself (assuming you mean the missing parameter on
start_unix_server), once someone decided what was going to happen code-wise, if that PR didn’t already include the docs change. But, feel free to do it yourself if you want to!Yea, I can take it.
What is your opinion on factoring out cleanup_socket logic into a separate function?
If this were new, I’d agree with you more strongly. I’m mostly bothered because it’s a compatibility break from Python 3.12. And not even one that’s nice to work around, given that I have to write this ugliness:
That's a good point. OTOH, a different default would just move the ugliness to those wanting to enable the feature?
But, I guess that ship has sailed anyway? 3.13 is already out and applications would need to be prepared for how that behaves.
That's a good point. OTOH, a different default would just move the ugliness to those wanting to enable the feature?
I don’t think so? The auto-cleanup feature didn’t exist prior to 3.13 at all, which means applications that want it simply have to require ≥3.13; whatever the default had been or became, they could unconditionally pass
True.But, I guess that ship has sailed anyway? 3.13 is already out and applications would need to be prepared for how that behaves.
Yes, that is unfortunately true; it may be too late to change anything other than documentation. I have no idea what the rules are.
"Specification has entered the chat..."
https://peps.python.org/pep-0387/#basic-policy-for-backwards-compatibility
This appears to be relevant. It seems to mainly be referencing the C-Api but I'm almost certain it applies to stdlib as well.
According to these here docs, the best course of action would be to resolve the incompatibility.
In general, incompatibilities should have a large benefit to breakage ratio, and the incompatibility should be easy to resolve in affected code.
Thanks for the pointer to that PEP.
It seems to mainly be referencing the C-Api but I'm almost certain it applies to stdlib as well.
Near the top is a section that says “This policy applies to all public APIs. These include: … Given a set of arguments, the return value, side effects, and raised exceptions of a function. This does not preclude changes from reasonable bug fixes.” which I think covers this case (the C API is mentioned as one bullet in that list, not as an overall topic for the list as a whole).
Anyway, also considering this quote:
Unless it is going through the deprecation process below, the behavior of an API must not change in an incompatible fashion between any two consecutive releases.
suggests to me that this change, at least for the
sockcase, should not have been made—it makes reasonable code crash or behave significantly differently.However, given that the change did happen, does that document serve as authority to make another change to change the behaviour back, or is that considered a second breaking change which needs to be considered on its own merits?
a different default would just move the ugliness to those wanting to enable the feature?
To clarify what I meant by my sentence above, in case it wasn’t clear. As I see it, there are basically two axes:
cleanup_socketdefaults toFalsecleanup_socketdefaults toTrueApplication doesn’t want asyncio to delete the socket Do nothing; works on all Python versions Check Python version and pass cleanup_socket=Falsefor ≥3.13, or not for ≤3.12Application wants asyncio to delete the socket (only possible at all on ≥3.13) Pass cleanup_socket=TrueDo nothing To me, the top-right quadrant is a lot uglier than any of the other three, so it seems like it should ideally be avoided. Of course, now, there is also the fact that 3.13 is out and already has the
Truedefault, so this isn’t purely a question of what the ideal initial behaviour would have been.@benjaminp Hey, since you were the original author of PEP 387, we were wondering if you had anything to add regarding our interpretation of the spec.
However, given that the change did happen, does that document serve as authority to make another change to change the behaviour back, or is that considered a second breaking change which needs to be considered on its own merits?
Pretty much, we're dealing with a breaking change in the asyncio stdlib module, this change was already shipped to 3.13 and all consecutive versions.
As it currently stands, the feature was introduced in 3.13 and 3.14 is still under development even though 3.15alpha0 is out. Does this count as "two consecutive releases"?
I think just to be safe we should wait until 3.15 is stable to continue with this issue unless anybody has any ideas. I'd be happy to work on the PR for it when that time comes.
I will also be opening an issue and PR for the doc change.
edit: I'll open it in a few hours when I get home.
Bug report
Bug description:
When
asyncio.start_unix_serveris called and thesockparameter is passed (rather thanpath), a server is wrapped around an existing socket. In this case, the fact thatcleanup_socketdefaults toTrueis very weird and potentially broken: it means that asyncio will try to delete a socket that someone else created (either other code in the same process, or, in the case of e.g. systemd socket passing, a socket created by a different program).This is problematic for a couple of reasons when using systemd socket passing:
os.statcall increate_unix_serverblows up.So this is definitely a nontrivial backwards compatibility break from 3.12 (which didn’t have cleanup logic at all and didn’t have the parameter) to 3.13, despite not being mentioned in the release notes. It also seems like a bit of a footgun in general, not to mention the fact that in implementation
start_unix_servertakes arbitrary kwargs and forwards them tocreate_unix_server, but the documentation instead mentions each parameter explicitly and doesn’t mentioncleanup_socketat all.CPython versions tested on:
3.13
Operating systems tested on:
Linux