Simplify sockets #670

jmfrank63 · 2018-09-16T18:37:18Z

This is code to look at as part of #542 It seems static analysis tools are not able to pick up any dynamically added attributes.
The only chance if we want code completion for all the trio.socket attributes is to define them statically.
I have an example in socket.py that works, it looks complicated and is a leftover of my try to add the attributes to a class dynamically hoping the static analysis would pick it up.
But to no avail. davidhalter/jedi#997

Code generation might be able to do this, but that will make it even more difficult to deal with the subject.

There is a discussion open at davidhalter/jedi#385 which could point out the problem being solved on the other end of the stick.

@Zac-HD @njsmith What do you think?

I think this request will be closed after the discussion. The only reason I could think of it being accepted (with modification) is the micro optimisation of _reexport to update the globals once with a full dictionary instead of once per loop.

jmfrank63 · 2018-09-17T16:37:34Z

I will go ahead and do a static implementation to have a usable example on which we can judge about. I am aware that the socket constants are highly volatile, but at the moment this is our only option if we want code completion to be working.

njsmith · 2018-09-19T02:47:15Z

I think you're making this more complicated than you need to :-)

For the first version, we want to:

Take the re-exporting of all-caps constants that's in trio/_socket.py, and move it into trio/socket.py. (Yes, this means that tab completion won't work for these symbols. Too bad. It's probably not fixable, and to the extent it is, we can worry about it later :-).)
Remove the explicit __all__ management from trio/_socket.py and trio/socket.py
For the lowercase exports, add an explicit from ._socket import (... long list here ...) to trio/socket.py
I think that's all?

njsmith

There's some irrelevant formatting churn here, not sure why... I guess yapf is acting up again. Kind of annoying, but not a big deal.

A number of small comments below.

A larger question, where I'm not sure of the answer: right now the way this PR is structured, we import all the ALLCAPS constants from socket → trio._socket, and then we import them again from trio._socket → trio.socket. Maybe it would be simpler to take them out of trio._socket, and instead import them directly socket → trio.socket?

What makes this complicated is that there are some constants that are missing from socket, so we define them ourselves. And one of those constants, IPPROTO_IPV6, we actually use inside trio._socket. So we can't just move all this stuff to trio.socket.

I guess it would still be simpler to have everything except IPPROTO_IPV6 imported directly from socket → trio.socket, and just special-case IPPROTO_IPV6?

(@Zac-HD, any thoughts here?)

njsmith · 2018-09-21T06:21:55Z

trio/_socket.py

 from functools import wraps as _wraps

+_stdlib_socket = _importlib.import_module('socket')


Why are you using importlib here?

Any use of importlib removed again, as there is no benefit and it makes the code only harder to read.

njsmith · 2018-09-21T06:23:04Z

trio/_socket.py

+try:
+    from socket import (
+        if_nameindexif_nametoindex, if_indextoname, sethostname
+    )


I think you're missing a comma in if_nameindexif_nametoindex

I thought I had fixed this already but I'll take a look again.

But this typo shows the weakness of this approach. I had to put these four into a try catch block because on py3.5 they are not there. Actually I should put each of the names in a try except block of its own but that would make the code even uglier.

So we do not only have a constant version/os dependency but also for functions.

I can see you are somehow reluctant towards a star import with overwriting which I now would prefer but I still do not know why exactly. Now you have about 25 years more experience than me so this is very likely well founded, I'd just like to understand why.

njsmith · 2018-09-21T06:23:25Z

trio/_socket.py

-#   # we only get here if the sync call in fact did fail with a
-#   # BlockingIOError
-#   return await do_it_properly_with_a_check_point()
-#


Why did you delete this comment?

Comment reintroduced deletion was an accident, held it for my own. I never touch comment of others.

njsmith · 2018-09-21T06:29:24Z

trio/_socket.py

+except ImportError:
+    if _sys.platform == "darwin":
+        IPPROTO_SCTP = 132
+        globals()['IPPROTO_SCTP'] = IPPROTO_SCTP


Huh, is IPPROTO_SCTP something you need for something? Not sure how it relates to this PR...

Removed all unnecessary definitions from _socket.py. I did also just remove the import of SO_REUSEADDR from _socket.py as it is not used and deleted it in socket.py.

njsmith · 2018-09-21T06:33:04Z

trio/_socket.py

        IPPROTO_IPV6 = 41
-        __all__.append("IPPROTO_IPV6")
+        globals()['IPPROTO_IPV6'] = IPPROTO_IPV6


The globals() assignment here doesn't do anything :-). We're not inside a function, so when we wrote IPPROTO_IPV6 = 41 it already assigned to globals.

Removed the noop statements.

njsmith · 2018-09-21T06:34:08Z

trio/_socket.py

