Re: [boost] [process] Formal Review starts today, 27 October

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 
years.

> 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 
own.

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 
API.

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 
hard.

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 
http://lists.boost.org/boost-announce/2011/03/0292.php 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.

Niall

-- 
ned Productions Limited Consulting
http://www.nedproductions.biz/ 
http://ie.linkedin.com/in/nialldouglas/