Boost logo

Boost :

Subject: Re: [boost] Formal Review of Proposed Boost.Process library ends Sunday!
From: Jeremy Maitin-Shepard (jeremy_at_[hidden])
Date: 2011-02-20 16:14:29


> - 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.


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk