client endpoint: process

[Glyph]

It should be possible to make a client endpoint like this:

clientFromString('process:imapd')

which would run the imap daemon in a subprocess and talk to it directly, rather than connecting to it over TCP.

As a sub-task (possibly big enough for a separate ticket) we should have one, official, blessed transformer that adapts from a process transport to a regular stream transport.

See #4695 and #3997

[ashfall]

(In [34639]) Branching to 'child-processes-endpoint-4696'

[Jean-Paul Calderone]

Thanks!

1. There are some conflicts. Please merge forward and resolve them.
2. There are some missing features:
   1. IProtocol.makeConnection needs to be called when the connection is established. This is going to be a job for _WrapIProtocol.
   2. IProtocol.dataReceived needs to be called sometimes! :) Another job for _WrapIProtocol.
   3. The protocol needs to be given a transport that implements ITransport, with its various methods - write, writeSequence, loseConnection, getHost, getPeer, etc (check the interface for the complete list).
3. It's probably a good idea to just refer to IReactorProcess.spawnProcess for the parameter API documentation, rather than reproducing all of it in ProcessEndpoint.__init__'s docstring (fewer places to update if we make improvements or other changes).
4. This assertion in the tests isn't useful: self.assertEqual(self.ep._wrappedPP.processEnded(connectionDone), self.ep._wrappedPP.protocol.connectionLost(connectionDone)). What this proves is that the return value of _wrappedPP.processEnded is equal to the return value of protocol.connectionLost. What you really want to say is that if processEnded is called on the wrapper, connectionLost gets called on the application protocol. The way to go about this is the same as how you did test_spawnProcess - define the method that's going to get called, implement it to save its arguments somewhere, and then verify those arguments after you expect it to have been called (this is what you'll want to do with makeConnection and dataReceived, too - makeConnection is half done for you, since the base Protocol implements that to save its argument as the transport attribute of the protocol instance).
5. self._protocol and self._wrappedPP should go away. They don't make sense, given that connect can be called multiple times.
6. _WrapIProtocol.processEnded needs to fiddle with the reason argument a little bit. Process protocols get handed a reason like ProcessDone or ProcessTerminated. ProcessDone with a status of 0 means "clean exit" (by POSIX convention, anyway). "clean exit" is sort of like twisted.internet.error.ConnectionDone. ProcessDone with any other status, or ProcessTerminated, carries some extra information about potential errors. I'd like to give you definitive instructions on what to do with them, but I'm not quite sure right now. One option would be to turn them into ConnectionLost, which is how IProtocol gets told the connection did not end "cleanly" (ie, data may have been lost). On the other hand, ConnectionLost doesn't convey as much information as a ProcessTerminated instance would. So another option would be to just pass the original exception through in these error cases. That might be surprising though, since IProtocol will never be given that kind of exception in any existing cases. That might be fine though, since this is a new API. It might be a good idea to solicit further feedback on IRC about opinions on this.
7. Some of the ProcessEndpoint parameters might not make sense. For example, a protocol only has one input (protocol.dataReceived) and one output (transport.write). childFDs can be used to set up multiple inputs and outputs, though. It's not clear how data would move around in that case. Perhaps we can discuss which of these can work at our meeting this week.

Child processes are complicated and subtle, so I'm not surprised at all that this branch needs some more work. It's a great start though. Thanks!

[ashfall]

(In [34681]) Branching to 'child-processes-endpoint-4696-2'

[ashfall]

​New build results

Addressed the review comment.

[Jean-Paul Calderone]

Thanks! Here's some more feedback:

1. twisted/internet/test/test_endpoints.py
   1. _TestProtocol and _TestFactory need documentation and probably better names. Also note that _TestFactory could almost as easily be replaced by just a Factory instance with its protocol attribute set.
   2. On _TestProtocol, dataReceived and connectionLost aren't allowed to return anything except None, so _TestProtocol doesn't behave like a nice protocol. It looks like these methods return their argument so that the value can be checked later on. The way to do this is to save the argument as an attribute on the instance and then check the attribute later on.
   3. _FakeProcessTransport also needs documentation. Also, a convention Twisted is working on adopting is using "Memory" in the name for this kind of fake, instead of "fake".
   4. _FakeProcessTransport.writeToChild ignores the childFD argument. This means the tests don't demonstrate data is ever being written to the child's stdin. It could be written to any other file descriptor without the tests realizing it.
   5. True isn't a useful return value for any of the other methods on _FakeProcessTransport.
   6. _FakeProcessReactor needs documentation.
   7. _FakeProcessReactor.spawnProcess should not be setting any attributes on the process protocol passed to it. It *may* want to call makeConnection on it, but I'm not sure it even needs to do that. Setting of this attribute doesn't seem to be relied on anywhere, so it probably just makes sense to delete it. Here's where you might want to return that _FakeProcessTransport instance though.
   8. ProcessEndpointsTestCase is a little bit confusing/confused. Is it a container for tests for the endpoint, for _WrapIProtocol, for the fake transport implementation? It contains tests for all of these things. I think the test method organization needs to be cleaned up a bit. Have one TestCase subclass for _WrapIProtocol behavior. Have another for endpoints._ProcessEndpointTransport. Have a third for the endpoint itself.
      1. test_writeToChild, in particular, is a unit test only for testing infrastructure. It definitely belongs separated from tests for the public endpoint APIs and functionality.
   9. ProcessEndpointsTestCase.setUp creates a ProcessEndpoint with a nonsense executable - a _FakeProcessTransport instance. The executable is supposed to be the filesystem path to the program to run.
   10. test_wrappedProtocol still doesn't assert something useful. We might have talked about this on IRC, but I guess I didn't write about it in my last review: it's not very interesting that there is a _wrappedPP attribute or that it is an instance of ProcessProtocol. It's slightly more interesting to assert that it's an instance of _WrapIProtocol, so you could update the test to do that.
   11. test_spawnProcess is a good start. However, neither it nor any other tests actually pass any interesting values to the endpoint. Nearly everything ends up with the default values. The test suite would benefit from an additional test that exercises support for passing each of these additional parameters.
   12. ProcessEndpointTransportTests's docstring says it is for testing _WrapIProtocol, but it seems to be for testing endpoints._ProcessEndpointTransport.
   13. test_ProcessEndpointTransportInstance is misnamed. Also, I don't think either of its assertions are very useful. The first assertion, at least, is entirely redundant: it must be an instance of that type, because setUp made it so.
   14. test_loseConnection, test_getHost, test_getPeer, should all be a bit different. True isn't a meaningful return value for any of those methods.
   15. WrappedIProtocolTests.setUp also constructs a endpoints.ProcessEndpoint with a nonsense executable.
   16. test_makeConnection should not call self.ep._wrappedPP.makeConnection. Instead, either the fake reactor should call makeConnection on the process protocol passed to it, or the fake reactor should remember the protocol passed to it and the test method should that memory (ie an attribute) to find the object to call makeConnection on. Calling it on self.ep._wrappedPP is unrealistic, since in reality makeConnection will only be called on the object passed to spawnProcess, not on a private attribute of the endpoint. It's true that they're the same object in this case, but there's no unit tests demonstrating this fact.
   17. The next line after the makeConnection call, self.assertEqual(self.ep._wrappedPP.protocol.transport, self.ep._wrappedPP.transport), has too many .s in it. Use a more local reference to the application protocol (ie, the object referred to by self.ep._wrappedPP.protocol): the test created it (by way of _TestFactory), the test should be able to get it more easily.
   18. test_logStderr calls log.addObserver. Use TestCase.addCleanup to undo this. It's very important to undo addObserver calls, since they affect global mutable state and typically come with a significant cost. :)
   19. Same goes for the other tests that call addObserver.
   20. test_stdout runs into the same problem with values being returned from the protocol's dataReceived. It needs to check instance state, not try to pass the data back via return values.
   21. test_processDone and test_processEnded might want to use Failure.check. And it needs to not use the return value trick.

I think I'll leave the comments at this for now, and let you find any necessary improvements in the implementation as a consequence of improving the tests (since the goal with test driven developments is for the tests to guide you to a good implementation :).

Thanks!

[ashfall]

Fixed the issues raised in the review (and some extra stuff that needed fixing).

[Jean-Paul Calderone]

1. twisted/internet/process.py
   1. There's some new trailing whitespace in this file.
   2. The docs for _ProcessEndpointTransport.__init__ repeat the same information twice. The @param should explain more about the object (or be omitted). For example, I'd write something like "An active process transport which will be used by write methods on this object to write data to a child process."
   3. twisted.python.components.proxyForInterface will probably make _ProcessEndpointTransport shorter by removing the proxying boilerplate for loseConnection, getHost, and getPeer.
   4. The errFlag accepted by _WrapIProtocol and the endpoint class isn't expandable. True and False have meanings now, and there are no other possible values for a boolean. How about twisted.python.constants.Names to define some constants for this?
   5. The transport attribute of _WrapIProtocol is undocumented and also unused, except for the line immediately following its definition. It can probably be removed entirely.
   6. _WrapIProtocol's childDataReceived and processEnded methods are undocumented.
   7. Failure.check is a better choice than type(someFailure.value) == SomeException.
   8. ProcessEndpoint.connect's docstring is a little light on details. How about "Launch a child process and connect it to a protocol created by C{factory}."? The current docstring doesn't say much that a reader couldn't guess. The docstring should try to convey extra information that the reader wouldn't know already.
   9. ProcessProtocol.connect returns a Deferred that fires with the wrong object. IStreamClientEndpoint.connect is meant to return a Deferred that fires with the protocol that has been connected.
   10. For the stderr logging case, log.msg(data) is basically correct. However, two things:
       1. We're trying to move towards "structured" logging. This means that any time you log something, it should probably look more like this: log.msg(format="foo %(bar)s baz", bar=quux). This makes quux available as part of the event dictionary, should an observer be interested in it specifically.
       2. The extra formatting also makes it easier for someone reading the log to understand what the log message means. In this case, I suggest identifying the child process executable and the application protocol associated with it, in addition to the data itself. So, log.msg(format="Process %(executable)r wrote stderr unhandled by %(protocol)s: %(data)s", executable=..., protocol=fullyQualifiedName(protocol), data=data).
2. twisted/internet/test/test_endpoints.py
   1. Some new trailing whitespace to clean up.
   2. Call addCleanup as soon as possible after doing the thing that's being cleaned up. Any exception that happens in between those two actions prevents the cleanup from being done.
   3. Try removing each occurrence of self.ep._wrappedPP from the unit tests. Actually, try removing the _wrappedPP attribute from the implementation of the endpoint entirely. You don't need it, and the tests will be better without it.
3. I broke the implementation of ProcessEndpoint in r34893. Try changing the tests or adding new ones to uncover the brokenness and then fixing it.

Thanks!

[ashfall]

Replying to exarkun:

1. twisted/internet/process.py

I think you mean twisted/internet/endpoints.py. :)

