Boost logo

Boost :

Subject: Re: [boost] [process] Formal Review starts today, 27 October
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2016-10-27 09:09:40

On 27 Oct 2016 at 9:26, Antony Polukhin wrote:

> We encourage your participation in this review. At a minimum, kindly state:
> - Your knowledge of the problem domain

Deep. I've written several of these over many years for many
platforms and used the many incarnations of Boost.Process over many

> You are strongly encouraged to also provide additional information:
> - What is your evaluation of the library's:
> * Design

I have serious concerns:

1. I really don't like the tagged named parameter idiom for any
constructor. It may be Python idiomatic, but it's not C++ idiomatic.
It also adds seriously to compile times and weird error messages for
a very low increase in usability. I particularly think tagging stdio
redirection using operator > a mistake.

If Process were ever standardised, the tagged parameter idiom would
be removed in any case.

2. I think supplying the process launch executable and arguments as a
parameter sequence is a serious design error. The process executable
should always be a filesystem::path period and it should stand alone
maybe passed to a constructor. The child process arguments should be
an iterator pair or a gsl::span or an initialiser list. I'd even
accept a const lvalue ref to any STL iterable.

3. The ability to pass a space separated string for launch executable
and arguments needs to be eliminated permanently. It's an enormous
reliability pit and it's dangerous.

4. I don't think the child constructor should launch the process. It
should construct a child instance only, maybe the call operator
should do the actual launch. The call operator should take the launch
arguments, the constructor the filesystem::path to the binary. The
call operator should be reusable, each time it is called you get a
new child process. This lets you reuse the setup to launch many very
similar child processes.

5. The default should not be that launching a child blocks until it
exits in any API. It should return an object matching the
promise-future protocol i.e. it has a .wait() and a .get(). This type
should be told to the Coroutines TS where available so Coroutines can
treat it as an await point. For non-Coroutines users, if they want to
wait until the child exits, they can manually call .get() on their

6. It is a *very* serious design flaw that i/o deadlocks if you try
to read after the child exits. The documentation suggests this is
because maybe because we are redirecting to a file. This is not a
good reason to have i/o deadlocking seeing as every system I've ever
used it is possible to configure the pipes to error if you try to
read from a closed pipe. The default should be to always error on a
closed pipe read, and if the user is foolish enough to redirect to a
file then it's on them to work around it rather than forcing very
unhelpful and bug inducing behaviour on every user.

7. Another *very* serious design flaw is ever terminating a child
process as part of any normal operation. If you're trying to destruct
a running child process, blocking until it exits is the correct
semantics and push the problem of the blocking onto the user.
Terminating processes is a very severe act, I would even recommend
that you remove all ability to terminate a child from Process
entirely. If Process were ever standardised, that ability would
definitely be removed, after all if a user super really needs it they
can fetch the native handle and call the native terminate process

8. I was surprised to see async_pipe exposed in the tutorial. I would
have thought a correctly designed Process library would have merely
provided getters for the async stdin, stdout and stderr which
returned some opaque thing which can be fed to ASIO's async_XXX()
functions and it all works as expected.

This brings me onto the next set of design problems: Process as
proposed today does far too much stuff which it doesn't need to do,
nor has any business doing.

1. Grouping is superfluous to needs and should be removed. If you
used the design pattern I mentioned above where the constructor
configures the launch but the call operator does the launch with some
args, you no longer need grouping.

2. I already mentioned the tagged parameter idiom is gratuituous and
adds very little to the user experience for a high usability cost.

3. Handlers are unnecessary and should be removed. If you were
returning a future protocol matching object, it would implement a
.then() anyway. For process launched callbacks etc they are very hard
to implement bug free anyway and should be removed.

4. You are providing wchar_t support on POSIX, totally unnecessary.
In fact given how the unicode parts of all this are not ever on
critical time paths relative to the cost of a process launch, I think
you should make the entire API UTF-8 on all platforms apart from the
filesystem::path. On Windows do a UTF-8 to UTF-16 conversion just
before you call CreateProcess if and only if the environment has been
modified from the calling process (passing NULL means inherit the
calling process). Simpler is better.

5. There is zero need for your own args and environment class
implementations when an iterator pair, an arbitrary iterable STL
container, an initialiser list or a gsl::span is everything needed.
Those implementations should be removed entirely as unnecessary
complication and baggage over what's already in the STL. They add no
value at all over STL classes.

6. I am unconvinced that you need to be implementing your own STL
iostreams layer for pipe i/o. I am not sure if such conventional
boilerplate one can take from any C++ fundamentals book should be
duplicated in a library. I'd remove it personally.

7. I think the vfork support needs to be dropped as unnecessary.

8. I think shell launching support needs to be dropped as
unnecessary, and unportable. If the user wants to invoke the shell
they can pass a shell program to Process and do it by hand, it's not

9. The windows hide, maximised, minimized etc stuff all needs to be
removed. Launch all child processes as console apps like POSIX does.
Make behaviour the same on both.

> * Implementation

I didn't look at the implementation given all the serious API design
problems above. If all the above get fixed and it comes to review
again I'd be happy to look into implementation.

> * Documentation

I felt the documentation lacking given just how much complexity is in
usage. If all the stuff I recommended be removed were so, I think the
amount of documentation right now would be abound the right amount.

> * Tests
> * Usefulness
> - Did you attempt to use the library? If so:
> * Which compiler(s)
> * What was the experience? Any problems?

None of the above given the serious design problems.

> - How much effort did you put into your evaluation of the review?

A lunch break at work :) But I've been using Boost.Process for
something like seven years now, and I was lurking through the
previous 2011 review where many of
my above issues were also raised.

As you might be gathering, my recommendation is to reject this
library as submitted. Too many severe design flaws in its API and
semantics, and it does far too much which is unnecessary and unneeded
yet simultaneously does not do enough of what is necessary and is
needed. Same as the 2011 review if I remember correctly, though for
different reasons.

I should make it clear that I think this library has *greatly*
improved over where it was even a year ago. I thank the author for
making so much substantial effort. But it's trying to retain some
backwards compatibility with previous Boost.Process, and hence this
API design is not dissimilarly flawed to all Boost.Process till now.

I think the author needs to start again from scratch in API design
and semantics. Ask himself what API design and semantics would
dovetail perfectly with the upcoming C++ 17 standard such that if
Process were standardised, it would look perfectly a part of the
standard C++ 17 or 20 library. Do *the minimum necessary* as a
standard C++ child process library.

I think that Process I would strongly approve of, and vote for
acceptance. This one unfortunately I cannot.


ned Productions Limited Consulting

Boost list run by bdawes at, gregod at, cpdaniel at, john at