@@ -310,13 +293,12 @@ def fromfd(*args, **kwargs):
 if hasattr(_stdlib_socket, "fromshare"):

    @_wraps(_stdlib_socket.fromshare, assigned=(), updated=())
-    @_add_to_all
+    # @_add_to_all


Can delete this line instead of leaving it commented out.

@jmfrank63 in general, please be sure that new pushes are addressing outstanding comments. Otherwise it seems that github automatically marks the comments as resolved, and they get lost. I think this was a pervasive communication problem on your last PR.

@belm0 Thanks for letting me know, I will go through all of them and address them in the next push. There is so much to learn, thanks for the patience. I will also try to attempt squashing this time, as there is so much back and forth.

Thank you for considering. (And again it's dubious that GitHub behaves that way.)

About squashing, that can be done trivially by the person doing the final merge into master.

njsmith · 2018-09-21T06:35:08Z

trio/_socket.py

@@ -549,6 +550,7 @@ def shutdown(self, flag):
        # empty list.
        assert len(gai_res) >= 1
        # Address is the last item in the first entry
+        # normed = gai_res[0][-1]


Not sure this comment is adding much :-)

I put it in because it does the same as your assignment but pylint doesn't complain. Was meant as a reminder to change this one day so we have one complaint less.
(actually this one was the last standing) But this should be very likely if at all a different PR. Right?

Oh, does pylint complain about the unpacking assignment? That's weird, it seems more readable to me...

And yes, better to separate it out in any case.

Yes, it did complain about too many stars in expression. Just FYI.

Huh, yeah, looks like a pylint bug – it's supposed to catch illegal code like *a, *b = c, but it also fires on valid code like this. Filed at pylint-dev/pylint#2513

Reported 16 hours ago, fixed an hour ago, how do you do this?
Besides I would have never considered this a bug in pylint. So amazing.
Anyways, removed the comment again.

njsmith · 2018-09-21T06:35:29Z

trio/socket.py

-# This is a public namespace, so we don't want to expose any non-underscored
-# attributes that aren't actually part of our public API. But it's very
+# This is a public namespace, so we dont want to expose any non-underscored
+# attributes that arent actually part of our public API. But its very


...why did you delete the apostrophes?

Back in, deleted by accident when doing a replace all.

njsmith · 2018-09-21T06:36:34Z

trio/socket.py

-from ._socket import __all__
+import importlib
+
+_smod = importlib.import_module('._socket', 'trio')


Probably clearer to write from . import _socket as _smod (or just from . import _socket, since it already has an underscore on the name).

Yes, done. I tried to pull directly into socket.py whatever I could. Interestingly the python interactive shell with jedi configured does have full code completion, while ipython hangs. Either some implications of pip install -e or just bugs.
Anyways it currently looks good.

njsmith · 2018-09-21T06:37:51Z

trio/socket.py

+    fromfd, from_stdlib_socket, getprotobyname, socketpair, getnameinfo,
+    socket, getaddrinfo, set_custom_hostname_resolver,
+    set_custom_socket_factory, real_socket_type, SocketType, fspath,
+    run_sync_in_worker_thread


Some of these imports look strange... real_socket_type wasn't public before, was it? and run_sync_in_worker_thread is definitely not supposed to be exposed from trio.socket... did you check that this PR doesn't change the things exported from trio.socket?

Removed. Shouldn't be in the next push.

Zac-HD · 2018-09-21T07:28:11Z