Made the suggested changes. ​New build results.

[ashfall]

Filed #5813 for the string description plugin for this endpoint.

[Jean-Paul Calderone]

Thanks!

1. twisted/internet/test/test_endpoints.py
   1. What is ProcessEndpointsTestCase.test_errFlag testing?
   2. The order of events in test_wrappedProtocol doesn't make sense. Either wpp ought to be looked up inside checkPPWrapper or the assertion should be made outside of checkPPWrapper. Splitting up the two parts of that assertion doesn't benefit anything.
   3. WrappedIProtocolTests.test_makeConnection ends up calling _WrapIProtocol.makeConnection twice. It shouldn't.
   4. There appears to be no unit test that covers the result of the Deferred returned by ProcessEndpoint.connect.
   5. WrappedIProtocolTests.test_processEnded should use ProcessTerminated instead of ConnectionLost. Only ProcessEnded and ProcessTerminated will ever be passed to the processEnded callback.
2. twisted/internet/endpoints.py
   1. There's a conflict here in the __all__ section that needs to be resolved
   2. Whitespace on the line importing ProcessProtocol needs to be fixed
   3. There's a spurious whitespace change to the implementation of StandardIOEndpoint.listen
   4. The process attribute of _ProcessEndpointTransport should be private
   5. The documentation for errFlag in _WrapIProtocol.__init__ still suggests it takes a boolean value
   6. _WrapIProtocol.childDataReceived and processEnded need docstrings
   7. The log.msg call in childDataReceived should use the format keyword argument to log.msg (example in one of my above comments)
   8. processEnded should use Failure.check
   9. _ERRFLAG_VALUE almost certainly needs to be public, otherwise it will be difficult for anyone to pass values from it to ProcessEndpoint. I suggest a name more like StandardErrorBehavior, with attributes like LOG and DROP/IGNORE.
   10. ProcessEndpoint.__init__ documents _spawnProcess with @ivar. @ivar only belongs in class docstrings (where this belongs, since _spawnProcess is not an __init__ parameter).
   11. The use of try/except and defer.execute and defer.succeed in ProcessEndpoint.connect is problematic. Add a test for that function's behavior in an error case - for example, pass object() as the executable to launch. connect should return a Deferred that fails, but it won't.

