Add a server endpoint for listening on a file descriptor inherited from systemd

[exarkun]

systemd supports a form of "socket activation" that is becoming popular in the initd-replacement world. This feature involves systemd creating a socket, binding it to a suitable address, and marking it as listening. It does this all according to systemd-specific configuration.

The file descriptor is then passed on to another process (typically only when the first connection arrives).

An endpoint (and string endpoint description plugin) which allows Twisted servers to listen on such a file descriptor would be handy.

[exarkun]

(In [34032]) Branching to 'systemd-5575'

[exarkun]

[Build results  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/systemd-5575] will hopefully be good. The code here is pretty simple, all the hard stuff was done by #5248.

I tried adding some more endpoint docs, covering existing stuff, not just this new thing. Not sure if I did a very good job, feel free to ask for better.

[exarkun]

(In [34048]) Make sure the socket is non-blocking after we take it from systemd

refs #5575

sorry for the horrible buggy untested code, but it's better now, promise

[glyph]

Thanks for doing this.

1. _serverParsers is a gross hack that should not be expanded upon. Sorry for not documenting it that way. It exists only to support the (now deprecated, always a bad idea) strports.parse function. If you look at how the (also deprecated) strports.listen function works, I think it'll be clear why nothing further should be added to this list.
   1. It would also be great to have an example within Twisted of an IStreamServerEndpointStringParser plugin, so fixing this to be a normal plugin would be good.
   2. Furthermore there would be an additional benefit that _parseSystemd would not need to be in the middle of the generic endpoints code, since it's not really generic.
2. As I've recently discovered, it's possible to automatically detect the correct value for the 'domain=' argument and not let the user screw it up. However, this is a whole new funhouse of implementation/dependency issues, so feel free to make it a separate ticket.
3. I'm tempted to say that it should be domain=AF_INET6, not domain=INET6, so that it's easier for someone to look up with a search. If you think that's a good idea, change it, otherwise, feel free to leave it.
4. There's maybe a more generic endpoint in here trying to get out; adopt: or somesuch. Separate ticket though, I think. It would just need a different parser.
5. Or... would it? I'm not thrilled with the idea of the endpoint parser calling dup(), reading the systemd environment, setting the file description to be non-blocking, etc. The parser should just parse, and the listen() method should do the rest. Unless there's some reason that it absolutely needs to be read immediately, but it looks like you're not supposed to daemonize anyway with systemd.
6. twisted.python.systemd
   1. I love the idea of a verified fake. But, I love the idea of not having a fake at all even more. Rather than having Environment inherit from Simulation and change the signature of __init__, why not change Simulation to ListenFDs, and then have ListenFDs.fromEnvironment(environ=...) (IMHO, you don't need start=.  `SD_LISTEN_FDS_START` is 3.) If you instantiate ListenFDs directly, then it can just do what the fake does now.)
   2. @param start: @param environ: sounds like it could have a little more to say on the subject.
7. Thanks for adding more documentation, but given how much crud you need to write to create a systemd service, I think that a "Deploying Twisted with SystemD" document is warranted, that references the endpoint, the fact that you don't want to daemonize (but you do want a log file?), how to write the .socket file, the .service file, and whatever else you need to do. I wouldn't block this ticket on such documentation, because the stuff you've added is sufficient to understand the feature if you know what you're looking for. But, our aspirations for documentation we shouldn't stop at "sufficient to understand"; and, deploying Twisted should be better documented, in general.
   1. The documentation is somewhat lacking. For example, the UNIX endpoint doesn't document the format of the 'mode' argument (is it decimal? hex? octal? rwxr-xr-x?) in either its documentation or its example.
8. In AdoptedStreamServerEndpoint, you document a bunch of private @ivars, but you don't document a bunch of public __init__ arguments. We do this all over the place (technically it's the policy) but I think the policy is backwards. Document the __init__, it's public; if you're not going to document something redundant, don't document the private attributes. In this case though, I don't feel like the attributes even need to be private, and frequently having a reactor attribute be public is a better idea than not.

[exarkun]

Thanks!

1. Changed to be a plugin in r34058
2. Yea, this would be nice. Since we can make domain optional later, I think it's appropriate to leave this as is for now. Filed #5599.
3. I like INET6, and a couple people on IRC agreed too. AF_INET6 is C legacy junk. Also, INET6 is pretty googleable.
4. Maybe! I think that there will be specific support for launchd and upstart, in addition to this. I suppose there are other places you can inherit a descriptor from (like, maybe, Twisted? Although hopefully we'll have sendmsg/recvmsg based support for that, which is probably usually nicer). Filed #5600.
5. Err. Well. IReactorSocket.adoptStreamPort is the thing that does the dup(). So I'm not sure what you mean here. The parser is still doing the non-blocking call. I can move it if you think it really matters, I guess.
6. Fixed in r34049
7. Yea. I don't actually know systemd configuration well enough to write that document. It would be really great if someone wrote it, though. Filed #5601.
   1. Fixed in r34059
8. Fixed in r34050

[glyph]

New  build results available shortly.

[glyph]

Thanks for your response. Looks like we're in the home stretch.

Regarding point 3, fine, whatever :).

