#425 Remove UNIX socket from FS before binding. by socketpair · Pull Request #441 · python/asyncio (original) (raw)
This repository was archived by the owner on Nov 23, 2017. It is now read-only.
Conversation32 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
| if path[0] not in (0, '\x00'): |
|---|
| try: |
| if stat.S_ISSOCK(os.stat(path).st_mode): |
| os.remove(path) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the file exists and is not a socket?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if stat.S_ISSOCK(os.stat(path).st_mode)
As much as we can. Let's kernel to decide what to do during bind()
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand: can stat.S_ISSOCK return True if it's not a socket?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it can not. If this path refers to plain file, bind syscall will fail. We must not erase non-sockets in any case.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK, I now understand what was @ZhukovAlexander's question about.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still have a questions ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, no further questions about this line :)
I think we should also remove the socket after the server is closed.
| os.remove(path) |
|---|
| except OSError: |
| # Directory may have permissions only to create socket. |
| pass |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to log this.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but we should introduce something like log = logging.getLogger(__name__). Please tell where to do that.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at how base_events.py logs stuff.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| with self.assertRaisesRegex(OSError, |
|---|
| 'Address.*is already in use'): |
| self.loop.run_until_complete(coro) |
| self.loop.run_until_complete(coro) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work? We have a live socket, bound to path. Then we create a server. Does this mean that the first socket is in an invalid state? Or they are both bound to path now?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems, potential client will connect to second socket. First one will be hidden.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not check. Maybe it is implementation specific - i.e. Different on bsd and linux.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try to figure out what exactly is going on in this unittest.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked. If socket-inode on FS is deleted, no one can connect. If it overrided with another UNIX-socket, all clients will be connected to that new one, as I expect eariler.
import socket import os if os.path.exists('/tmp/qwe'): os.unlink('/tmp/qwe') sock1 = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) sock1.bind('/tmp/qwe') sock1.listen() os.unlink('/tmp/qwe')
sock2 = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) sock2.bind('/tmp/qwe') sock2.listen() #os.unlink('/tmp/qwe')
sock3 = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) sock3.connect('/tmp/qwe') sock3.send(b'asd')
(sk, addr) = sock2.accept() print(sk.recv(6))
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can not find POSIX standard, describinf this...reading man 7 unix say that many things are implementation specific.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, that unittests are fine. Unittests must check that all assertions are not failed on target OS.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What assertions? Clearly, there is some system-depending behaviour here: some systems allow Unix sockets to bind to existing in-use Unix sockets paths. Why should we have a test for that?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well. I will remove that test.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I transform that test: It now checks that binding to same path as CLOSED socket works. This will improve code-coverage. And also I have found a bug with logging :).
@socketpair FWIW we're just a couple days from a complete feature freeze of asyncio. Will you have time to finish this, or should I submit a new PR?
| @@ -272,9 +281,10 @@ def create_unix_server(self, protocol_factory, path=None, *, |
|---|
| raise ValueError( |
| 'path was not specified, and no sock specified') |
| if sock.family != socket.AF_UNIX: |
| if sock.family != socket.AF_UNIX or sock.type != socket.SOCK_STREAM: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I've just committed this check to asyncio, please rebase your PR. Regardless of this PR, this is something that needed to be fixed (I wanted to fix this before 3.5 came out, but completely forgot about it). /cc @gvanrossum
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
I think we should also remove the socket after the server is closed.
@1st1 It is not so easy, to do that, I have to intorduce something new inbase_events.Server
Actually (as I think), removing before binding and removing after closing should be done at Python's socket level, not at asyncio level. But this is another sad story.
I think we should also remove the socket after the server is closed.
@1st1 It is not so easy, to do that, I have to intorduce something new in base_events.ServerActually (as I think), removing before binding and removing after closing should be done at Python's socket level, not at asyncio level. But this is another sad story.
Maybe. But let's fix this in asyncio first.
Maybe. But let's fix this in asyncio first.
@1st1 Can you advice best way to add unlink() after closing ?
@1st1 Can you advice best way to add removing after closing ?
After giving this some thought - let's not add this. I now remember an issue with uvloop: originally I was removing the path, but then someone's program broke because it was cleaning up the path manually and assumed it's always there.
Trying to remove the path before connecting to it makes total sense, removing it after is nice, but not strictly necessary.
