Re: [boost] [gsoc] Boost.Process done

Subject: Re: [boost] [gsoc] Boost.Process done
From: Jeremy Maitin-Shepard (jeremy_at_[hidden])
Date: 2010-09-09 23:02:54

This is a library of significant interest to me, since particularly 
under POSIX child process creation and management is rather complicated 
to do right.  Based on looking at the documentation on the website, and 
browsing through the discussion that has taken place on this list, I 
have some comments.  I understand that the website does not reflect the 
planned changes to the stream behavior configuration interface.

- I suggest some interface changes for the facilities for retrieving the 
parent side of pipes created for stdin, stdout, and stderr of the child:
   - The std::istream/std::ostream interface should be decoupled from 
the facility for retrieving the file descriptor (int) or Windows HANDLE, 
to avoid the overhead of allocating a streambuf for those users that 
wish to access the file descriptor/HANDLE directly or use it with asio. 
  Instead, the std::istream/std::ostream interface to a file 
descriptor/HANDLE could be provided as a separate, completely 
independent facility (designed so that exactly the same syntax could be 
used on both Windows and POSIX), and likewise a separate facility for 
providing an asio interface to a file descriptor/HANDLE (which also 
allows exactly the same syntax to be used on POSIX and Windows).
   - It seems it would be cleaner for the parent side of these pipes to 
be returned in the course of specifying that pipes should be used (in 
the current terminology, in the course of specifying the pipe stream 
behavior).  This prevents context from being reusable, but it seems that 
it would be reusable only a very limited set of circumstances anyway. 
(E.g. if the context::setup method needs additional invocation-specific 
parameters, etc.)  Furthermore, it seems inconsistent that certain 
things are part of the "reusable" context, like the environment and 
process_name, but other things, like the executable path and arguments, 
are not.

- On POSIX, due to the complicated semantics of waitpid, etc. and 
SIGCHLD, and the general desire to avoid leaving around zombie child 
processes, any code that deals with child processes needs to coordinate 
with all other code that deals with child processes.  The library should 
explicitly document what it does with respect to SIGCHLD signals and 
waiting on child processes (and possibly change the behavior if the 
current behavior doesn't handle all cases properly), so that other code 
can correctly interact with it.

- As was observed in the discussion regarding setting up 
stdin/stdout/stderr file descriptors on POSIX, it is somewhat tricky to 
correctly set up child file descriptors.  As I believe it is fairly 
common on POSIX to want to set up child file descriptors other than 0, 
1, and 2, it would be best for the library to provide explicit support 
for this, rather than forcing the user to implement it manually in 
context::setup.  A reasonable interface might be e.g.:
   context::map_file_descriptor(int child_fileno, file_descriptor_t 
existing_descriptor);

Potentially this same interface could be used on Windows, with 
child_fileno only valid in the range 0 to 2 (and with appropriate 
constants defined), and could thus serve as the lowest level interface 
on top of which other behaviors like pipe, redirect, etc. could be 
implemented.

- On POSIX, the precise stage in the child process setup at which 
context::setup is called should be documented.

- One potential issue is that the Windows extension interface only 
exposes a (relatively) small subset of the functionality that can be 
configured on Windows.  There seem to be numerous flags to CreateProcess 
that are not exposed by Boost.Process, and there are also two other 
functions, CreateProcessAsUser and CreateProcessWithLoginW, which allow 
specifying some additional options.  I am not a Windows programmer, so I 
don't know how important this additional functionality is in practice, 
but it seems it would be unfortunate for users that would like to use 
other conveniences provided by Boost.Process (such as pipe setup, 
perhaps) to have to revert to invoking the Windows API directly in order 
to make use of additional functionality not exposed.  In contrast, it is 
hard to imagine the POSIX extension interface being insufficient.  I'm 
not sure how this could be solved, exactly.

- I think it would be cleaner for the POSIX and Windows extension 
interfaces to be in the form of a callback function (templated or 
boost::function), rather than a subclass of context that overrides the 
setup function.

- It seems that natively on Windows, unlike on POSIX, there is no notion 
of an array of command line arguments --- there is just a single string 
command line.  It seems it would be useful for Boost Process to allow a 
single string command line to be specified directly, in addition to the 
existing array (Container) interface which presumably does some sort of 
encoding into a single string on Windows.  Perhaps create_child can 
simply distinguish (at compile-time) between being passed a range of 
char and being passed a range of range of char.

- On Windows, it seems there should be both a char and wchar_t version 
of everything that involves characters.  I suggest that the wchar_t 
version simply not exist on POSIX, as it wouldn't make much sense nor 
would it be useful.