[ashfall]

(In [35108]) Branching to 'child-processes-endpoint-4696-3'

[ashfall]

​New build results.

Addressed the review points, fixed all but 2.11, as discussed on IRC.

[Jean-Paul Calderone]

2.11 still needs to be addressed. Our conversation on IRC was about how you can't provoke the error by passing a bad value for executable. That doesn't mean it's impossible to provoke the error. Passing a bad value in the arguments list will provoke it against a real reactor. Since the tests use fakes, you'll need a fake that can somehow raise an exception (this should be very simple, since the fake is completely under the control of the test).

[ashfall]

Replying to exarkun:

2.11 still needs to be addressed. Our conversation on IRC was about how you can't provoke the error by passing a bad value for executable. That doesn't mean it's impossible to provoke the error. Passing a bad value in the arguments list will provoke it against a real reactor. Since the tests use fakes, you'll need a fake that can somehow raise an exception (this should be very simple, since the fake is completely under the control of the test).

I misinterpreted. Fixed now.

​Build results

[Jean-Paul Calderone]

Thanks! Here's a couple points:

1. The branch deletes the TCP server endpoints from the __all__ definition for twisted.internet.endpoints.
2. Missing test coverage for _WrapIProtocol.makeConnection

Can't finish this right now, unfortunately.

