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

Subject: Re: [boost] [gsoc] Boost.Process done
From: Jeremy Maitin-Shepard (jeremy_at_[hidden])
Date: 2010-09-10 20:24:45

On 09/10/2010 03:57 PM, Boris Schaeling wrote:
> On Fri, 10 Sep 2010 05:02:54 +0200, Jeremy Maitin-Shepard
> <jeremy_at_[hidden]> wrote:
>
> Jeremy,
>
> I'll go through your feedback quickly as it's pretty late here. :)
>
>> [...] - 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).
>
> Can you show some sample code you would like to write? Then I think I
> understand better how Boost.Process would need to be changed.

See below.

>> - 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
>
> Not sure what you mean - can you show some sample code again? :)

file_descriptor_t stdout_pipe = output_pipe(ctx, bp::stdout);
file_descriptor_t stdin_pipe = input_pipe(ctx, bp::stdin)

(file_descriptor_t would be some type that manages the lifetime of the 
int/HANDLE.)

bp::std{in,out,err} would be enum values, and on posix, you could also 
use a numeric fd.

output_pipe and input_pipe would be convenience functions that couple 
the creation of a pipe with setting up a file descriptor redirection on ctx,

e.g. something like

file_descriptor_t output_pipe(context &ctx, fileno fd) {
   pipe_t p = create_pipe();
   ctx.map_fd(fd, boost::move(p.write_end));
   return boost::move(p.read_end);
}

I'm not sure what the status of boost::move is, but...  Alternatively, 
file_descriptor_t could be reference counted, though I like moveable better.

You could then use asio, boost iostreams, or something else to interact 
with these file_descriptors.

>
>> 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.
>
> So far reusing a context means launching the very same application a
> second time. This didn't work first with the stream behavior class
> hierarchy but works now with the delegates.

It seems that it is questionable how useful this would actually be in 
most applications, and with the added overhead of redirection through 
boost::function, it might well actually be more efficient to just obtain 
the same effect by writing a function that takes care of creating the 
context and calling create_process, etc.

>
>> - 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.
>
> I agree that the documentation should be improved. It was created from
> scratch for Boost.Process 0.4 and doesn't cover yet really everything.
>
>> - 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);
>
> Once we start to support platform-specific features the question is
> where do we end? The context class in previous Boost.Process versions
> grew more and more as support for uid, euid, gid, eguid, chroot etc. was
> added. Instead of arguing about each and every feature whether it's
> justified to hardcode it into the library we decided it might be better
> to support cross-platform features only and provide extension points.

I don't suggest that Boost.Process should provide an interface to 
specific Windows features; however, I believe the current Windows 
extension interface is not sufficiently powerful, whereas the POSIX one 
is.  However, I think file descriptor setup on POSIX is a special case, 
because it is something the library is already doing anyway (simply 
restricted to 0, 1, and 2), and is also something that is fairly 
complicated to implement correctly, in contrast to e.g. setuid which 
just involves a single library call.

> [snip]

>> - On Windows, it seems there should be both a char and wchar_t
>> versionof 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.
>
> I don't remember anymore all the details which made it difficult to add
> support for wide chars in previous Boost.Process versions. But this is
> something I still want to look into (probably after everything else has
> been settled).

It seems it could either be done with templates, or even just using the 
preprocessor.  Somehow supporting wchar_t on POSIX would naturally be a 
lot more difficult, but it wouldn't be useful anyway.

As an additional comment that I didn't mention before, and seems to be 
related to some recent comments made in this thread regarding the fact 
that POSIX can inherit more than just stdin,stdout,stderr, there needs 
to be a facility on POSIX for specifying the default behavior for file 
descriptors not otherwise specified, e.g. either inherit or close.