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

Unix client socket support. See: #279 #401

Merged
merged 7 commits into from Jan 19, 2018

Conversation

Projects
None yet
3 participants
@Fuyukai
Contributor

Fuyukai commented Jan 14, 2018

A month later, I finally found the motivation to implement this.

Implementation was relatively simple; I reused SocketStream and wrote a very basic connector function (for client connections, I don't think there's any more that could be done).

Note:

  • Some of the changes are apparently due to yapf reformatting, and were unintentional.
  • The new test uses raw socket.socket to emulate a UDS server - not sure if that's the right thing to do.

Fuyukai added some commits Jan 14, 2018

@codecov

This comment has been minimized.

codecov bot commented Jan 14, 2018

Codecov Report

Merging #401 into master will decrease coverage by 0.09%.
The diff coverage is 88.88%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #401     +/-   ##
========================================
- Coverage   99.29%   99.2%   -0.1%     
========================================
  Files          87      89      +2     
  Lines       10256   10330     +74     
  Branches      712     718      +6     
========================================
+ Hits        10184   10248     +64     
- Misses         56      63      +7     
- Partials       16      19      +3
Impacted Files Coverage Δ
trio/_highlevel_socket.py 100% <ø> (ø) ⬆️
trio/_core/tests/test_run.py 100% <100%> (ø) ⬆️
trio/tests/test_highlevel_open_unix_stream.py 100% <100%> (ø)
trio/_core/_ki.py 100% <100%> (ø) ⬆️
trio/__init__.py 100% <100%> (ø) ⬆️
trio/_socket.py 100% <100%> (ø) ⬆️
trio/testing/_checkpoints.py 100% <100%> (ø) ⬆️
trio/_ssl.py 100% <100%> (ø) ⬆️
trio/tests/test_exports.py 100% <100%> (ø) ⬆️
trio/tests/test_ssl.py 100% <100%> (ø) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de54e1d...a3c9f09. Read the comment docs.

@pquentin

This comment has been minimized.

Member

pquentin commented Jan 14, 2018

Hello, and thank you for the pull request! I'm not part of the team, but just trying to help here.

You have formatting changes because trio currently uses yapf 0.19.0 but you followed the documentation and used the latest version, 0.20.1. It's not easy to notice: it turns out yapf 0.19.0 is happy with the changes made by yapf 0.20.1. Anyway, since google/yapf#484 is now fixed, it would make sense to upgrade to yapf 0.20.1, but maybe not as part of this pull request? Hope that helps!

@njsmith

This comment has been minimized.

Member

njsmith commented Jan 16, 2018

@pquentin

I'm not part of the team, but just trying to help here.

Pff, if you're going to go reviewing pull requests then you're part of the team :-). Check your inbox for an invite.

@SunDwarf

Some of the changes are apparently due to yapf reformatting, and were unintentional.

Yeah, sorry about this – as @pquentin mentioned, we've had some trouble with yapf bugs recently and the contributor docs are a little out of sync there. I think we can leave it be in this case, given the weird details. (Specifically it looks like 0.20.1 has fixed the misformatting bugs we were seeing, and also fixed a bug where older versions of yapf were simply skipping over some if statements entirely. So normally I'd suggesting running yapf 0.19.0 over your PR to revert back to the old formatting, but this is the one case where that doesn't actually work, and asking you to go through and revert them by hand seems like unnecessary busywork.)

I'll do a proper review in a second.

@njsmith

This looks pretty good! A few comments.