[ashfall]

(In [35758]) Branching to 'child-processes-endpoint-4696-4'

[ashfall]

Replying to exarkun:

Thanks for the review.

Thanks! Here's a couple points:

1. The branch deletes the TCP server endpoints from the __all__ definition for twisted.internet.endpoints.

Fixed in the new branch.

2. Missing test coverage for _WrapIProtocol.makeConnection

I think I messed something up while fixing 1.3 from your ​previous review in twisted.internet.WrappedIProtocolTests.test_makeConnection.

Something's giving me a false positive in this test, or leading me to believe makeConnection is called, when in fact it isn't?

[ashfall]

Replying to ashfall:

Replying to exarkun:

Thanks for the review.

Thanks! Here's a couple points:

1. The branch deletes the TCP server endpoints from the __all__ definition for twisted.internet.endpoints.

Fixed in the new branch.

2. Missing test coverage for _WrapIProtocol.makeConnection

I think I messed something up while fixing 1.3 from your ​previous review in twisted.internet.WrappedIProtocolTests.test_makeConnection.

Something's giving me a false positive in this test, or leading me to believe makeConnection is called, when in fact it isn't?

So fixed this, but I still think something's not right with test_makeConnection. It passed even when makeConnection wasn't being called. Need to figure out _what_.

[Jean-Paul Calderone]