(not subscribed to this thread since know almost nothing about sockets, but @ me and I'll respond!)

A larger question, where I'm not sure of the answer: right now the way this PR is structured, we import all the ALLCAPS constants from socket → trio._socket, and then we import them again from trio._socket → trio.socket. Maybe it would be simpler to take them out of trio._socket, and instead import them directly socket → trio.socket?

I would certainly find it more tasteful to import them directly from socket → trio.socket.

What makes this complicated is that there are some constants that are missing from socket, so we define them ourselves. And one of those constants, IPPROTO_IPV6, we actually use inside trio._socket. So we can't just move all this stuff to trio.socket.

Define as many as possible in trio.socket, and define-and-export those we need in trio._socket I guess. This is probably better than defining them twice, but I could easily be argued around.

I guess it would still be simpler to have everything except IPPROTO_IPV6 imported directly from socket → trio.socket, and just special-case IPPROTO_IPV6?

Yep 😥

belm0 · 2018-09-22T00:38:27Z

trio/_core/_io_windows.py

@@ -341,8 +341,8 @@ def monitor_completion_key(self):
        if sock in self._socket_waiters[which]:
            await _core.checkpoint()
            raise _core.ResourceBusyError(
-                "another task is already waiting to {} this socket"
-                .format(which)
+                "another task is already waiting to {} this socket".


suggest that unrelated code formatting be done in a separate PR

@belm0 Oh, I didn't touch that module at all, guess that came from yapf and it did slip in with a git add --all which I learned I shouldn't do. I'll have to check how to revert a single file of a commit to redo this change, I guess that should be possible.

changes.

jmfrank63 · 2018-09-23T17:39:39Z

Hmmh, didn't touch any of the tests, and in theory this message should be filtered out at line 596.

trio/_socket.py

njsmith · 2018-09-26T06:08:11Z

trio/_socket.py

+    if _sys.platform == "darwin":
+        TCP_NOTSENT_LOWAT = 0x201
+    elif _sys.platform == "linux":
+        TCP_NOTSENT_LOWAT = 25


This whole block of code should just move into trio/socket.py, with the other constant export stuff. IPPROTO_IPV6 has to stay in this file because we actually use it down below, but we don't use TCP_NOTSENT_LOWAT here.

Makes sense, moved to socket.py

trio/socket.py

njsmith · 2018-09-26T06:14:59Z

trio/socket.py

+    try:
+        del SO_REUSEADDR
+    except NameError:
+        pass


You can just do del SO_REUSEADDR without any try/except. It happens that SO_REUSEADDR is always available, and you can see this by looking at the coverage report – the except NameError lines are never executed: https://codecov.io/gh/python-trio/trio/src/2ce378d3dd3597022847d9ced06187ed799037f1/trio/socket.py

njsmith · 2018-09-26T06:15:41Z

trio/socket.py

+try:
+    IPPROTO_IPV6
+except NameError:
+    from ._socket import IPPROTO_IPV6


This can just be from ._socket import IPPROTO_IPV6 without the try/except, since _socket has to make sure it's unconditionally defined.

try/except removed, let's see if we still pass...

njsmith · 2018-09-26T06:15:57Z

trio/socket.py

+    TCP_NOTSENT_LOWAT
+except NameError:
+    if _sys.platform != 'win32':
+        from ._socket import TCP_NOTSENT_LOWAT


This block should be replaced by the code from trio/_socket.py, as noted above.

…socket.py

pquentin · 2018-09-27T06:57:27Z

trio/_socket.py

    # As of at least 3.6, python on Windows is missing IPPROTO_IPV6
    # https://bugs.python.org/issue29515
-    if not hasattr(_stdlib_socket, "IPPROTO_IPV6"):  # pragma: no branch
+    if _sys.platform == "win32":  # pragma: no branch


According to https://codecov.io/gh/python-trio/trio/pull/670/changes you don't need pragma: no branch: this is covered by the Windows tests.

@pquentin I removed the comment. How do I learn about codecov? What would be some good resources to read?

@jmfrank63 Nothing that I know of, sorry :(

njsmith

Just two small changes below, and then I think this is ready!

I also confirmed manually that this doesn't change the set of exported symbols from socket (at least on my system), by comparing the output of

python -c 'import trio; [print(n) for n in sorted(dir(trio.socket)) if not n.startswith("_")]'

before and after the change.

njsmith · 2018-09-28T04:30:40Z

newsfragments/670.feature.rst

@@ -0,0 +1 @@
+* Reworked :mod:`trio.socket` namespace construction, making it more understandable by static analysis tools. This should improve tab completion in editors, reduce false positives from pylint, and is another step towards providing type hints.


Thinking about it, it's going to be kind of weird-looking in our final change log to have multiple almost-identical entries with just the module name changed.

Since #542 is already an umbrella bug for these changes, how about we drop this file, and instead modify the existing 542.feature.rst to cover the new work as well? And while we're at it, we can add mention of the work in #656 to fix trio.testing... so it can say something like:

"Reworked :mod:trio, :mod:trio.testing, and :mod:trio.socket namespace construction, making them more understandable..."

Also BTW, newsfragments shouldn't start with a *. Towncrier will automatically add a * when it assembles the release notes, and then things will look weird if we have two *s :-)

Yes, makes sense. I'll correct this later this day, currently commuting. I actually put this in in haste to have a change, before I learned that I could actually do an empty commit.

njsmith · 2018-09-28T04:31:48Z

trio/_core/_run.py

-                    "without some sort of compatibility shim."
-                    .format(async_fn)
+                    "without some sort of compatibility shim.".
+                    format(async_fn)


I upgraded yapf (#694), so I think you need to merge from master and then re-run yapf (using 0.24.0) before we can merge. Hopefully that will fix these weird formatting issues too...

Guess what ;-) now yapf fails with yapf: Unknown style option "SPLIT_BEFORE_DOT"
... Oh, maybe I should upgrade yapf as well. Nevermind false alarm.

Yeah, you definitely need yapf 0.24.0

njsmith · 2018-09-28T07:53:53Z

Well, I filed google/yapf#624 anyway for this annoying thing in trio/_signals.py.

jmfrank63 · 2018-09-28T07:55:37Z

Looks like we are fine now, any fine tuning possibly?

njsmith · 2018-09-28T08:01:38Z

@jmfrank63 #670 (comment)

jmfrank63 · 2018-09-28T09:27:41Z

Good to see checks are finally passing ;-) I guess we're getting close now.

njsmith · 2018-09-28T09:28:10Z

newsfragments/542.feature.rst

You need the backticks around the module names :-) It's a restructured text thing: http://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#roles

Reworked :mod:`trio`, :mod:`trio.testing`, and :mod:`trio.socket` ...

Ups, the last 1% is 99% of the work :-)