async def open_unix_socket(filename,):
"""Opens a connection to the specified
`Unix domain socket <https://en.wikipedia.org/wiki/Unix_domain_socket>`_.

This comment has been minimized.

@njsmith

njsmith Jan 16, 2018

Member

Weird ReST nitpick: it's better to get in the habit of using a double-underscore for ReST links, like

`Unix domain socket <https://en.wikipedia.org/wiki/Unix_domain_socket>`__

Double-underscore gives you the regular link semantics that everyone expects. Single underscore is subtly different, and can cause problems in weird cases (e.g. if you have two links in the same document with the same link text but different link targets, then with single underscores that's an error, for some reason).

Not a big deal, but trivial to fix so probably worth fixing just for style consistency.

# unwrap path-likes
if hasattr(address, "__fspath__"):
return address.__fspath__()
return address

This comment has been minimized.

@njsmith

njsmith Jan 16, 2018

Member

Oh, excellent catch.

At first I was going to suggest using os.fspath, but then I remembered that that's only added in 3.6. We could skip this check on 3.5, but... we probably want to support trio.Path objects even on 3.5. And then I looked again at trio's async file I/O code, and we actually have a second copy of this os.fspath emulation there.

Would you be up to refactoring this into a trio._util.fspath helper? Ideally based on the sample code here? (It turns out that calling address.__fspath__() directly is subtly wrong, because python's magic method lookup is weird.)

This comment has been minimized.

@njsmith

njsmith Jan 16, 2018

Member

I also filed bpo-32562.

This comment has been minimized.

@Fuyukai

Fuyukai Jan 16, 2018

Contributor

Sure, I'll update it when I get home today.

serv_sock.listen(1)
# for some reason the server socket can accept the connection AFTER it has
# been opened, so use that to test here

This comment has been minimized.

@njsmith

njsmith Jan 16, 2018

Member

Yep, that's the point of the listen(1) :-). It means "allow at least 1 connection to accepted, and hold onto it in the kernel, before I call accept".

This comment has been minimized.

@Fuyukai

Fuyukai Jan 16, 2018

Contributor

Ah, I didn't know that. Been a long time since I've used raw sockets.

# for some reason the server socket can accept the connection AFTER it has
# been opened, so use that to test here
unix_socket = await open_unix_socket(name.__fspath__())

This comment has been minimized.

@njsmith

njsmith Jan 16, 2018

Member

Can you also test with passing the Path object directly to open_unix_socket? (On 3.5 you can use a trio.Path to make sure it has an __fspath__ method.)

This comment has been minimized.

@Fuyukai

Fuyukai Jan 16, 2018

Contributor

Will do later today.

RuntimeError: If AF_UNIX sockets are not supported.
"""
if not has_unix:
raise RuntimeError("Unix sockets are not supported on this platform")

This comment has been minimized.

@njsmith

njsmith Jan 16, 2018

Member

Huh, how to handle this is an interesting question.

One tradition says, unsupported operations should just be missing, so you can do hasattr(trio, "open_unix_stream") to check for whether it's there. The other approach is to do this. On the one hand, the missing approach is probably more traditional in Python, and it's what we do for OS-specific operations in trio.hazmat. OTOH, it might make it harder to get trio.__all__ set up correctly and in a way that doesn't confuse static analysis tools, and we might need to switch how we handle OS-specific operations anyway to support pluggable backends. Hmm.

I'm going to hit post on this review so you can see the other comments, while I think about this a little more :-)

This comment has been minimized.

@njsmith

njsmith Jan 18, 2018

Member

I haven't come to any real conclusions here, but given the problems that autocompleters have when some functions are only conditionally defined (#314), and that this way callers get a better error message than AttributeError: open_unix_stream, I'm going to say let's leave this as is for now for now and we can revisit it later if we change our mind.

Fuyukai added some commits Jan 16, 2018

@Fuyukai

This comment has been minimized.

Contributor

Fuyukai commented Jan 16, 2018

Changes requested have been made.

There was a few places where the fspath util could be added, but I figured they were out of scope for the PR so I didn't touch them.

@njsmith

This comment has been minimized.

Member

njsmith commented Jan 17, 2018

The build is failing because yapf is cranky -- maybe we found a place where 0.19 and 0.20.1 disagree after all. If you could do a quick

pip install yapf==0.19.0
yapf -rpi setup.py trio
git commit -a -m "Run yapf"
git push

Then it should get the test suite running again.

Re-YAPF.
Signed-off-by: Laura F. D <l@veriny.tf>
@Fuyukai

