[process] Formal Review
I would like to surface a number of issues, large and small. You are clearly proud to support a number of different syntactic ways to express the same semantic operation. To me this is a minor negative. While in theory it sounds nice to be able to write Process consumer code any way I want, in a production environment I spend more time reading and maintaining other people's code than I do writing brand-new code. Since each person contributing code to our (large) code base will select his/her own preferred Process style, I will be repeatedly confused as I encounter each new usage. I admire the support for synchronous pipe I/O, while remaining skeptical of its practical utility. Synchronous I/O with a child process presents many legitimate opportunities for deadlock: traps for the unwary. I would be content with a combination of: * async I/O on pipes (yes, using Asio) * system() for truly simple calls * something analogous to Python's subprocess.Popen.communicate(): pass an optional string for the child's stdin, retrieve a string (or, say, a stringstream) for each of its stdout and stderr. The example under I/O pipes the stdout from 'nm' to the stdin of 'c++filt'. But the example code seems completely unaware that c++filt could be delayed for any of a number of reasons. It assumes that as soon as nm terminates, c++filt must have produced all the output it ever will. I worry about the Process implementation being confused about such issues. I'm dubious about the native_environment / environment dichotomy. As others have questioned, why isn't 'environment' a typedef for a map<string, string> (or unordered_map)? I understand that you desire to avoid copying the native process environment into such a map until necessary, but to me that suggests something like an environment_view (analogous to string_view) that can perform read-only operations on either back-end implementation. Operations involving splitting and joining on ':' or ';' should be defined purely in terms of strings and ranges of strings. They should not be conflated with environment-map support. The documentation so consistently uses literal ';' as the PATH separator that I worry the code won't correctly process standard Posix PATH strings. At this moment in history, an example showing integration of Process with Boost.Fiber seems as important as examples involving coroutines. Why is the native_handle_t typedef in the boost::this_process namespace? While in full context it makes sense to speak of "assigning" an individual process to a process group, the method name assign() has conventional connotations. Use add() instead. There's a Note that says: "If a default-constructed group is used before being used in a process launch, the behaviour is undefined." I assume you mean "destroyed before being used," but this is a concern. If a program has already instantiated a process group, but for any reason decides not to (or fails to) launch any more child processes, does that put the entire parent process at risk? What remedial action can it take? Move the group object to the heap somewhere and let it leak? Spawn a bogus child process for the purpose of defusing the ticking process group instance? If you're going to reify process group at all, you should wrap more logic around it to give it well-defined cross-platform behavior. And it should definitely be legal to create and destroy one without associating any child process with it. Given support elsewhere for splitting/joining PATH strings, the string_type path parameter to search_path() feels oddly low-level. Maybe accept a range of strings?
At a minimum, kindly state: - Whether you believe the library should be accepted into Boost * Conditions for acceptance
YES, IF the present Boost.Process preserves the ability of its predecessor to extend it without modifying it. I'm sorry, thorough reading of the documentation plus some browsing through the code leaves me doubtful. Examples of such extensions: * With Boost.Process 0.5 I can use Boost.TypeErasure to pass an "initializer" which is a runtime collection of other initializers. Such a feature in Process 0.6 would support people's requests to be able to assemble an arbitrary configuration with conditional elements and use it repeatedly. * I can create an initializer (actually one for each of Posix and Windows, presenting the same API, so portable code can use either transparently) to implicitly forcibly terminate the child process being launched when the parent process eventually terminates in whatever way. Please understand that I am not asking for the above features to be absorbed into the Process library: I am asking for a Process library extensible enough that I can implement such things with consumer code, without having to modify the library. Perhaps Process 0.6 already is! It's just hard for me tell. Another feature should, in my opinion, be incorporated into the library: * On Windows, if you desire to pass any file handles at all to the new child process, it is completely whimsical what *other* open file handles you may inadvertently pass -- unless you play games with STARTUPINFOEXA and PROC_THREAD_ATTRIBUTE_LIST to enumerate exactly the set of handles you intend to pass. If the library doesn't natively support that, I *must* be able to pass a custom initializer with deep access to the relevant control blocks to set it up. This is a showstopper, as in "you can't remove that file because, unbeknownst to you, some other process has it open."
- Your knowledge of the problem domain
I have hand-written process-management code in C++ several times before -- each with multiple implementations to support multiple platforms. I have tested the previous candidate Boost.Process 0.5 with a number of programs exercising a number of features.
You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design
Design notes are at the top of this mail.
* Implementation
I have only glanced over parts of the implementation. It seems somewhat more obscure than the corresponding pieces of Process 0.5, which is why I couldn't quickly satisfy myself as to the library's extensibility.
* Documentation
Others have noted that the documentation is very sketchy in places. I too wish for more explanation. Much of the time I spent on this review was reading through the documentation. Apologies if I have misunderstood the library's capabilities.
* Usefulness
I have felt for years now that it is essential to get a child-process management library into Boost. It grieves me to have to write platform-specific API calls into different applications.
- Did you attempt to use the library?
I did not, sorry, ran out of time -- as you can infer from this review arriving at the end of the final day!
- How much effort did you put into your evaluation of the review?
Most of my time was spent reading the documentation. I looked through a couple of the implementation files for further information.
I would like to surface a number of issues, large and small.
You are clearly proud to support a number of different syntactic ways to express the same semantic operation. To me this is a minor negative. While in theory it sounds nice to be able to write Process consumer code any way I want, in a production environment I spend more time reading and maintaining other people's code than I do writing brand-new code. Since each person contributing code to our (large) code base will select his/her own preferred Process style, I will be repeatedly confused as I encounter each new usage. First of all: thanks for taking the time to write the review. I just saw the critizism of "std_out > p" coming, while others really
I admire the support for synchronous pipe I/O, while remaining skeptical of its practical utility. Synchronous I/O with a child process presents many legitimate opportunities for deadlock: traps for the unwary. I would be content with a combination of: I think that is an overkill for many problems, I'd rather provide all
Am 06.11.2016 um 00:44 schrieb Nat Goodspeed: liked that. In practice you'd have "std_out > p", "std_out = p" and "std_out(p)". I think that is manageable. the possibilities. Btw.: the main deadlock issue I had will be fixed, I'm working on that right now. But I'm generally not very fond of forcing people to use one solution when there are several.
* async I/O on pipes (yes, using Asio) * system() for truly simple calls Not sure why this would need to be for simple calls, I really like my coroutine solution (that's what I'm proud of here ;)). * something analogous to Python's subprocess.Popen.communicate(): pass an optional string for the child's stdin, retrieve a string (or, say, a stringstream) for each of its stdout and stderr. That'd be limiting the interface, we discussed that in length. There cannot be any solution using stringstream, but boost::asio::streambuf is available. To make that even easier the library does provide a syntax for that:
boost::asio::streambuf in, out, err; child c("foo", std_out > out, std_err > err, std_in < in); I might be adding a new feature to make it easier to implement your own process object. Then again: it's not meant as a high-level implementation, it's meant to make it easy for you implement something like the python module. I.e. you can do that without worrying about the OS-specific stuff.
The example under I/O pipes the stdout from 'nm' to the stdin of 'c++filt'. But the example code seems completely unaware that c++filt could be delayed for any of a number of reasons. It assumes that as soon as nm terminates, c++filt must have produced all the output it ever will. I worry about the Process implementation being confused about such issues. I fear you are right, I'd need to be checking for an empty line at the end.
I'm dubious about the native_environment / environment dichotomy. As others have questioned, why isn't 'environment' a typedef for a map<string, string> (or unordered_map)? For native_environment:
auto nat_env = this_process::environment(); nat_env["FOO"] = "BAR"; assert(getenv("FOO") == "BAR"); For environment: it stores the data already in the needed format for the OS. Thereby I don't need to transform anything when launching a process. That just seemed like the most logical approach to me, and once I have the native_environment it just makes sense. Also it is easily convertible: environment env = this_process::environment();
I understand that you desire to avoid copying the native process environment into such a map until necessary, but to me that suggests something like an environment_view (analogous to string_view) that can perform read-only operations on either back-end implementation. Well native_environnent modifies the environment of the curent process, while environment allows you to build Operations involving splitting and joining on ':' or ';' should be defined purely in terms of strings and ranges of strings. They should not be conflated with environment-map support. Seems convenient to me - what's wrong with that?
The documentation so consistently uses literal ';' as the PATH separator that I worry the code won't correctly process standard Posix PATH strings. I'll look into that. At this moment in history, an example showing integration of Process with Boost.Fiber seems as important as examples involving coroutines. To be honest, I haven't looked that deep into boost.fiber, not sure how
Why is the native_handle_t typedef in the boost::this_process namespace? Because it's used as a return-type there. While in full context it makes sense to speak of "assigning" an individual process to a process group, the method name assign() has conventional connotations. Use add() instead. Good point, assign's the term used in the OS documentations, but that of course has a different connotation in C++.
There's a Note that says: "If a default-constructed group is used before being used in a process launch, the behaviour is undefined." I assume you mean "destroyed before being used," but this is a concern. If a program has already instantiated a process group, but for any reason decides not to (or fails to) launch any more child processes, does that put the entire parent process at risk? What remedial action can it take? Move the group object to the heap somewhere and let it leak? Spawn a bogus child process for the purpose of defusing the ticking process group instance? Nope, it just may return garbage; yeah undefined behaviour is a term
At a minimum, kindly state: - Whether you believe the library should be accepted into Boost * Conditions for acceptance YES, IF the present Boost.Process preserves the ability of its
If you're going to reify process group at all, you should wrap more logic around it to give it well-defined cross-platform behavior. And it should definitely be legal to create and destroy one without associating any child process with it. Destroying is no affected by that. Given support elsewhere for splitting/joining PATH strings, the string_type path parameter to search_path() feels oddly low-level. Maybe accept a range of strings? Would make sense, though a range of fs::path would be even better. predecessor to extend it without modifying it. I'm sorry, thorough reading of the documentation plus some browsing through the code leaves me doubtful. It is possible, that's one of the main reasons I insist on that interface.
Examples of such extensions:
* With Boost.Process 0.5 I can use Boost.TypeErasure to pass an "initializer" which is a runtime collection of other initializers. Such a feature in Process 0.6 would support people's requests to be able to assemble an arbitrary configuration with conditional elements and use it repeatedly. That is not possible, because the functions in the handlers need to be templated - which is a major change in that regard from 0.5. This one is necessary to allow the initializer to access the sequence; this again is required for the simplified asio-stuff. Regarding the construction at runtime: I've got an idea for that (though also not runtime) which might help out there and be extensible. Or there could be an initializer_variant, though that would also not be type-erasure. I'll let you know if I got a prototype of either one up. * I can create an initializer (actually one for each of Posix and Windows, presenting the same API, so portable code can use either transparently) to implicitly forcibly terminate the child process being launched when the parent process eventually terminates in whatever way. You can do that, by just inherting the handler_base which has on_error, on_success and on_setup or the os-specific extension (handler_base_ext) which on posix adds on_exec_setup, on_exec_error, on_fork_error.
Please understand that I am not asking for the above features to be absorbed into the Process library: I am asking for a Process library extensible enough that I can implement such things with consumer code, without having to modify the library. Perhaps Process 0.6 already is! It's just hard for me tell. Sure, that's important to me, especially since that would be a good way to eventually add more features if they are required and to know that
auto p = nat_env["PATH"].to_vector(); assert(p == {"bar", "foo"}); and that also goes the other way around: nat_env["PATH"] = {"bar/foo", "foo/bar"}; that would look. I'm open for any suggestions; async_pipe implements an asio-I/O object, so you probably can already use that with the interface boost.fiber provides to boost.asio. that smells like it would blow up you program, that's a misnomer, sry. On posix it will just yield nonsense (has(pid)) or do nothing and give an error (terminate()), while that works on windows. I think platform-dependent would be the correct word and I'll describe the details. they're already used. I fear that calls for another section in the documentation, though that would be more implemenation notes than a tutorial.
Another feature should, in my opinion, be incorporated into the library:
* On Windows, if you desire to pass any file handles at all to the new child process, it is completely whimsical what *other* open file handles you may inadvertently pass -- unless you play games with STARTUPINFOEXA and PROC_THREAD_ATTRIBUTE_LIST to enumerate exactly the set of handles you intend to pass. If the library doesn't natively support that, I *must* be able to pass a custom initializer with deep access to the relevant control blocks to set it up. This is a showstopper, as in "you can't remove that file because, unbeknownst to you, some other process has it open."
You can do that through a custom initializer.
- Your knowledge of the problem domain I have hand-written process-management code in C++ several times before -- each with multiple implementations to support multiple platforms. I have tested the previous candidate Boost.Process 0.5 with a number of programs exercising a number of features.
You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design Design notes are at the top of this mail.
* Implementation I have only glanced over parts of the implementation. It seems somewhat more obscure than the corresponding pieces of Process 0.5, which is why I couldn't quickly satisfy myself as to the library's extensibility. That's mainly because of the dual-syntax, to allow `child c("foo", "/bar")`, which means that there are two ways to obtain the actual initializers. That makes is not as straight-forward as 0.5.
On 6/11/2016 14:10, Klemens Morgenstern wrote:
The example under I/O pipes the stdout from 'nm' to the stdin of 'c++filt'. But the example code seems completely unaware that c++filt could be delayed for any of a number of reasons. It assumes that as soon as nm terminates, c++filt must have produced all the output it ever will. I worry about the Process implementation being confused about such issues.
I fear you are right, I'd need to be checking for an empty line at the end.
Once the pipe-closing issues are resolved, you should simply read the output stream until EOF. This will only occur once c++filt has terminated (and thus completed and closed its output stream). (This is the right thing to do regardless of whether it's sync or async.)
The documentation so consistently uses literal ';' as the PATH separator that I worry the code won't correctly process standard Posix PATH strings.
I'll look into that.
A common tactic to resolve this sort of issue is to provide a platform-dependent property that contains the joining character and/or a join() method. Of course, encouraging developers to actually *use* them instead of hard-coding it is another matter.
* On Windows, if you desire to pass any file handles at all to the new child process, it is completely whimsical what *other* open file handles you may inadvertently pass -- unless you play games with STARTUPINFOEXA and PROC_THREAD_ATTRIBUTE_LIST to enumerate exactly the set of handles you intend to pass.
FWIW, a slight saving grace with that is that it's most common to open files using NULL security attributes, which makes the file non-inherited by default. You can also flip any existing handle to non-inherited whenever you want.
Mere moments ago, quoth I:
Once the pipe-closing issues are resolved, you should simply read the output stream until EOF. This will only occur once c++filt has terminated (and thus completed and closed its output stream). [...]
* On Windows, if you desire to pass any file handles at all to the new child process, it is completely whimsical what *other* open file handles you may inadvertently pass -- unless you play games with STARTUPINFOEXA and PROC_THREAD_ATTRIBUTE_LIST to enumerate exactly the set of handles you intend to pass.
FWIW, a slight saving grace with that is that it's most common to open files using NULL security attributes, which makes the file non-inherited by default. You can also flip any existing handle to non-inherited whenever you want.
Incidentally, this raises another good reason why it's important to close the child-end of pipes as soon as possible after child processes are launched -- as this would be the only child-inheritable handle of the pipe, then as long as processes are launched only on a single thread (and thus a second process can't be launched until after the inheritable handle is closed) this will avoid unintended inheritance. If you're going to be launching processes concurrently from multiple threads then unless Boost.Process has internal locking to prevent the race it would need to use STARTUPINFOEX to ensure only the "correct" handles get inherited. That API is not available on XP, though; not sure if you still care about that.
On Sun, Nov 6, 2016 at 9:56 PM, Gavin Lambert <gavinl@compacsort.com> wrote:
If you're going to be launching processes concurrently from multiple threads
or for other reasons mentioned earlier in this thread
then Boost.Process would need to use STARTUPINFOEX to ensure only the "correct" handles get inherited. That API is not available on XP, though; not sure if you still care about that.
I think this falls under the heading of new Boost libraries not needing to support obsolete platforms. I have written code to make use of STARTUPINFOEX and the essential InitializeProcThreadAttributeList(), etc., API functions conditional. I don't consider it necessary for Boost.Process, but would be happy to share if others deem it essential.
On Sun, Nov 6, 2016 at 9:38 PM, Gavin Lambert <gavinl@compacsort.com> wrote:
* On Windows, if you desire to pass any file handles at all to the new child process, it is completely whimsical what *other* open file handles you may inadvertently pass -- unless you play games with STARTUPINFOEXA and PROC_THREAD_ATTRIBUTE_LIST to enumerate exactly the set of handles you intend to pass.
FWIW, a slight saving grace with that is that it's most common to open files using NULL security attributes, which makes the file non-inherited by default.
I believe the std::ifstream implementation from Visual Studio 2013 -- the API you use in portable C++ code -- opens files as inheritable by default. Moreover, this API gives you no opportunity to change their inheritability.
You can also flip any existing handle to non-inherited whenever you want.
If your application uses third-party libraries, you have no way to enumerate the complete set of handles held open by your process.
On 11/06/2016 02:10 AM, Klemens Morgenstern wrote:
To be honest, I haven't looked that deep into boost.fiber, not sure how that would look. I'm open for any suggestions; async_pipe implements an asio-I/O object, so you probably can already use that with the interface boost.fiber provides to boost.asio.
The pattern used in Asio is to have both synchronous and asynchronous functions: return_value foo(args); return_value foo(args, error_code); void async_foo(args, void (*)(return_value, error_code)); If async_foo supports boost::asio::async_result, then the coroutine and fiber support can be implemented seperately from your library.
On 11/06/2016 02:10 AM, Klemens Morgenstern wrote:
To be honest, I haven't looked that deep into boost.fiber, not sure how that would look. I'm open for any suggestions; async_pipe implements an asio-I/O object, so you probably can already use that with the interface boost.fiber provides to boost.asio.
The pattern used in Asio is to have both synchronous and asynchronous functions:
return_value foo(args); return_value foo(args, error_code); void async_foo(args, void (*)(return_value, error_code));
If async_foo supports boost::asio::async_result, then the coroutine and fiber support can be implemented seperately from your library. Well there are two things. First you can use async_pipe as any asio I/O-Object, thus this should work with fiber. Secondly there's a special case for process::system, where you can suspend the coroutine for the time the process runs. This requires a custom solution, since the event handler is different - and must be. The reason is, that I need to start
Am 06.11.2016 um 12:26 schrieb Bjorn Reese: the async-wait on posix before launching the process, which would be sort of difficult, if the corutine get's suspendend when I do that. Therefore this application needs an extra implementation, where I imagined something like that: void build(boost::asio::yield_context yield) { bp::system("g++", "-c", "foo.cpp", "-o", "foo.o", yield); bp::system("g++", "-o", "foo.exe", "foo.o", yield); }; I don't know really if anyone would like to use boost.fiber here; this however would require an implementation inside process/system.hpp. So I wouldn't add this now.
On 11/06/2016 12:40 PM, Klemens Morgenstern wrote:
Am 06.11.2016 um 12:26 schrieb Bjorn Reese:
The pattern used in Asio is to have both synchronous and asynchronous functions:
return_value foo(args); return_value foo(args, error_code); void async_foo(args, void (*)(return_value, error_code));
If async_foo supports boost::asio::async_result, then the coroutine and fiber support can be implemented seperately from your library. Well there are two things. First you can use async_pipe as any asio I/O-Object, thus this should work with fiber. Secondly there's a special case for process::system, where you can suspend the coroutine for the time the process runs. This requires a custom solution, since the event handler is different - and must be. The reason is, that I need to start the async-wait on posix before launching the process, which would be sort of difficult, if the corutine get's suspendend when I do that.
Here is how you could implement an async_system() with async_result on Posix platforms. Replace waitpid() in bp::child with a self-pipe [1], set up a SIGCHLD handler which writes to the pipe (using the syscall write(2) to be async-signal-safe), and use asio to read from the pipe with the yield context. The biggest task here is that the signal handler must serve all child processes, so you would need one self-pipe per process, and the management of that map of self-pipes and process identifiers have to be async-signal-safe (which means you have to go lock-free.) Next, add async_result to async_system() and forward the handler to the asio read operation. This will immediately give you support for coroutines, futures, and (using the example from fiber.) With the above, you can completely remove the yield context meta programming from the process library, and thereby avoid having to mess around with its internals. [1] http://cr.yp.to/docs/selfpipe.html
I don't know really if anyone would like to use boost.fiber here; this however would require an implementation inside process/system.hpp. So I wouldn't add this now.
The point with async_result is that this can be done outside the process library.
participants (4)
-
Bjorn Reese -
Gavin Lambert -
Klemens Morgenstern -
Nat Goodspeed