Public API for instantiating twisted.internet.tcp.Port from a file descriptor

[rwall]

I want to launch a twisted service via systemd, inetd, launchd using "socket activation".

•  http://0pointer.de/blog/projects/socket-activation.html

So I will need to pass an existing listening socket filedescriptor to t.i.t.Port

Or maybe I should be looking at creating a new type of Endpoint.

Related tickets: #4697, #4387

See also:

•  http://0pointer.de/public/systemd-man/sd_listen_fds.html
•  https://code.google.com/p/pyftpdlib/issues/detail?id=147
•  https://code.google.com/p/pyftpdlib/wiki/InetdSupport

[exarkun]

The interface should not be based on twisted.internet.tcp.Port:

1. Perhaps you want to inherit a listening UNIX socket instead of a TCP socket
2. Perhaps you want to use a reactor which isn't based on twisted.internet.tcp.Port
3. Direct use of twisted.internet.tcp shouldn't be encouraged

This suggests a reactor-based interface, eg reactor.listenTCPFD, etc. This sounds a bit tedious though. Perhaps there's a better approach.

There should likely also be an endpoint API, but that is most likely a simple layer on top of the real implementation.

[rwall]

Here is my first attempt...don't think Exarkun's going to like it :)

[rwall]

Replying to exarkun:

The interface should not be based on twisted.internet.tcp.Port:

Ah, I had already started down that route...only just saw your comment...nevertheless have a look at the attached patch...perhaps it will make it help us understand how to do this right.

1. Perhaps you want to inherit a listening UNIX socket instead of a TCP socket

Well once I had some feedback on SharedTcpPort I was intending to create a "SharedUnixPort".

1. Perhaps you want to use a reactor which isn't based on twisted.internet.tcp.Port

Hmm....hadn't thought of that...ie twisted.internet.iocpreactor.tcp.Port

1. Direct use of twisted.internet.tcp shouldn't be encouraged

This suggests a reactor-based interface, eg reactor.listenTCPFD, etc. This sounds a bit tedious though. Perhaps there's a better approach. There should likely also be an endpoint API, but that is most likely a simple layer on top of the real implementation.

Surely, I'm going to need to modify the Port APIs regardless of whether those are the public API.

So I create

• endpoint.Tcp4FdEndpoint
• reactor.listenTCPFD
• tcp.SharedPort
• iocp.SharedPort

...and then repeat for unix.SharedPorts udp.SharedPort etc

Let me know what you think...and ignore the scripts I added in that patch....those were just for me to test with.

[rwall]

A short conversation with Exarkun and Itamar about a better API for inheriting already listening sockets.

rwall 13:50:25
exarkun: Are you around?
exarkun: I was about to do some more work on http://twistedmatrix.com/trac/ticket/5248#comment:1 and want some advice on how to do it without modifying twisted.internet.tcp.Port 13:52:33
Heisenmink has disconnected (Quit: Leaving) 13:53	
sdeng has joined the room 13:53	
sdeng has disconnected (Client Quit) 13:57	

