Changes between Initial Version and Version 1 of DocumentationAnalysis/UsingProcesses/AndrewBennetts


Ignore:
Timestamp:
02/24/2006 09:58:47 PM (8 years ago)
Author:
trac
Comment:

--

Legend:

Unmodified
Added
Removed
Modified
  • DocumentationAnalysis/UsingProcesses/AndrewBennetts

    v1 v1  
     1= Review details = 
     2 
     3 * Link to document: [http://twistedmatrix.com/projects/core/documentation/howto/process.html Using Processes] 
     4 * Reviewer's name: Andrew Bennetts 
     5 * Review date: 17 January 2006 
     6 
     7= Document expectations = 
     8 
     9== Intended user == 
     10 
     11Before someone begins using this document, I'd really really like it if they knew: 
     12 * What processes, `argv`, process exit codes, environment variables, `stdin`, `stderr` and `stdout` are (i.e. the basics of what processes in a modern OS are). 
     13 * It would be nice if they knew how to spawn processes in Python, and read their output, e.g. with the `subprocess` module, although strictly speaking this isn't really relevant to doing this in Twisted. 
     14 * Of the security risks in using untrusted data from the network to spawn processes, or as input for a process. 
     15 * The typical pitfalls in communicating with subprocesses over pipes: e.g. deadlocks due to buffering.  (although Twisted's async API ameliorates this a little), Python's `-u` switch, and so on. 
     16 * reactor basics. 
     17 * deferred basics (how to be a client of APIs that return them, perhaps not necessarily how to create them). 
     18 
     19== Outcomes == 
     20 
     21Once someone has read this document they should understand: 
     22 * how to spawn processes in Twisted: 
     23   * that the fundamental API to use is `reactor.spawnProcess` (which is defined in `IReactorProcess`), and how to use it. 
     24     * that this involves writing a `ProcessProtocol`, quite similar to writing a `Protocol` for a TCP socket, and how to do so. 
     25   * that the standard library functions for process spawning are not (unfortunately) safe to use in Twisted, due to signal handling woes on POSIX platforms -- `os.popen` is a strict no-no. 
     26   * that there are helper functions in `twisted.internet.util` such as `getProcessOutput` that take care of the common cases for you. 
     27   * how to use `IProcess` objects, e.g. `foo.signalProcess('KILL')`. 
     28 * that process support on Windows is more-or-less totally broken (except under cygwin) -- help would be appreciated!  Hmm, but see http://twistedmatrix.com/bugs/issue591 -- I guess the key message is "make sure the reactor you're using supports `IReactorProcess`". 
     29 * advanced uses: 
     30   * when and how to use the `usePTY=True` argument of `spawnProcess` (POSIX only?) 
     31   * the `childFDs` argument of `spawnProcess` (POSIX only) 
     32   * that if the developer doesn't understand `usePTY` and `childFDs`, they probably don't need them so can probably just ignore them (this is why I didn't include PTYs and file descriptors in the expected user prerequisite knowledge). 
     33 * that the reactor will process events (data written over pipes, process ending) from spawned processes just as it handles events from sockets (and so just like with network stuff in Twisted, you need to start the event loop with `reactor.run()` or `twistd` or similar). 
     34 
     35= Document review = 
     36 
     37== Coverage of subject matter == 
     38 
     39This document seems to cover the following subject matter at least acceptably well: 
     40 * the `spawnProcess` API and its arguments and return value. 
     41 * that `env` argument of `spawnProcess` is empty by default because the default is to be secure rather than convenient -- it's good that both the API and the text remind developers to think about security issues. 
     42 * writing a simple `ProcessProtocol` that uses both stdin and stdout. 
     43 * events that a `ProcessProtocol` may need to handle. 
     44 * methods that you can call on a `ProcessProtocol`. 
     45 * using `getProcessOutput`.  Very good example, too. 
     46 * using `childFDs` 
     47 
     48This document seems to be attempting to cover the following subject matter, but its coverage is flawed: 
     49 * "Doing it the Easy Way" (where `getProcessOutput` is described) is probably what most people need, and is simpler, so it really ought to be up-front instead of the low-level `spawnProcess` API. 
     50 * the pitfalls of needing to manually ensure `argv[0]` is the executable and that the enviroment will be empty by default probably show be in a little "warning" note, rather than buried in the text. 
     51 * The `WCProcessProtocol` example should be downloadable as a single file ready for execution, rather than only being available in several snippets -- the interleaved explanatory text (almost source comments, really) are very good though. 
     52 * The methods of `ProcessProtocol`s are listed with inconsistent use of parens (some `.method(args)`, some `.method` -- I'd expect the latter to be written `.method()`) 
     53 * the SIGCHLD issue that prevents stdlib functions like `commands.getoutput` from working properly is only mentioned as an aside buried in the text.  It deserves more prominence, it's a serious and unobvious pitfall. 
     54 * the `FortuneQuoter` example for `getProcessOutput` has unused imports. 
     55 * the `GPGProtocol` example is not a complete, executable program.  It also violates PEP-8 by putting `if childFD == 1: self.plaintext += data` on one line instead of two, and not seperating methods with blank lines.  The `decrypt` function it defines is actually the main API of the example, so it deserves a docstring.  It is an unusually practical example though, which is excellent. 
     56 
     57This document ought to be covering the following subject matter but is not: 
     58 * the `usePTY` option 
     59 * it should be stated that `childFDs` is not part of `IReactorProcess` because it does not necessarily exist on all platforms (it's probably a bit of a wart that `usePTY` is part of that interface... ah well). 
     60 
     61This document is recommending the following things that it shouldn't be recommending: 
     62 * `os.kill(self.transport.pid, signal.SIGKILL)` is not the right way to kill a process -- `self.signalProcess('KILL')` is, which is more portable.  And quite likely it's more appropriate to use `TERM` instead of `KILL`. 
     63 * in the "verbose" example, hard-coding a call to `reactor.stop()` inside a `Protocol` (or rather `ProcessProtocol`) implementation is poor taste.  Protocols generally should be mechanism, and policy should be implemented on top of that, not intertwined. 
     64 * also in the verbose example, the `status_object` variable name should be `statusObject` to properly conform to the coding standard. 
     65 
     66This document could be supplemented by links to these existing pre-requisite ("you should read this first") documents: 
     67 * reactor basics 
     68 * deferred intro 
     69 
     70This document could be supplemented by links to these existing follow-up ("now that you know X you can try Y") documents: 
     71 
     72This document could be supplemented by as-yet non-existant pre-requisite ("you should read this first") documents on: 
     73 
     74This document could be supplemented by links to these as-yet non-existant follow-up ("now that you know X you can try Y") documents: 
     75 * using `twisted.internet.stdio` 
     76 
     77This document could be supplemented by links to the following relevant source in Twisted, for concrete examples of this knowledge as well as deeper understanding: 
     78 * `twisted.internet.utils.getProcessOutput` 
     79 * `twisted.web.cgi` 
     80 * `doc/core/examples/ptyserv.py` 
     81 
     82This document could be supplemented by links to other references: 
     83 * W. Richard Steven's ''Advanced Programming in the UNIX(R) Environment'', ISBN: 0201563177. 
     84 
     85== Style == 
     86The following changes to the style of the document would make it easier to read: 
     87 
     88Perhaps splitting out the `childFDs` stuff (the "Mapping File Descriptors" section) into a seperate document.  It's less vital knowledge, and necessarily denser and requiring more prerequiste knowledge (e.g. what a "fd" is, some idea of how piping between processes works).  It clearly feels like a seperate document tacked on to the rest, which is more-or-less true. 
     89 
     90Following the current Twisted Documentation style guide by e.g. declaring prerequisite knowledge. 
     91 
     92The "Easy Way" should '''definitely''' be mentioned first. 
     93 
     94== Overall summary == 
     95 
     96This document is: good, with some minor improvements to recommend