Regarding point 4, yes, I think that in some cases it will make sense to accept() from multiple Twisted instances simultaneously, and that's where we'd want such an endpoint. In others singlea parent accept()-ing process makes more sense.

Regarding point 5, sorry about saying dup. I really am more concerned about the setNonBlocking. Why bother doing it in the parser? It sets a bad example (parsers shouldn't do any I/O) and it's not like it's valid for any other AdoptedStreamServerEndpoint socket to be blocking. I still think this should be changed. (Although, honestly, more for the example it sets than for any particular detrimental effect this specific example of I/O in a parser will have.)

1. Please make sure that  whatever is going on on the documentation builder is not actually the fault of this branch. I am concerned that it is, and that your new docs are doing something weird. I haven't figured out what; hopefully you can reproduce it locally or on baelnorn or something. (We should never, ever let anything like what the documentation builder does be deployed to a buildbot again. If somebody wants a tool like that, they check it into SVN.)
2. There are currently no tests, and completely unhelpful behavior, if you use this endpoint incorrectly from the command line. For example, if you don't actually set up systemd properly to send you a socket, you will get a "list index out of range" exception. If you specify an invalid address family, you will get an AttributeError. (Also, you could specify AF_UNSPEC or AF_IPX and I have no idea what that would do.) Now, this is, quite unfortunately, no worse than the existing endpoints, but it is perhaps even easier to screw up. You can also move this out to a separate ticket if you want, since a user-friendly interface is a bunch of additional effort, but let's at least stop forgetting to make our user-facing tools at least marginally helpful when reporting errors. If you're feeling especially generous, you could file some tickets for the user-friendliness of other endpoints as well, maybe an official error-reporting channel for the plugin interface, even.
3. SURPRISE!  WINDOWS!!!!.

At this point, we've got an implementation which behaves well enough, and we've got docs, so you can land it assuming you can deal with points 1 and 3 in this new review well enough. But please deal with point 2 in a timely manner following it, if possible.

[exarkun]

(In [34063]) Attempt to get the tests "working" (ie, skipping) on Windows

There is no AF_UNIX on Windows.

refs #5575

[exarkun]

(In [34064]) MORE EMPHASIS

refs #5575

This makes latex happy. Do not ask me why.

[exarkun]

Thanks!

1. Addressed in r34064
2. Filed #5604, #5605, #5606, #5607, and I see you filed #5608.
3. Addressed in r34063

If  latest builds look good, I'll merge.

[exarkun]

(In [34065]) Use a pipe here, which should always support being set to non-blocking (apparently /dev/null does not, at least on FreeBSD)

refs #5575

[exarkun]

Regarding point 5, sorry about saying dup. I really am more concerned about the setNonBlocking. Why bother doing it in the parser? It sets a bad example (parsers shouldn't do any I/O) and it's not like it's valid for any other AdoptedStreamServerEndpoint socket to be blocking. I still think this should be changed. (Although, honestly, more for the example it sets than for any particular detrimental effect this specific example of I/O in a parser will have.)

Argh. I agree that the parser is not a good place for this. I put it there because IReactorSocket.adoptStreamPort assumes the file description given to it is already non-blocking. Consequently AdoptedStreamServerEndpoint assumes the same thing. The parser is the only place left. I'm not sure why I'm so averse to throwing a potentially redundant setNonBlocking into one or the other of those two APIs. Perhaps I should get over it?

[glyph]

Replying to exarkun:

Regarding point 5, sorry about saying dup. I really am more concerned about the setNonBlocking. Why bother doing it in the parser? It sets a bad example (parsers shouldn't do any I/O) and it's not like it's valid for any other AdoptedStreamServerEndpoint socket to be blocking. I still think this should be changed. (Although, honestly, more for the example it sets than for any particular detrimental effect this specific example of I/O in a parser will have.)

Argh. I agree that the parser is not a good place for this. I put it there because IReactorSocket.adoptStreamPort assumes the file description given to it is already non-blocking. Consequently AdoptedStreamServerEndpoint assumes the same thing. The parser is the only place left. I'm not sure why I'm so averse to throwing a potentially redundant setNonBlocking into one or the other of those two APIs. Perhaps I should get over it?

I think so, yes.

I suspect your impulse to avoid it (as mine might be) is hypothetically inefficient, because you'll be making extra syscalls that might not have been be necessary.

Points against this impulse, though:

• there's currently no way to actually invoke this logic without making those syscalls anyway.
• it's only hypothetically inefficient. no benchmark, no profiling information indicates it would be a problem.
• it seems highly unlikely that this code would ever make it into the critical path of anything.

So, if it were ever to become an issue in the future, we could add an __init__ argument to AdoptedStreamServerEndpoint like alreadyNonBlocking. But I would lay money that it will never be ;).

[exarkun]

(In [34068]) Mark the file description non-blocking in the endpoint's listen method, not the string endpoint description parser.

refs #5575

[exarkun]

(In [34076]) Minor API doc correction

refs #5575

[exarkun]

(In [34077]) Merge systemd-5575

Author: exarkun Reviewer: glyph Fixes: #5575

Add a server string endpoint description parser plugin for listening on a port inherited from systemd.