kenaan 13:58:38
itamar reviewed [#5382] - Provide a library for valueless named constants (assigned to exarkun)	

exarkun 14:08:05
rwall: I probably could have made that comment more clear (or maybe I couldn't have then, but I could have now)
rwall: I expect tcp.Port will need to be modified - but if so, it should be modified in support of some /other/ API 14:08:35
rwall: Even if we added the gross reactor.listenTCPFD, tcp.Port would still need some modification, because tcp.Port wants to create its own socket now. 14:09:01
 
itamar 14:11:20
one can imagine doing something like "fd = FD(mySocket.fileno()); reactor.listenTCP(fd, factory)"
i.e. extend the API to accept a file descriptor object instead of a port number 14:11:37
 
exarkun 14:13:40
Interesting idea	

rwall 14:13:47
Or should I start a new Endpoint method t.i.i.IStreamServerEndpoint.listenFD	

exarkun 14:14:01
No, not that.
The current endpoint interface works fine for a file descriptor 14:14:31
You just need a new implementation of the interface. 14:14:36
The endpoint is easy once there's support in the reactor, though, so we don't have to worry about it too much. 14:15:07
 
rwall 14:15:12
Right, I see that now.	
rajesh has joined the room 14:15	

exarkun 14:16:32
One annoying thing is that someone (itamar) finally started the long, long, long overdue refactoring of tcp.Port recently	

itamar 14:16:44
well, I've only done UDP so far
and I don't think it'll change the relevant code much 14:17:10
even once I get to TCP 14:17:24
 
exarkun 14:18:14
Without knowing what the changes are going to be to support listening on fds, I'm not sure how well one can judge how much of a barrier it will be to refactoring. 
If you don't mind though, okay. 14:18:28
 
itamar 14:18:40
assuming you do it with my suggestion above, no real impact
but really, based on what I did to UDP, it wasn't major surgery 14:18:52
 
rwall 14:19:04
Ok, so I modify reactor.listenTCP to allow a port integer arg or an FD instance....is that the idea?	

itamar 14:19:05
the *real* big deal is stream transports	

exarkun 14:19:07
I don't think the API you proposed implies any particular implementation	

itamar 14:19:21
I suppose
but it's just an if statement somewhere 14:19:26
plus 5 extra lines of code? 14:19:39
 
exarkun 14:19:50
You must have something else in mind that you haven't mentioned yet
(Also I'm not sure that API has a huge benefit over reactor.listenFD(socket, factory) - but either way seems like it is going to require some kind of tcp.Port changes) 14:20:14
 
itamar 14:21:09
well, the file descriptor may be UNIX, or TCP, or UDP	

exarkun 14:21:19
Noooo	

itamar 14:21:37
my proposed API means you can extend each separately	

exarkun 14:21:40
No	

itamar 14:21:42
instead of adding a new method for each	

exarkun 14:21:47
Those three things are not the same kinds of things
SOCK_STREAM, AF_UNIX 14:22:02
SOCK_STREAM, AF_INET 14:22:06
 
itamar 14:22:12
yes, which is why a single listenFD is insufficient	

exarkun 14:22:14
SOCK_DGRAM, AF_INET
itamar: no 14:22:17
 
itamar 14:22:55
oh?
I suppose you could pass the address family and socket type in along with the FD 14:23:17
 
exarkun 14:23:19
Argh, so many things tangled together. Give me a second to untangle them and I'll try to explain my thinking.	

itamar 14:23:47
but then you have wierd API that can accept either DatgramProtocol or factory	

exarkun 14:23:58
So, for one thing, a socket object carries sufficient information with it to figure out what to do. It tells you its address family and its socket type.	

itamar 14:24:20
ah, so you'd have socket constructed by user?	

exarkun 14:24:37
(A file descriptor doesn't carry enough information, I _think_ - it only carries its socket type)
itamar: I don't know, maybe. That's what I was thinking when I said reactor.listenFD(socket, factory), though, yes. 14:24:57
I agree that an API that takes either a factory or a datagramprotocol is not very good 14:25:11
Partly this is complicated because the existing APIs are nonsensical 14:25:29
Nothing to be done about that though, so I won't dwell. 14:26:03
reactor.listenTCP(FD(fd), ...) just seems like a hack to work around that problem 14:26:37
any API that forces you to add an `if isinstance...` is probably a bad one, right? 14:26:53
 
itamar 14:27:10
true
so, listenFD(socket, factory/datagramprotocol)... or maybe listenFD(fileno, family, type, factory/dp) 14:27:36
 
exarkun 14:27:50
The latter is pretty tempting to me
I think I may have even suggested it on some ticket at some point 14:28:00
tenth has joined the room 14:28	

exarkun 14:28:05
Probably the endpoints ticket
(minus the `fileno`) 14:28:12
 
itamar 14:28:31
how do you pass the fd in?	

exarkun 14:28:37
Still mixes factory and datagramprotocol together though	
esteve has joined the room 14:29	
gtaylor has joined the room 14:29	

itamar 14:29:12
if there's no 'fileno' argument?	

exarkun 14:29:29
eh, forget I mentioned that.  it was sort of a different feature, it just looked similar.	

itamar 14:29:29
I think mixing the two is ok, given the contraints
ah 14:29:33
so, listenFD(fileno, family, type, factory/dp)... and add this conversation to trac? 14:30:32
 
rwall 14:31:06
itamar: ok	

itamar 14:31:08
(still need to modify the various port classes, of course, and starting with just TCP support seems fine)	

exarkun 14:31:36
or land your udp.Port refactoring and then implement it for UDP first? 	

rwall 14:31:49
itamar: right.	

itamar 14:32:06
exarkun: I don't think the order really matters
so not necessary 14:32:10
 
exarkun 14:32:13
okay
in my dreams, though, one day we do things in the right order 14:32:35
 
itamar 14:32:49
well, this doesn't actually impact the state transitions
so it's mostly orthogonal 14:32:58
 
exarkun 14:33:03
it should!
I'm not sure what states ports are getting exactly, but it sounds like this starts your port in the "I have a socket but I'm not listening yet" state 14:33:32
instead of the "I have no socket and I'm not listening" state 14:33:39
 
itamar 14:34:07
eh, you could do it as "I have this fd I will listen on in the future but aren't yet"	

exarkun 14:34:11
anyway, I defer to you on that point
So as far as the changes to tcp.Port go 14:34:34
 
itamar 14:34:39
and now I really need to get ready for work	

exarkun 14:34:57
there should be no new public apis in twisted.internet.tcp (or twisted.internet.iocpreactor.tcp)
there should be no new public attributes on Ports 14:35:07
rays has joined the room 14:35	

exarkun 14:35:57
apart from that, whatever works, I guess	

rwall 14:35:58
exarkun, itamar: Thanks for your help.

[exarkun]

(In [33955]) Branching to 'tcp-port-from-fd-5248'

[exarkun]

First pass in the branch. New reactor interface with one method. Potential for supporting UDP and UNIX sockets (but not SSL, directly; that's what wrapping factories are for, though).

There's no new howto documentation yet. I think that should be added. I'm open to suggestions about where it might best fit.

We may also want another method on this interface, for importing a single existing connection into the reactor, rather than a listening port. This should be sufficient for both client and server connections, since all the SOCK_STREAMs we support are symmetric.

Soon we'll also want some endpoints associated with this kind of server setup.

[glyph]

Some  build results.

[glyph]

Looks like there are a few issues with this one.

1. I noticed some  new pydoctor errors, but rather than have you fix them, I somehow managed to convince mwhudson to close  both a very new  and a very old bug related to name references in pydoctor. Would you mind deploying trunk pydoctor to the relevant buildbot?
2. Tests are failing  all  over  the  place, but  especially  on  Windows. You should take a look at those.
3. As far as the new howto, in this case, I think maybe it actually should be in a separate ticket. Trying to explain how all the moving pieces fit together in order to use this feature is going to be pretty nerdy, and in fact, I don't even know if we should bother with narrative docs for this specific feature. I would like the narrative docs to focus on something substantially higher-level, and perhaps more portable, like a multi-process listening endpoint that uses listenSocket, rather than something so comparatively low-level. However, it's definitely not clear enough how to use it, and more documentation should be added before it's merged. So, my suggestion is this: document the heck out of the IReactorSocket interface, adding references to all relevant POSIX specifications, and explaining how the semantics of (socket) file descriptor inheritance interact with this feature.
4. Unfortunately, fromfd calls dup. I believe you're leaking file descriptors. At any rate your description in the interface docstring is wrong, since the reactor won't actually close that socket when it's finished. Fix this either in the documentation or the implementation as you see fit, but in either case, add a test to make sure that the correct file descriptors are left in the expected state. See:

   >>> import _socket as socket # just for better repr's
   >>> sk1 = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
   >>> sk2 = socket.fromfd(sk1.fileno(), socket.AF_INET, socket.SOCK_STREAM)
   >>> sk1
   <socket object, fd=3, family=2, type=1, protocol=0>
   >>> sk2
   <socket object, fd=4, family=2, type=1, protocol=0>
   

5. In posixbase, listenSocket has a @see followed by what is ostensibly the main description of the function. But the main description should come first, if not by necessity of the epytext format (I believe it is in fact required, since @see, like @param, might have multi-paragraph contents) then at least by convention.

I think this merits a re-review, but I anticipate that the next one should go pretty speedily if these issues are addressed. Thanks a lot!

[exarkun]

(In [34010]) Only implement IReactorSocket if we have socket.fromfd

refs #5248

[exarkun]

(In [34011]) Support Python 2.5 by using .args[0]

refs #5248

[exarkun]

(In [34012]) Add an example of how you might use listenSocket to the interface docstring; also mention the reasons you might want to use it, and fix the docs about the open/closed state of the input file descriptor (it is open and belongs to the application).

refs #5248

[exarkun]

(In [34013]) improve listenSocket method docstring

refs #5248

[exarkun]

(In [34014]) make the tests require that the input socket is not closed by listenSocket, and make the tests clean up that socket themselves

refs #5248

[exarkun]

Review points are addressed, I think. New  build results.

[exarkun]

Oh yea. I couldn't find pydoctor trunk, so I didn't upgrade the version on any slaves.

[exarkun]

(In [34024]) Change the interface from IReactorSocket.listenSocket to IReactorSocket.adoptStreamPort, reflecting the reality that this method can only work with SOCK_STREAM

refs #5248

[glyph]

OK. So,

1. Some conflicts will need resolving (but I anticipate these will be relatively straightforward).
2. It probably doesn't make sense to pass socketType to this API, since the behavior effectively mandates SOCK_STREAM.
3. _fromListeningDescriptor is missing a couple of @params and probably a @return, if that's important.
4. Please file a separate ticket for adopting a connected stream socket, if there isn't one already, since that use-case is also interesting and people looking for it may well end up on this ticket by accident.

Otherwise: looking good. I don't think I'm going to have a ton more to say about this, so if you have addressed to your satisfaction, feel free to land.

[exarkun]

(In [34025]) Better docs for _fromListeningDescriptor

refs #5248

[exarkun]

(In [34026]) Branching to 'tcp-port-from-fd-5248-2'

[exarkun]

(In [34029]) Move the usage docs up to the IReactorSocket docstring (from the method docstring) and add links to planned future work tickets

refs #5248

[exarkun]

(In [34031]) Merge tcp-port-from-fd-5248-2

Author: exarkun Reviewer: glyph Fixes: #5248

Add IReactorSocket and make the POSIX reactors implement it. This allows Twisted to process events on TCP ports (ie, accept connections that arrive at them), where those ports are created and initialized outside of Twisted's control.

This supports usage such as launchd and systemd socket activation, where a system daemon creates the listening socket and passes it to Twisted (via inheritance through forking).