This comment has been minimized.

Contributor

Fuyukai commented Jan 17, 2018

No clue why the build failed there. The tests just seemed to not run?

@njsmith

Yeah, the tests were acting weird because Travis had an outage yesterday -- it seems to be done now, so I restarted them.

A few small comments below, plus:

  • Can you add a newsfragment for this in newsfragments/? There's a README in that directory with instructions

  • Can you add open_unix_stream to the docs? The stream docs are a bit disorganized right now, but a .. autofunction:: open_unix_stream near the current docs for open_tcp_stream will at least make it findable :-)

# mktemp is marked as insecure, but that's okay, we don't want the file to
# exist
name = os.path.join(tempfile.gettempdir(), tempfile.mktemp())
await open_unix_socket(name)

This comment has been minimized.

@njsmith

njsmith Jan 18, 2018

Member

I believe xfail is supposed to mark tests that we want to pass, but that are currently failing -- like an executable bug report. If the goal is to test that this raises an exception, the best way to do that is

with pytest.assert_raises(OSError):  # or whatever the appropriate type is
    await open_unix_socket(name)
if sys.version_info[0:2] >= (3, 6):
fspath = os.fspath

This comment has been minimized.

@njsmith

njsmith Jan 18, 2018

Member

Let's make this something like:

if hasattr(os, "fspath"):
    fspath = os.fspath

It's generally a good habit to check directly for the thing we need instead of hard-coding version numbers, when we can. For example, it's possible PyPy will add os.fspath before they finish implementing the rest of 3.6's new features.

"not " + type(path).__name__)
raise TypeError("expected str, bytes or os.PathLike object, not "
+ path_type.__name__)

This comment has been minimized.

@njsmith

njsmith Jan 18, 2018

Member

Unsurprisingly, codecov is complaining about missing coverage in this function, because we don't have tests for the error branches. It'd be good to fix this at some point just in case, but I'm not going to hold up merging just for that...

RuntimeError: If AF_UNIX sockets are not supported.
"""
if not has_unix:
raise RuntimeError("Unix sockets are not supported on this platform")

This comment has been minimized.

@njsmith

njsmith Jan 18, 2018

Member

I haven't come to any real conclusions here, but given the problems that autocompleters have when some functions are only conditionally defined (#314), and that this way callers get a better error message than AttributeError: open_unix_stream, I'm going to say let's leave this as is for now for now and we can revisit it later if we change our mind.

@njsmith

This comment has been minimized.

Member

njsmith commented Jan 18, 2018

Meh, I started that review before you did the reformatting and it seems to have confused github a bit. Github is hiding some of my comments; please click "Show outdated" to see them, because they aren't actually outdated :-)

@Fuyukai Fuyukai force-pushed the Fuyukai:unix-sockets branch from 72ab0d1 to 3774149 Jan 18, 2018

@Fuyukai

This comment has been minimized.

Contributor

Fuyukai commented Jan 18, 2018

Okay, that should be everything done.

@pquentin

This comment has been minimized.

Member

pquentin commented Jan 18, 2018

Thanks! Unfortunately you have a new yapf issue: https://travis-ci.org/python-trio/trio/jobs/330320293

@Fuyukai Fuyukai force-pushed the Fuyukai:unix-sockets branch from 3774149 to 9978c0f Jan 18, 2018

@njsmith

This comment has been minimized.

Member

njsmith commented Jan 19, 2018

Looking good! I think the pragma: no cover on the if hasattr(os, "fspath") check was unnecessary (we run tests on both 3.5 and 3.6, so it should exercise both variants of the branch). So I removed it and now the tests are running again, and I'll merge once we confirm I didn't screw anything up with the extremely complicated edit :-)

@njsmith njsmith merged commit 645e5bb into python-trio:master Jan 19, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@njsmith

This comment has been minimized.

Member

njsmith commented Jan 19, 2018

Done! I'll send your Github invite in a moment.

@Fuyukai Fuyukai deleted the Fuyukai:unix-sockets branch Jan 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment