#8146 #8147 Support systemd named inherited file descriptors #11691
Conversation
exarkun
force-pushed
the
8147-named-systemd-file-descriptors
branch
from
4243f7f
to
ba3ae17
Compare
exarkun
changed the title
There was a problem hiding this comment.
I left a few comments.
The code looks good and as long as the tests are green it can be merged.
I am doing these reviews in "emergency" mode, trying not to block the work from @exarkun ... but I am always tired after work and not in the best shape for review.
Doing proper reviews requires a full time job, not after hours work.
Jean-Paul, I will accept this and put it back for review.
If no other Twisted dev will have time for it, I think is best to merge it.
Thanks
docs/core/howto/systemd.rst
Outdated
| @@ -282,7 +282,7 @@ ExecStart | |||
|
|
|||
| The ``domain=INET`` endpoint argument makes ``twistd`` treat the inherited file descriptor as an IPv4 socket. | |||
|
|
|||
| The ``index=0`` endpoint argument makes ``twistd`` adopt the first file descriptor inherited from ``systemd``\ . | |||
| The ``name=www`` endpoint argument makes ``twistd`` adopt the file descriptor inherited from ``systemd`` named ``www``. | |||
There was a problem hiding this comment.
I don't know where to comment.
But at line 261 we have
This configuration file contains the following important directives:
I think FileDescriptorName should be in the list.
Also at line line 272 I think the following diff is requried
-You also need to modify the ``systemd.service`` file as follows:
+You also need to modify the ``www.example.com.service`` file as follows:And besides NonBlocking I think we also make sure the service is configured with Accept=false
Maybe false is the default value, but I think is important to make sure systemd will not spawn a separate twistd process for each connection, and will let twistd to handle the lower level networking.
| """ | ||
| items = list(expected.items()) | ||
| names = [name for name, _ in items] | ||
| descriptors = [fd for _, fd in items] |
There was a problem hiding this comment.
this is ok...but when you are tired, this is harder to read, in comparison with passing a static input data.
just some feedback about hypothesis
I try to have the test code as simple as possible...as when I am refactoring something, I will see other tests failing, and then I need to read the test to see if the test is valid, or if there was a bug in the previous test itself.
|
just asking a review from another team member to make sure I don't miss anything needs-review |
|
I think that if no other review is made in the next few days, is better to merge this. I think this is already a bit of improvement over trunk. Thanks for the docs updates. All the comments that I left are minor and should not block the merge. |
docs/core/howto/listings/systemd/www.example.com.socketactivated.service
Outdated
Show resolved
Hide resolved
Co-authored-by: Tom Most <twm@freecog.net>
Co-authored-by: Tom Most <twm@freecog.net>
Scope and purpose
Fixes #8146
Fixes #8147
The unit tests for systemd.py were well-intentioned but sadly misguided (nice try though, jean-paul@2012). So I kind of rewrote them (a lot of the tests are the same but the infrastructure they fit into is better now).