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.