Re: [boost] [process] Formal Review

Subject: Re: [boost] [process] Formal Review
From: Klemens Morgenstern (klemens.morgenstern_at_[hidden])
Date: 2016-11-04 15:26:12

Am 04.11.2016 um 17:21 schrieb Raphaël Londeix:
>> We encourage your participation in this review. At a minimum, kindly state:
>> - Whether you believe the library should be accepted into Boost
>>
> YES
>
>
>>    * Conditions for acceptance
>>
> The interface used to spawn processes should be reworked.
>
>
>> - Your knowledge of the problem domain
>>
> I have implemented some process wrappers many times. I have used the last
> release of Boost.Process (0.5 I think?).
>
>
>> - What is your evaluation of the library's:
>>    * Design
>>
> I think the similarity with the boost::thread api is a great choice. The
> lifetime of process
> is handled by small functors that will be called at specified times. I
> think is a very good
> design as it allows one to write small "plugins" in order to affect the
> setup or the tear down
> of a process. The integration with Boost.Asio follows the previous choice
> of using boost::thread
> as a reference, i.e. the child process is not the resource you read
> from/write to, you need
> some separate stream objects whose lifetime are tied to the child process:
>
> struct MyProcess
> {
>      bp::async_pipe std_in;
>      bp::async_pipe std_out;
>      bp::async_pipe std_err;
>      bp::child child;
> };
>
> I haven't seen a way however to be asynchronously notified when a child
> exited, so I'm not
> sure how one would handle gracefully multiple async_read/write while the
> process could exit
> at any time.
You've got bp::on_exit for that if you use boost.asio, i.e. pass a 
io_service.
> The choice of having a nice interface to spawn processes have been
> discussed already, but I
> still think that the library should aim to have the simplest and lowest
> level interface.
> IMO, C++ users do not need to have a nice and expressive syntax to spawn
> process, I
> cannot imagine a code base where many processes are launched at many places
> in the
> code. In other words, I think that either you rarely need to spawn a
> process (a hack for example),
> or your job is to spawn processes (a shell, a make clone, ...).  In both
> cases, you will
> spawn processes in very few places in the code.
>
> Moreover, this interface style has an implication on build times, errors
> seen at build time
> and makes it hard to use in cases where your program needs to spawn
> processes based on
> runtime decisions. if one decided to implement a shell that supports
> redirections, pipes,
> environment modification and background jobs, they would certainly need to
> make runtime
> adjustments to the arguments given to child constructor. As of today, this
> has to be
> worked around by the user ; I think this should be the primary interface.
The only annoying thing there are the redirections, because there you'd 
need a variant. I really don't want to add variant to the 
std_in/std_err/std_out initializer, since would include them everytime 
then. But: I could add it as an optional feature. Now that thing could 
be used like this:

auto var = bp::make_variant(bp::std_out);
bp::pipe p;
var = p;

bp::child c("foo", var);

> A suggestion has been made to have a more idiomatic C++ construction, I
> would extend it
> with a specific options type:
>
> auto options = bp::options().args("gcc");
> if (verbose) options.append_arg("-v");
> if (ignore_errors) options.std_err.redirect(bp::null_device);
> bp::child child(io_service, std::move(options));
>
> This way you make it impossible to modify the options after the child
> process creation.
> But that's just an idea :)
Thanks for the idea: option objects might be the best solution for that, 
and I think the best way would be to allow an easy implementation of 
that. Maybe something like that:

auto options = bp::make_options(bp::exe, bp::args, bp::std_err);
options.set_exe("gcc");
if (verbose) options.add_arg("-v");
if (ignore_errors) options.std_err(bp::null);
bp::child c(options);

I'll need to check if I can come up with a syntax for that, but that 
would be a way to easily build an option type, without changing anything 
underneath. And that would also be extensible, which the other ideas 
were not. And if that works, I might even be able to automatically add 
the io_service if you do not pass it, hence reducing the pain caused by 
the fusion-errors.

auto opt = bp::make_options(bp::std_err & bp::std_out); //requires an 
io_service, since it might be async.
opt.get_io_service(); //and there it is.

Now if you combine these two you'd have no problem making your own 
child-implementation.

struct my_child
{
     asio::io_service ios;
     decltype(bp::make_options(bp::exe, bp::args, bp::std_err) opt{ios}; 
//so it's only one
     bp::async_pipe out(ios); //because you already know what that's 
going to be.

     bp::child c;
     void operator()() {c = child(opt, std_out > out); };
};

This is also extensible. I would however not make this the default, 
since it incurrs run- and compile-time overhead.

>
>    * Implementation
> The implementation is clear, but the support for the aforementioned
> constructor interface
> "leaks" into the implementation everywhere. As the previous version of
> Boost.Process,
> the different OS support is nicely separated and each stage of the process
> creation is
> clearly implemented in its own header.
>
>
>>    * Documentation
>>
> The asynchronous section feels a bit short given the many possibilities the
> interface
> gives us.
>
>
>
>>    * Tests
>>
> Didn't check
>
>
>>    * Usefulness
>>
> I think it would be very useful to have something we can rely on instead of
> our own
> half baked implementations.
>
>
>> - Did you attempt to use the library? If so:
>>    * Which compiler(s)
>>
> g++4.8 using c++11
>
>
>>    * What was the experience? Any problems?
>>
> I had many problems and decided to fallback on popen().
> The first kind of problems were some compile errors. But the author kindly
> stated
> that I needed the HEAD of boost (which is not an option for me) but I found
> a
> workaround by not using some niceties of the interface:
>
> https://github.com/klemens-morgenstern/boost-process/issues/20
> https://github.com/klemens-morgenstern/boost-process/issues/21
>
> As said previously, for someone not used to boost.fusion, the errors are
> hard to
> read.
Yep, those are really ugly even if you know fusion. But I've got no idea 
how to create a good error message there, if anybody has an idea, let me 
know. SFINAE doesn't really help, because that just tells you, that no 
matching function could be called. And deep nested static_asserts also 
don't help anybody, especially since they get buried in the template 
error messages.
>
> The other problem that made me give up (temporarily) on using Boost.Process
> was the fact that asynchronous reads where unreliable when more than one
> process
> is spawned.
>
> https://github.com/klemens-morgenstern/boost-process/issues/22
>
> This issue has been closed despite the fact that it is not fixed.
Well I've got several tests for that, and the particular example (with 
echo) also can fail when only used with one - that's why I assume(d) 
there's some other issue.
But since we found a design bug during the review, which is actually 
caused by a faulty analysis on my part, the behaviour of pipes will 
change. That means that the way the async_pipe handles the exit of the 
child process will be different and we'll see if that fixes it.
> - How much effort did you put into your evaluation of the review?
> I'd say half a day counting this mail and the testing I made.
>
> Cheers,
>