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/