Opened 16 years ago

Last modified 9 years ago

#1939 enhancement new

improve process module with generic support for pipes, PTYs, socketpairs etc.

Reported by: naked Owned by: naked
Priority: normal Milestone:
Component: core Keywords:
Cc: Jean-Paul Calderone, jknight, Manish Branch:
Author:

Description

The process module could be improved in a number of ways.

It should be possible to use an arbitrary number of reader pipes, writer pipes, PTYs, socketpairs and other communication channels to a process. These communication channels should be freely assignable to any set of child filedescriptors.

It should be possible to somehow assign separate protocols for communication channels. For example, it should be possible to use NetstringReceiver for stdin/stdout and LineReceiver for stderr for the same process.

It should be possible specify a preexec function or a list of preexec functions to execute after forking but before execing the child. For example, this is useful for a combination of chroots and chdirs before execing the child.

Attachments (2)

twisted-process-improvements.patch (16.4 KB) - added by naked 16 years ago.
first version of the improvement patch
twisted-process-improvements-2.patch (38.8 KB) - added by naked 16 years ago.
second version of the improvement patch

Download all attachments as: .zip

Change History (12)

comment:1 Changed 16 years ago by naked

Glyph had several ideas:

I believe that putting this into spawnProcess is operating at the 
wrong layer of abstraction.  The feature you're describing is highly 
seful though, so providing a utility implementation of 
ProcessProtocol that allows you to hook up regular protocols to 
various FDs would be very good.  IMHO ProcessProtocol looks 
deceptively like a regular protocol, and probably its dataReceived and 
connectionLost methods should be (softly) deprecated.  The right way 
to write a ProcessProtocol is to override childDataReceived, 
childConnectionLost and processEnded.  A ProcessProtocol that could 
hook up arbitrary FDs as input and output to a Protocol would be 
incredibly useful but it would be a good start just to have a 
better-defined way to deal with separating process control from 
protocol logic; so useful it might even be worth putting this 
functionality on the base ProcessProtocol. 
 
It seems like a good first step here would be refactoring the various 
flavors of file descriptor (including the mess in _pollingfile and 
_dumbwin32proc) so that they can be easily and portably invoked by 
users wanting to create out-of-band (non stdin/stdout) channels of 
communication between their processes.  I think it would be best if 
these channels could have a "flavor requirement" specified (i.e. pipe, 
socketpair, PTY), but also have a general stream-based transport API 
that would work the same from a high-level application's point of view 
on Windows and Mac and Linux; for example, use pipes if available, 
numeric unix sockets if not, localdomain sockets if that's not 
available either, depending on platform. 
 
All of this is leaning towards making 0,1,2 as un-special as possible, 
which I like very much.  That also implies that you'll need to clean 
up stdio.StandardIO, and instead add an API like 
reactor.connectParentFD(factory, fileno, flavor=None).  On my first 
read through I started trying to describe a way to communicate the 
expected list of file descriptors and their settings to the child 
process but that seems best left up to the application.
This gives me another good idea.  Right now there's a bunch of stuff 
that happens in the guts of the process module; chdir, setuid, fdmap 
setup.  Most of this could be factored into an UNIX-specific 
ProcessProtocol, centralized in "beforeFork" and "beforeExec" methods. 
I'm not really sure how to best clearly expose that non-portable 
features are required when invoking spawnProcess, but the obvious idea 
is that if IUNIXProcessProtocol.providedBy(yourProtocol), barf on 
Windows, Jython (if they ever do another release and we add support 
for it again), etc.  Then you could explicitly subclass either 
UNIXProcessProtocol if you explicitly needed nonportable features 
(which would still allow you to use them on UNIX; UNIXProcessProtocol 
== ProcessProtocol, on platforms which support it); but subclassing 
ProcessProtocol would select a more platform-appropriate method. 
 
Originally I thought that the process-communication logic would be 
specific to a reactor, which is why it's factored as it is; after a 
few years of working with process spawning closely though it is clear 
to me that differences are entirely a feature of the OS and not of the 
reactor. 

Changed 16 years ago by naked

first version of the improvement patch

comment:2 Changed 16 years ago by naked

I added a first try at some of the improvements listed here.

The current interface is not at all what I had in mind, but the internals are closer. The interface is like that to keep an apparent compatibility with current API while allowing testing of the new features as well.

All of the process test cases pass after this change. The semantics of processes spawned with usePTY=1 have changed somewhat, but none of the border cases are triggered by the tests.

comment:3 Changed 16 years ago by Jean-Paul Calderone

Some superficial comments on the patch:

  • lots and lots of docstrings are missing :)
  • use isinstance() instead of type(foo) in bar

It looks like PTYProcess is not longer used. If that's the case, that's pretty excellent: the patch should delete it entirely.

comment:4 Changed 16 years ago by naked

Thanks for the comments. The omittance of docstrings is intentional, most of the functions are will be different soon. The type(foo) calls were there because they were there in the original code and I didn't know if there was a reason they were such. And I don't remove PTYProcess as to not bloat the patch size.

That said, I have a new version of the patch now. Now it implements a factory for setting up things, among other things. The patch is in even worse condition than the one before, but it does pass the test suite. Lot of things will probably still change, the factory stuff can completely disappear as well.

Changed 16 years ago by naked

second version of the improvement patch

comment:5 Changed 15 years ago by Glyph

Owner: changed from Glyph to naked

Please resubmit with docstrings on everything and unit tests for the new functionality.

comment:6 Changed 15 years ago by naked

There were some discussion on the features on the mailing list, where some developers said that this probably isn't the API they would want - however, nobody gave any specifics as to which direction the patch should be developed, even though I asked for some. And since I was busy with other projects as well, I decided to postpone this work.

I can start the API discussion again in January, if the developers are interested.

Once the API is even remotely what it should be, I will add docstrings and write unit tests.

comment:7 Changed 15 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added

It would be valuable to separate the existing patch into at least two pieces:

  • refactoring to remove the duplication between Process and PTYProcess
  • API/functionality changes

This will make it easier to think about the changes being made.

comment:8 Changed 15 years ago by itamarst

The first item is now being tracked under bug #2341.

comment:9 Changed 15 years ago by jknight

Cc: jknight added

comment:10 Changed 9 years ago by Manish

Cc: Manish added
Note: See TracTickets for help on using tickets.