Formal Review of Proposed Boost.Process library ends Sunday!

If you haven't submitted a review (and plan to), please do not delay. Writing a review ================ If you feel this is an interesting library, then please submit your review to the developer list (preferably), or to the review manager (me!) Here are some questions you might want to answer in your review: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? - Are you knowledgeable about the problem domain? And finally, every review should answer this question: - Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. -- Marshall Marshall Clow Idio Software <mailto:mclow.lists@gmail.com> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait). -- Yu Suzuki

- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
This review is based on a read of the documentation, reading select parts of the code, as well as a prior evaluation of an earlier version of the library. I didn't actually write or compile any code.
- What is your evaluation of the design?
1. The stream behavior interface adds an unnecessary additional layer of abstraction, solely to allow reuse of the context instance, which seems unlikely to be useful, and certainly not worth the added complexity cost. I suggest the following simpler interface: Individual stream_ids are mapped to single (as opposed to pairs of) file descriptors (e.g. boost::iostreams::file_descriptor). Boost process takes care of setting up these mappings. A convenience interface for inherit could still be provided. If some of these file descriptors are pipes, the user code should have maintained a reference to the appropriate end, on its own. Related to this, the pipe creation facility should be written as a general facility independent of Boost.Process (even though it might well officially be contained in it), as should the null file facility. It also should not be tied to C++ iostreams; instead, users could make use of the existing facilities in Boost.Iostreams to obtain a C++ iostream interface to a file descriptor. 2. On POSIX, the asynchronous wait interface does not provide notification of child processes being suspended or continued. 3. Unicode support for Windows (for executable names/paths, command-line arguments, and environment variables) is needed. 4. A simple cross-platform interface to exit status might be useful, e.g. boost::optional<int> normal_exit_status(), where on Windows there is always a value, and on POSIX the value is present only if the process exited normally. 5. On Windows, the extension interface may be too limited. 6. Some simple convenience functions for common tasks such as running a process, waiting (either synchronously or asynchronously) for it to exit, and receiving its STDOUT output as a string, might be useful.
- What is your evaluation of the implementation?
On POSIX, the asynchronous wait interface is quite inefficient, as it requires an extra thread per child process. Instead, waiting based on SIGCHLD should be used. Interoperability with other common C/C++ libraries that deal with process creation, such as glib, when possible would be desirable. On POSIX, errors that occur in the child process after fork() but before execve() should be distinguishable from an exit status that occurs after execve().
- What is your evaluation of the documentation?
Many operations in the library, while to some extent platform-independent, nonetheless have platform-specific behavior that should be documented. For instance, it should be documented that process::terminate calls TerminateProcess on Windows (because then users can look up the specific behavior of TerminateProcess) and ignores the force argument, while on POSIX it sends a SIGTERM signal (or SIGKILL signal if force is true). The note that accessing the process object after calling terminate may be dangerous is incorrect, I believe. On POSIX, the PID is valid until it is returned by a call to wait/waitpid. Thus, on POSIX, if the process is not a child process, the PID may become invalid at any time, before or after the call to terminate. On Windows, the PID is valid until the last handle is closed, and the process class specifically keeps a handle open for this reason, and therefore regardless of whether or not it is a child process, the PID will remain valid. Additionally, on POSIX, it should be documented precisely when the context::setup function is invoked, in relation to other process setup operations, such as changing the working directory, and setting up file descriptors.
- What is your evaluation of the potential usefulness of the library?
If only the synchronous waiting is to be used, or only a small number of processes are to be created, the library is quite adequate for use on POSIX. The support for mapping arbitrary file descriptors, rather than just 0, 1, and 2, makes it more powerful than some other libraries, such as glib. On Windows, it is of limited use as is, due to lack of Unicode support in the various strings, though that could probably be fixed relatively easily.
- Are you knowledgeable about the problem domain?
I am fairly knowledgeable about the complexities of process creation and signal handling on POSIX platforms, having written several simple C++ interfaces for process creation (on top of the POSIX system API) for use in my own software. I also have a reasonable understanding of process creation in Windows (and I believe Windows is fairly straightforward).
And finally, every review should answer this question:
- Do you think the library should be accepted as a Boost library?
The library should not be accepted as is. The lack of efficient asynchronous waiting support, and the overly complicated stream behavior interface, are showstoppers for me.

Sorry for being a day late...
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
As I do not yet need the functionality of the library but might need it in the future for simple cross platform (Windows and OS X) executing of other existing binaries/child processes (with perhaps adjustment of the arguments passed and of the child process priority) I took a simple glance over the library code (the one in the sandbox) to see whether it 'goes in an acceptable direction':
- What is your evaluation of the implementation?
And I immediately found material for my typical rant against a practice too often encountered in many Boost libraries (e.g. a related one http://lists.boost.org/Archives/boost/2010/10/172390.php): the implementation is inefficient to the point of being plain evil: compiling a for-size optimized MSVC++ 10 x86 release build of the start_child.cpp example (that simply starts a child process) produced a ~100 kB binary...!? "Why on Earth" must one pay for strings, vectors and maps of strings, shared pointers, streams (!?), exceptions and all the other 'goodies' and complex logic only to (after 'getting your head dizzy' by stepping through the code) finally arrive to the CreateProcess() call...? Sorry for the 'rant', I appreciate any effort for public contribution, but my vote, if it still counts, being a day late, is NO... -- "What Huxley teaches is that in the age of advanced technology, spiritual devastation is more likely to come from an enemy with a smiling face than from one whose countenance exudes suspicion and hate." Neil Postman

"Domagoj Saric" <domagoj.saric@littleendian.com>:
the implementation is inefficient to the point of being plain evil: compiling a for-size optimized MSVC++ 10 x86 release build of the start_child.cpp example (that simply starts a child process) produced a ~100 kB binary...!? "Why on Earth" must one pay for strings, vectors and maps of strings, shared pointers, streams (!?), exceptions and all the other 'goodies' and complex logic only to (after 'getting your head dizzy' by stepping through the code) finally arrive to the CreateProcess() call...?
..because peaple want program on modern C++, not on "C/C++" If you create a process, don't worry about any performance penalties, you've got it already! :0)
participants (4)
-
Domagoj Saric
-
Jeremy Maitin-Shepard
-
Marshall Clow
-
Max Sobolev