Dear Douglas, thank you for your E-Mail, I wasn't really sure if I should answer at all, because it is my impression that some of your points are just a matter personal preference. That of course is my personal impression, so please understand, that I have no interest in arguing with you, especially since you already gave me your verdict. Which was expected with exactly this outcome. However: many of the points you raised have actual reason and it has been discussed to come up with the solutions I provided. Therefore I consider it the right way to address the points and actually explain the reasons for design decisions, even though you reach different conclusions. Please don't understand this as a personal attack, I just want other reviewers to see your criticism beside my reasoning. Am 27.10.2016 um 15:09 schrieb Niall Douglas:
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. Just out of curirosity - are any of those open-source so I could look into them for reference? 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. First of: operator> is one of three ways to express that, the others are operator() and operator=. I personally like >, which is why it is used in the examples. I added the other variants to accomodate people with different taste.
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. Well this is among the possibilites you have, and I don't like to force
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. Sometimes you need this, e.g. "bp::system("C:/Program Files/MyTool/foo.exe", "C:/Program Files/OtherTool/bar.exe");". This has well defined rules. 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. It's similar to std::thread in that way; if you want to reuse the
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. That's just wrong. A child does not block it is a type matching the
The reason I chose this style is, because you have a lot of arguments going into a child process where in most cases you only use a few. Therefore you could either build a large function with many unused parameters like CreateProcess, you could take the posix approach in calling many functions or you could use a variadic function. Since the first one looks horrible and the second incurs a runtime-overhead we chose the third. With the variadic list you cannot get around named parameters. Secondly all the parameters you pass to a launched process are useless after it is launched, so I think the named parameter approach is quite fitting, since it's a rather unique problem. people to use a rather narrow style. Additionally not every command is a real file, you might want to use some command, e.g. "bp::system("type my_file.txt", bp::shell)". Now granted, this particular example would not be necessary, since we have C++ facilities, but I always assume, that I don't know every use-case. parameters you can use std::tuple to store them. thread, it has a detach and a join. It leaves the ctor as soon as the process has started. system is the blocking function.
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. That is not the reason. In order to use boost.asio with pipes, you need to use named pipes on windows, which behave like file-handles. Therefore (afaik) the pipe does not get closed automatically, hence I cannot guarantee that a pipe will be closed automatically. The deadlock is caused for that reason: the pipe is still open, but no one is writing into it. 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. That was the original behaviour I intended. But consider this example:
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. It works as expected if I know what the user wants. I don't. 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. As stated in the documentation, the child processes of your child
2. I already mentioned the tagged parameter idiom is gratuituous and adds very little to the user experience for a high usability cost. It only adds compile time cost and reduces runtime-cost if compared to
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. One thing I have thought of adding, is the possibility of passing std::future<int> to get the behaviour. You can however already use the on_exit when using boost.asio, so you can do such things. This library is C++11, so future does not have .then. 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. Well there are more differences on windows (e.g. the environment variables) and to check this is for performance. A conversion would just be inefficient if I don't need it. The reason it's added on posix, is so the interface stays the same. I know that it's unnecessary, but it should be the same on both platforms. 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. Well I need them, since I don't have any STL implementation for that
- 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
7. I think the vfork support needs to be dropped as unnecessary. Explicitly requested by a potential user for embedded development. 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. It has a few more properties, like already using a changed PATH-Variable. I.e. it will find an executable in my modified path. Not sure it's a bad thing to allow users to do more. 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. It's default behaviour if nothing is used and it is explicitly marked as an extension. Not sure why this is bad. 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. Well for the reason statet above I think this API design is the best solution; it has very little to do with backwards-compatibility, I broke nearly everything there. But as statet in our last discussion: I'd be really interested in how you would design that. Just fyi: it actually started out as a C++14 library, and I made the
opstream os; child c("foo.exe", std_in < os); os << std::stoi("invalid string"); //throws Now we have a deadlock, because foo waits for input, while the program waits for it to exit. That's bad, really bad. Therefore child is modeled after std::thread. Also, though std::thread doesn't have a terminate, I think that child needs that for one simple reason: you don't have control over the process you launch. Unlike with std::thread, where the content of the thread is at least in your codebase, you might not even know the content of the binary you execute. Additionally, there are applications which don't exit automatically, like "c++filt". That's why there's no way around providing a terminate. Before we start the discussion: there is not portable way to provide a terminate-request, that's why it's not in the library. process, will not be terminated, i.e.: child c("gdb", "foo.exe"); c.terminate(); //foo.exe still runs (on windows) Therefore groups are provided, implemented with process groups on posix and job objects on windows. I added them because I needed them for my application. the alternatives. this_process::environment allows you to modify the environment of the current process and is convertible to process::environment. It also adds the seperated lists like in PATH, which have different seperators on posix and windows. that I can use. It was you who recommended me to not encourage the usage of boost.iostreams. painful decision to go down to C++11, since there is a real need for a process library, including those people still using old compiler. I also had people asking me if I could go down to C++98, since they were forced to use old compilers at work. Though I personally always use the newest compiler versions, we shouldn't forget that many people are bound to older ones. I wrote this library so it may be used and help people, therefore I will not rewrite it in C++17, at least not now. Thanks, Klemens