njsmith · 2018-09-29T02:52:45Z

🎉 🎈

jmfrank63 added 7 commits September 16, 2018 13:04

switched to dynamic importing

e404d40

Added AF_APPLETALK manually to see the effect

3fcd475

Removed AF_APPLETALK again as tests failed

711e7b4

imported _socket directly as socket

8ea86ab

Added print for better understanding (temporarly)

268cc33

Reverted to import of socket

b9ee0d7

Dynamic updates are simply not picked up by static analysis tools

7bbb52b

jmfrank63 added 10 commits September 19, 2018 02:31

trio/socket.py : Moved to static import of names and emoved __all__

1b50f2a

Fixed import error of some constants not found on some platforms

fc666c2

Finding the offending symbols 1

9217753

Added new name IPPROTO_SCTP to import

1f01599

Moved new name IPPROTO_SCTP to try except block

b764a97

Moved new name TCP_KEEPINTVL to try except block

7118f9b

Moved new name TCP_KEEPCNT to try except block

6c93661

Moved new name TCP_FASTOPEN to try except block

37afde1

Moved new name AF_LINK to try except block

1e438e3

Moved new name sethostname to try except block

305db2d

jmfrank63 added 3 commits September 19, 2018 03:48

Moved new name PF_SYSTEM to try except block

f7fe91b

Moved new name AF_SYSTEM to try except block

fc30951

Moved new name SO_USELOOPBACK to try except block

97bc5a8

python-trio deleted a comment from codecov bot Sep 19, 2018

njsmith reviewed Sep 21, 2018

View reviewed changes

belm0 reviewed Sep 22, 2018

View reviewed changes

Moved upper case names import to socket.py and undid prior comment

9ac6d34

changes.

jmfrank63 added 6 commits September 24, 2018 10:22

Merge branch 'master' into simplify_sockets

bd9a952

added newsfragment

7f142f9

Increased pickleshare version to 0.7.5

39a84f7

Dummy to trigger CI again

2453587

10 minutes too early try again now

f1d45a7

Do not import TCP_NOTSENT_LOWAT on windows

2ce378d

njsmith reviewed Sep 26, 2018

View reviewed changes

unnecessary code removed and definitions moved to from _socket.py to …

caead79

…socket.py

pquentin reviewed Sep 27, 2018

View reviewed changes

jmfrank63 added 2 commits September 27, 2018 09:31

Merge branch 'master' into simplify_sockets

ad2e09f

removed pragma comment

c43ced7

njsmith reviewed Sep 28, 2018

View reviewed changes

jmfrank63 added 3 commits September 28, 2018 08:21

Merge branch 'master' into simplify_sockets

70c9fd9

New yapf version should fix formatting issues

3b7cb99

Manually adjusted trio/_signals.py formatting

9b8dc48

Merged newsfragments under a single file

4cf0c77

njsmith reviewed Sep 28, 2018

View reviewed changes

added backticks to module names

59a899d

njsmith merged commit b01333c into python-trio:master Sep 29, 2018

jmfrank63 deleted the simplify_sockets branch September 30, 2018 13:41

		from functools import wraps as _wraps

		_stdlib_socket = _importlib.import_module('socket')

		@@ -0,0 +1 @@
		* Reworked :mod:`trio.socket` namespace construction, making it more understandable by static analysis tools. This should improve tab completion in editors, reduce false positives from pylint, and is another step towards providing type hints.

Simplify sockets #670

Simplify sockets #670

Conversation

jmfrank63 commented Sep 16, 2018

jmfrank63 commented Sep 17, 2018

njsmith commented Sep 19, 2018

njsmith left a 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

belm0 Sep 22, 2018 • edited Loading

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

Zac-HD commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmfrank63 commented Sep 23, 2018

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

njsmith left a 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

jmfrank63 Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njsmith commented Sep 28, 2018

jmfrank63 commented Sep 28, 2018

njsmith commented Sep 28, 2018

jmfrank63 commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njsmith commented Sep 29, 2018

belm0 Sep 22, 2018 •

edited

Loading

jmfrank63 Sep 28, 2018 •

edited

Loading