Once #6236 is resolved, I hope there will be less tedious subversion junk in the way of reviewing this.

[Jean-Paul Calderone]

(In [36704]) Branching to 'child-processes-endpoint-4696-5'

[Jean-Paul Calderone]

Hello again! With #6236 resolved, there were indeed many fewer merge conflicts. There were still a couple simple ones, so I merged the branch forward once more and resolved them myself. I also switched a couple uses of implements to implementer in r36706.

Some more comments about the code:

1. There are a few more Python 3 compatibility changes necessary. These are mostly trivial (though unfortunately tedious, sorry):
   1. twisted.internet.endpoints may not import any modules that haven't been ported to Python 3 (since twisted.internet.endpoints itself is expected to work on Python 3 now). The easiest solution to this is to take the new imports of twisted.python.constants and move them into the if not _PY3: block. Since there are module-scope uses of the names imported from twisted.python.constants, you probably also need to define some dummy versions in an else block for the if not _PY3:. Since StandardErrorBehavior will get deleted when _PY3 is true, these dummy versions don't need to work, they just need to let the code get past the definition of StandardErrorBehavior. Collectively, this is the approach where you do not make this new functionality available on Python 3, and so you also do not add "ProcessEndpoint" or the other related names to __all3__. The harder solution is to actually make ProcessEndpoint work on Python 3 which involves first porting its dependency, twisted.python.constants. I recommend saving this task for later, after this ticket has been resolved.
   2. Since the unit tests won't pass if ProcessEndpoint is deleted, at the end of test_endpoints.py you need to add the test cases being added by this branch to the delete statement, so they won't run on Python 3.
2. There are a few pyflakes errors in twisted/internet/test/test_endpoints.py.
3. There are some boring doc/formatting issues. See r36707 where I fixed them (easier to explain them that way than here)
4. StandardErrorBehaviors should document its attributes in its class docstring (@cvar)
5. MemoryProcessReactor.spawnProcess fails to instantiate MemoryProcessTransport. It just uses the class. This seems not to cause any tests to fail because the transport is never actually used when MemoryProcessReactor is used to set it up. _ProcessEndpointTransport is already tested by ProcessEndpointTransportTests so it's probably fine that the transport set up by MemoryProcessReactor.spawnProcess is never actually used. I suggest using object() as the transport, though, to avoid giving the impression that this code is actually exercised.

