Unix client socket support. See: #279 #401

Fuyukai · 2018-01-14T15:08:34Z

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.

codecov · 2018-01-14T15:15:15Z

Codecov Report

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

@@           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 · 2018-01-14T20:16:01Z

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 · 2018-01-16T00:56:35Z

@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.

njsmith · 2018-01-16T01:00:05Z

trio/_highlevel_open_unix_stream.py

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


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.

njsmith · 2018-01-16T02:21:28Z

trio/_socket.py

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


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.)

I also filed bpo-32562.

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

njsmith · 2018-01-16T02:24:58Z

trio/tests/test_highlevel_open_unix_stream.py

+    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


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".

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

njsmith · 2018-01-16T02:26:00Z

trio/tests/test_highlevel_open_unix_stream.py

+
+    # 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__())


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.)

Will do later today.

njsmith · 2018-01-16T02:43:19Z

trio/_highlevel_open_unix_stream.py

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


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 :-)

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 · 2018-01-16T13:11:58Z

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 · 2018-01-17T03:39:35Z

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.

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

Fuyukai · 2018-01-17T19:24:05Z

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 :-)

njsmith · 2018-01-17T03:29:55Z

trio/tests/test_highlevel_open_unix_stream.py

+    # 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)


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)

njsmith · 2018-01-17T03:32:12Z

trio/_util.py

+
+
+if sys.version_info[0:2] >= (3, 6):
+    fspath = os.fspath


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.

njsmith · 2018-01-17T04:05:03Z

trio/_util.py

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


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...

njsmith · 2018-01-18T02:16:19Z

trio/_highlevel_open_unix_stream.py

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


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 · 2018-01-18T02:22:13Z

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 · 2018-01-18T11:49:58Z

Okay, that should be everything done.

pquentin · 2018-01-18T12:15:52Z

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

njsmith · 2018-01-19T05:22:00Z

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 · 2018-01-19T05:38:12Z

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

Fuyukai added 2 commits January 14, 2018 15:05

Unix client socket support. See: #279

25c3795

Don't break if Unix sockets aren't supported.

ba0af18

njsmith reviewed Jan 16, 2018

View reviewed changes

Fuyukai added 2 commits January 16, 2018 13:00

Rework some tests.

931b108

Add a proper fspath implementation for 3.5.

b61cc6d

8000

Re-YAPF.

860daab

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

njsmith reviewed Jan 18, 2018

View reviewed changes

Fuyukai and others added 2 commits January 18, 2018 13:47

Add to docs, add a news fragment, fix tests and fix function check.

9978c0f

Remove unnecessary #pragma: no cover

a3c9f09

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

pquentin mentioned this pull request Jan 19, 2018

Bump yapf version to 0.20.1 #406

Merged

Fuyukai deleted the unix-sockets branch January 19, 2018 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unix client socket support. See: #279 #401

Unix client socket support. See: #279 #401

Unix client socket support. See: #279 #401

Unix client socket support. See: #279 #401

Conversation

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment