On Tue, 08 Mar 2011 00:40:36 +0100, Marshall Clow
[...]Thank you to everyone who participated, and especially to Boris and those who wrote a review.
And thank you for having volunteered to be the review manager! Now how do we proceed from here after the library has been rejected? Here are some comments from me: There were a couple of ideas and new proposals how a process management library could look like. If you feel you'd like to step in and take over Boost.Process to give it another boost - drop me a mail and let's discuss how I can support you! I would very much welcome new developers actively working on the library! In case there is not much happening until summer and Boost is accepted in this year's Google Summer of Code program again, I maybe offer to be available as a mentor for another Boost.Process GSoC project. In the short term I probably won't have time to work on Boost.Process. Nevertheless, I want to write down some conclusions I drew from the review on what I think should be done next. Jeff had proposed an executor concept. I'm not sure if I understand everything about the executor concept but I can see a few advantages over the current design. For example the current design requires to use stream_ends to configure streams of a child process. A stream_ends object contains two handles in case the parent process wants to communicate with the child process. However there are use cases where you don't need two handles - for example when you want to redirect stdout of the child process to a file. Jeff's executor concept makes it possible to pass only one handle without a second dummy handle in a stream_ends object. The stream_ends class wouldn't be required anymore and could be removed from Boost.Process. A library user would still need to create a handle for the file to redirect to. But with the executor concept one could use boost::path. Boost.Process could support different types and the implementation would do the rest. Many stream behaviors which are currently used to create stream_ends could be removed. Others can be redesigned to make them more useful - also outside of Boost.Process. This includes for example pipe, named_pipe and async_pipe. These could be functions which return a read and a write end (instead of a parent and a child end). I guess we still want a class for a pair of handles (stream_ends was introduced to make it clear what the parent and child end is; with std::pair you would need to remember). With different classes like boost::path and a handle for pipes (whatever the class would be; some classes from Boost.Iostreams were proposed) to configure streams, the context class currently used by Boost.Process would turn into something hard to describe. There would need to be all kind of containers as different types could be used to configure streams. I think this also speaks for Jeff's idea. Instead of defining a context class which needs to support each and every possible combination of stream settings, providing overloaded functions seems to make more sense. There was the requirement of reusing a context (to create multiple child processes with the same configuration). This would need to be given up with the executor concept (stream behaviors from context are used to create handles on demand; with the executor concept the library user would for example need to create pipes himself to pass read or write ends to child processes). I would be fine with this as I think a better design and API is more important than this requirement (so I agree with Jeremy that this is not worth the extra complexity). When the executor is used to create a child process - whatever "use" means here - I wonder whether it should return a child object (currently done by Boost.Process) or a handle (eventually even a native handle?). With a handle a library user could decide himself what to do and whether he needs the features of the child class. The current implementation of the child class is in so far problematic as it does something extra on Windows only to match the behavior on POSIX (the handle of the child process is kept open to guarantee that the exit code can be fetched). Instead of a child class with methods, one could use a handle and free-standing functions. There is another reason why the child class wouldn't be required anymore: It currently stores the stream ends the parent process uses to communicate with the child process. If we give up the context class and require library users to create handles themselves (via pipe, named_pipe and async_pipe) the parent process has already access to its "stream ends". The child class would only store a handle of the child process - but for that we don't need a child class of course. No process class wouldn't be required either. The methods wait() and terminate() would become free-standing functions. Supporting asynchronous read/write operations would be as easy as it is: The handle can be used with Boost.Asio's classes. Whether we can reuse code from other libraries (Boost.Iostreams?) to support streams I don't know. Currently Boost.Process defines pistream and postream (but only because a close() method had to be added) which are based on a systembuf class (not sure if anything is available in other Boost libraries which could be reused here). Even though I added support for an asynchronous wait() operation I argue now to remove that part (at least for POSIX platforms). The function is very easy to use because it blurs differences between systems. But this is more Java's than C++'s realm. Or to quote Artyom: "SIGCHLD is used for wait operation on the child to exit, it is its purpose." While one could try to implement an asynchronous wait() operation with SIGCHLD there are other problems. In my opinion a cleaner solution is to simply support signal handling. Here we give up the goal of being platform-independent. But if I look at all options I think having a signal handler for POSIX comes closest to what C++ developers and Boost users want. The signal handler from Dmitry Goncharov could be used for a start. For developers on Windows support for an asynchronous wait() looks very different. The current implementation is based on WaitForMultipleObjects() which is called in a helper thread. This can be implemented as a Boost.Asio service (it wouldn't be the first one based on a helper thread). Boost.Asio doesn't ship a service based on WaitForMultipleObjects() (and there is currently no plan that such a service will be added to Boost.Asio) so it would need to be shipped with Boost.Process. The Windows implementation of find_executable_in_path() has to be changed: It should use FindFirstFile()/FindNextFile() and PATHEXT. If an utility function like shell() will be provided, it should use COMSPEC on Windows (and possibly something similar on POSIX instead of a hardcoded path to /bin/sh). There was an interesting remark that create_child() (or an executor) should tell the parent process if the child process couldn't be created - even if fork() has been successful and there are already two processes. I guess we should indeed not write an error message to STDERR_FILENO but throw an exception. The user can then handle the error himself. In that case we need to help him though to find out whether the exception was thrown before fork() (then it's handled in the current process) or after fork() (then it's handled in the copy of the current process) - for example with two different exception classes. Max proposed to use a DSEL. Whether I pass command line arguments to a process via an overloaded operator[] or overloaded functions like in Jeff's executor concept doesn't make a big difference to me in the end. I would probably prefer Jeff's proposal as I currently don't see what operator[] and operator% buy us. Max's proposal puts some emphasis on piping processes though which is not supported by the current draft. I guess it would make indeed sense to add support again (we had support in earlier versions) - and if it's only to think about how the library would need to evolve in the future. The biggest problem I have with Max's proposal is that it seems to be based on the assumption that declarative programming is automatically better than imperative programming. That may be true for certain use cases and some examples. However once you want to do something else not covered by the DSEL, you have to leave the DSEL behind. From the examples I've seen the DSEL seems to be optimized for piping processes. But changing the working directory of a child process, accessing a STARTUPINFO structure, setting up environment variables for a child process, terminating child processes or waiting for them and whatever other developers would like to do would all need to be done differently. I'm not sure whether a DSEL can be created which provides that much flexibility - especially as some complained that not even the current draft provides enough flexibility. There were many more remarks about implementation details. But I think I would bore you if I wrote them all down. I stop for now - the email is long enough to give you lots of new opportunities to disagree. ;) Boris