Ticket #4697 enhancement closed fixed

Opened 4 years ago

Last modified 22 months ago

endpoint: stdio

Reported by: glyph Owned by: ashfall
Priority: normal Milestone:
Component: core Keywords:
Cc: itamar@… Branch: branches/stdio-endpoint-4697-2
(diff, github, buildbot, log)
Author: ashfall Launchpad Bug:

Description (last modified by glyph) (diff)

You should be able to run a Twisted server via a facility like inetd, or just as a subprocess for debugging.

This is a little tricky, as there can only be one, and it will interact with daemonization and other implementation details, and may need to have some nasty clever bits in order to stop the reactor when the connection is done.

I think this should be invoked as something like: serverFromString("stdio").

Change History

1

  Changed 4 years ago by glyph

  • description modified (diff)

2

follow-up: ↓ 4   Changed 4 years ago by itamar

inetd is not the only use case; one might want to e.g. run a pipeline tool (assuming ticket #3442 is fixed). #3442 might actually affect the inetd case too, actually...

3

  Changed 4 years ago by itamar

  • cc itamar@… added

4

in reply to: ↑ 2   Changed 4 years ago by glyph

Replying to itamar:

inetd is not the only use case; one might want to e.g. run a pipeline tool

absolutely!

(assuming ticket #3442 is fixed). #3442 might actually affect the inetd case too, actually...

Actually, this is totally orthogonal. #3442 is about sharing the same file description with other processes, as with a controlling terminal. But the read end and the write end of a pipe are distinct file descriptions, so you can mess with your end without disturbing your peer's view of the socket or pipe. In the inetd case, the file descriptor is all yours (plus, in that case, it's a socket, and the other end is quite possibly in somebody else's kernel).

But even in the local case the flags are independent:

>>> from os import O_NONBLOCK, pipe
>>> from fcntl import F_GETFL, F_SETFL, fcntl
>>> pipe()
(3, 4)
>>> fcntl(3, F_GETFL)
0
>>> fcntl(4, F_GETFL)
1
>>> fcntl(3, F_SETFL, O_NONBLOCK)
0
>>> fcntl(3, F_GETFL)
4
>>> fcntl(4, F_GETFL)
1

QED.

5

  Changed 3 years ago by <automation>

  • owner glyph deleted

6

  Changed 23 months ago by ashfall

  • owner set to ashfall

7

  Changed 23 months ago by ashfall

  • branch set to branches/stdio-endpoint-4697
  • branch_author set to ashfall

(In [34517]) Branching to 'stdio-endpoint-4697'

8

  Changed 22 months ago by ashfall

  • keywords review added
  • owner ashfall deleted

9

  Changed 22 months ago by ashfall

10

  Changed 22 months ago by exarkun

  • owner set to exarkun
  • status changed from new to assigned

11

  Changed 22 months ago by ashfall

  • branch changed from branches/stdio-endpoint-4697 to branches/stdio-endpoint-4697-2

(In [34615]) Branching to 'stdio-endpoint-4697-2'

12

  Changed 22 months ago by exarkun

  • keywords review removed
  • status changed from assigned to new
  • owner changed from exarkun to ashfall

Thanks. This looks pretty nice. Just a few minor points:

  1. The stdioProtocol attribute of the endpoint doesn't look like it's needed. It's also not part of the endpoint interface, so code that works with arbitrary endpoints won't be able to use it (in other words, no other endpoints have an attribute with that name).
  2. The test method docstrings should be a little more detailed about what's being tested, and not start with "Test to check". Document test methods like "The reactor passed to the endpoint is set as its _reactor attribute."
  3. There should also be a test verifying that a PipeAddress instance is passed to buildProtocol. I suggest doing this with a custom factory that records the address passed to it on an attribute of itself and then using assertIsInstance in the test to verify that object was an instance of PipeAddress.
  4. It looks like Windows has some difficulty with this code. It may be necessary to move the definition of PipeAddress out of _process.py, since that module is not importable on Windows (check out the build results on the Windows slaves for details).

Thanks again!

13

  Changed 22 months ago by ashfall

  • owner ashfall deleted
  • keywords review added

Fixed. (Hopefully the build failures as well)

 Build Results

14

  Changed 22 months ago by ashfall

I pasted build results for a wrong branch in the above comment, sorry about the same. Here's the appropriate link:  Build Results

15

  Changed 22 months ago by ashfall

  • keywords review removed
  • owner set to ashfall

Found more errors, working on it.

16

  Changed 22 months ago by ashfall

  • keywords review added
  • owner ashfall deleted

Looks better now. Sorry for all the mess.

 Build Results

17

follow-up: ↓ 18   Changed 22 months ago by exarkun

  • keywords review removed
  • owner set to ashfall

Thanks.

  1. in twisted/internet/test/test_endpoints.py, stdioFactory is misnamed according to the naming convention. Classes are named like StdioFactory.
  2. Also don't write "should" in a test method docstring.
  3. The docstring for test_protocol talks about something not very interesting. The test isn't a test for factories. It's a test for the stdio endpoint. And in particular, it's a test for the stdio endpoint using the factory to get the protocol instance to use. So the docstring should talk about that instead. We already know that Factory.buildProtocol instantiates whatever the factory's protocol attribute is set to.
  4. There are some reactor dirty warnings when the tests run on Windows. This is caused by only calling loseConnection, not waiting for the reactor to finish disconnecting the stdio connection. There are two ways to approach solving this. One would be to use a fake reactor that doesn't really establish a stdio connection. The other would be to wait for connectionLost to be called on the protocol before firing the Deferred returned to trial. If you'd rather defer this fix to another ticket, I think that's fine (just please file that ticket when you close this one).
  5. The Win32PipeAddress subclass in twisted/internet/stdio.py is unnecessary. Also, I don't quite understand how test_address passes with the implementation working this way. :) You can provide a new name for an import in a couple ways. Either PipeAddress = Win32Address or by changing the import to from ... import Win32PipeAddress as PipeAddress. This avoids creating a new class, which is wrong, because the Windows stdio implementation won't actually use the new class, it'll keep using the base class.
  6. There is one pyflakes warning for endpoints.py and one pyflakes warning for stdio.py

Thanks again

18

in reply to: ↑ 17   Changed 22 months ago by ashfall

  • keywords review added
  • owner ashfall deleted

Replying to exarkun:

1. There are some reactor dirty warnings when the tests run on Windows. This is caused by only calling loseConnection, not waiting for the reactor to finish disconnecting the stdio connection. There are two ways to approach solving this. One would be to use a fake reactor that doesn't really establish a stdio connection. The other would be to wait for connectionLost to be called on the protocol before firing the Deferred returned to trial. If you'd rather defer this fix to another ticket, I think that's fine (just please file that ticket when you close this one).

I'll file a new ticket once this is ready to merge.

19

  Changed 22 months ago by exarkun

  • keywords review removed
  • owner set to ashfall

Thanks! A couple more tiny points:

  1. There's a pyflakes warning in test_endpoints.py
  2. StandardIOEndpoint.listen's docstring appears to be truncated :) Please make it a bit more specific, like the docstrings for other listen implementations.

Once those are addressed, please merge. Thanks again.

20

follow-up: ↓ 22   Changed 22 months ago by ashfall

Filed #5629 for the string description plugin, and #5630 for the dirty reactor warnings on Windows.

21

  Changed 22 months ago by ashfall

  • status changed from new to closed
  • resolution set to fixed

(In [34653]) Merge stdio-endpoint-4697-2: An endpoint interface that listens on stdin/stdout

Author: ashfall Reviewer: exarkun Fixes: #4697

Added StandardIOEndpoint to twisted/internet/endpoints.py, along with corresponding tests in twisted/internet/test/test_endpoints.py

22

in reply to: ↑ 20   Changed 22 months ago by ashfall

Replying to ashfall:

Filed #5629 for the string description plugin, and #5630 for the dirty reactor warnings on Windows.

Err, I mean #5729 and #5730 respectively.

Note: See TracTickets for help on using tickets.