-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add support for binding Unix sockets in Linux's abstract namespace. #3405
Conversation
fb895f3
to
f5b0705
Compare
f5b0705
to
e169be7
Compare
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, this looks pretty good, but I think the test could be improved.
else: | ||
if stat.S_ISSOCK(st.st_mode): | ||
os.remove(file) | ||
if not file.startswith("\0"): |
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.
This should have a comment to say that filenames with a leading NUL byte are used by the linux-specific abstract namespace. (Should we have a check for the platform name so that attempts to use a leading NUL are errors on other platforms? Probably not, IMHO)
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.
Added comment in be9e153.
tornado/test/httpserver_test.py
Outdated
self.stream = IOStream(socket.socket(socket.AF_UNIX)) | ||
self.io_loop.run_sync(lambda: self.stream.connect(self.sockfile)) | ||
if sys.platform.startswith("linux"): | ||
self.sockabstract = "\0" + os.path.basename(self.tmpdir) |
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.
This is a clever way to get a unique name for the abstract socket in most cases, although it does have some weaknesses since the abstract socket namespace may be shared more widely than the filesystem (e.g. containers with host networking, or just processes with different values of TMPDIR. So I'd rather use a uuid4 here than tie the abstract socket name to the temp directory.
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.
tornado/test/httpserver_test.py
Outdated
self.stream.write(b"garbage\r\n\r\n") | ||
response = yield self.stream.read_until_close() | ||
with closing(IOStream(socket.socket(socket.AF_UNIX))) as stream: | ||
stream.connect(self.sockfile) |
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.
This test is about introspecting socket addresses, so it feels like something that should be tested for abstract sockets too. Instead of binding both abstract and filesystem sockets in setUp (half of which is going to be wasted work in any given test), and only testing the abstract socket with certain tests that must be duplicated, I think it would be better to use subclasses: factor the socket binding logic out of setUp into an overridable method, and then make an AbstractUnixSocketTest that subclasses this one.
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.
Refactored unit-tests as suggested in be9e153.
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.
Sorry, I'm not getting on very well with the pull-request checks…
Any hints as to how you'd like to make mypy happy about the refactored test case. I didn't want to make the UnixSocketTest
base class an AsyncTestCase because I don't want the test functions to be executed outside of one of the, UnixSocketTestAbstract
or UnixSocketTestFile
concrete implementations; but a failure to do so makes mypy sad.
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.
Hmm, I thought I had a clever solution for this but it looks like the other places where I've done this are just hacks with a lot of typing.Any
and type:ignore
annotations. (Other discussions of the issue at abseil/abseil-py#166 and python/cpython#61721)
In this case I think I'd make UnixSocketTest
a subclass of AsyncTestCase
to make type checking happy, and then skip the base class at runtime: In UnixSocketTest.setUp()
, add if type(self) == UnixSocketTest: raise unittest.SkipTest("abstract base class")
. That might have the fortunate side effect of making pytest happier with this pattern too (#3390)
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.
Implemented as suggested in 6e14f25.
1ddd3d5
to
6e14f25
Compare
6e14f25
to
d24497e
Compare
Looks good, thanks for your patience with all the CI headaches (next time it'll be a little less painful because I won't have to come click "approve CI" for every new commit) |
Thank you! I did wonder whether I was missing something in being able to trigger the CI myself… I generally use single-quoted strings; and don't bother to run mypy on my unit tests (life's too short). Thanks for all of your work on Tornado; we use it extensively. |
Linux offers an "abstract namespace" for Unix sockets, distinguished by the address comprising an initial null byte. Specifying a
file
argument describing an abstract namespace address tonetutil.bind_unix_socket(…)
fails as a consequence of the additional, file-system orientated, processing performed:tornado/tornado/netutil.py
Lines 212 to 222 in bdfc017