I forced a build on the new branch, <​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/child-processes-endpoint-4696-5>, that will point out a few other issues to take care of (don't worry too much about osx10.6-py2.6-select, it seems to be running on failing hardware). Do note the new twistedchecker builder, though, which checks for compliance with various (mostly stylistic) coding standard points.

Thanks.

[ashfall]

Thanks! Fixed, ​new builds.

There's ​one twistedchecker error that looks like a bug in twistedchecker.

[Tom Prince]

Sorry for the delay in getting to this, and thanks for your persistence.

1. ProcessEndpointsTestCase
   1. It isn't clear that test_errFlag is testing anything useful. test_constructorDefaults already tests that self.ep._errFlag is a specific constant in StandardErrorBehavior. So this test can probably be deleted.
   2. Since you already have a fake IReactorProcess, you could just have that store the arguments passed to spawnProccess to test in test_spawnProcess, rather than having _spawnProcess which is only used in tests.
2. ProcessEndpointTransportTests
   1. The docstrings for test_getHost and test_getPeer don't describe what the test actually asserts. Also, as written, the functions on _ProcessEndpointTransport could call the reverse functions on the protocol, and the tests wouldn't notice.
3. WrappedIProtocolTests
   1. test_logStderr should probably check the format as well (otherwise, it could be removed, which would mean that by default nothing gets written to a log file).
   2. The docstring for test_processEnded` is no longer accurate.

Please resubmit for review after addressing the above. One additional point that you may want to consider:

4. There is now good support for testing things using deferreds without returning them from test methods. Specifically successResultOf, failureResultOf, and assertNoResult. Since you are using an in-memory reactor for everything, the tests could be improved by using these throughout.

[therve]

Things fixed, thanks!

[Tom Prince]

A couple of minor nits:

• ProcessEndpointsTestCase.test_processAddress: Having an accessor getAddress seems gratuitous.
• Rather than accessing self.ep._reactor, we should save the reactor on self.
• What happens if log.msg is called in tests? Should it fail (in _logMsg)?

Please merge after addressing the above points.

[Tom Prince]

Actually, ProcessAddress is going to be public, and I think it eventually wants to have more attributes (and thus more constructor arguments). So that should be done before merging.

For the peer address, I think it wants at least the executable and arguments. And perhaps also the pid. I'm not sure about for the host address. Probably the pid, and probably a different type.

[Jean-Paul Calderone]

Actually, ProcessAddress is going to be public, and I think it eventually wants to have more attributes (and thus more constructor arguments). So that should be done before merging.

Or it could be private for now and we can fix this later.

[Glyph]

Replying to exarkun:

Actually, ProcessAddress is going to be public, and I think it eventually wants to have more attributes (and thus more constructor arguments). So that should be done before merging.

Or it could be private for now and we can fix this later.

+1. While it's important for the transport to satisfy its interface and return an IAddress of some kind, I can't think of any reason application code would particularly want to construct these. Making it public later is easy.

[therve]

(In [37840]) Merge child-processes-endpoint-4696-5

Authors: ashfall, therve Reviewers: exarkun, tom.prince Fixes: #4696

Add a ProcessEndpoint facility to easily connect a protocol to a process.

[Tom Prince]

(In [37849]) Revert r37840: Merge child-processes-endpoint-4696-5.

Reopens: #4696

There is a failure on the python3 buildbot. twisted.internet.test.test_endpoint imports StandardErrorBehavior unconditionally, although it isn't supported on python3.

[therve]

Sorry about that. It seems to work on Python 3 though, so I fixed the imports.

Build results here: ​http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/child-processes-endpoint-4696-5

[Tom Prince]

twisted on python3 doesn't support processes so I don't think having the endpoint is useful, or warranted.

[therve]

Fair enough, it should be fixed now.

[Glyph]

Replying to tom.prince:

twisted on python3 doesn't support processes so I don't think having the endpoint is useful, or warranted.

It might not be useful, but it's warranted because we shouldn't be creating additional work for future porters. It might be OK to defer the porting work to the point where processes get ported, but if we can avoid that without putting in too much effort, then let's.

[Glyph]

Glancing over the build results, it looks like "new pydoctor errors" builder still doesn't work right. These don't look like errors relevant to this branch: ​http://buildbot.twistedmatrix.com/builders/documentation/builds/3139/steps/api-documentation/logs/new%20pydoctor%20errors

[Tom Prince]

(In [38049]) Branching to child-processes-endpoint-4696-6.

[Tom Prince]

(In [38050]) Merge forward.

Refs: #4696

[Tom Prince]

​build results

glyph: StandardErrorBehavior uses twisted.python.constants which isn't ported.

This looks good. Please merge.

[therve]

(In [38053]) Merge child-processes-endpoint-4696-6

Authors: ashfall, therve Reviewers: exarkun, tom.prince Fixes: #4696

Add a ProcessEndpoint facility to easily connect a protocol to a process. This new merge fixes a compatibility issue with Python3.