Subject: Formal Review of Proposed Boost.Process library starts tomorrow
It's my pleasure to announce that the review of Boris Schäling's Process library starts tomorrow, February 7th and lasts until February 20, 2011, unless an extension occurs. What is it? =========== Boost.Process is a library to manage system processes. It can be used to: • create child processes • run shell commands • setup environment variables for child processes • setup standard streams for child processes (and other streams on POSIX platforms) • communicate with child processes through standard streams (synchronously or asynchronously) • wait for processes to exit (synchronously or asynchronously) • terminate processes Getting the library =================== The library is available at: http://www.highscore.de/boost/gsoc2010/process.zip with documentation at: http://www.highscore.de/boost/gsoc2010/ There was a quite involved "informal review" on the mailing list this past summer. That thread is archived here: http://thread.gmane.org/gmane.comp.lib.boost.devel/207594 Writing a review ================ If you feel this is an interesting library, then please submit your review to the developer list (preferably), or to the review manager (me!) Here are some questions you might want to answer in your review: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick - reading? In-depth study? - Are you knowledgeable about the problem domain? And finally, every review should answer this question: - Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. -- Marshall Review Manager for the proposed Boost.Process library
I did a brief look into the docu and the "informal review" thread. I would suggest to provide a function indicating if you are running on POSIX or Windows platform: class process { ... static bool is_posix(); ... } The lib could provide an encapsulation for testing the exit code against WIFEXITED, WEXITSTATUS, WIFSIGNALED, WTERMSIG, WIFSTOPPED, WSTOPSIG, WIFCONTINUED, WCOREDUMP (as the old version of boost.process did). Sending signals (SIGSTOP, SIGCONT, SIGKILL, etc.) to the process would also be possible. In the case this functionality is used on Windows paltform without testing process::is_posix() you could raise an assertion. Oliver -- Neu: GMX De-Mail - Einfach wie E-Mail, sicher wie ein Brief! Jetzt De-Mail-Adresse reservieren: http://portal.gmx.net/de/go/demail
AMDG Review part 1, documentation: * Introduction: eg. should be e.g. * create_child: since the argument is the path to the file, why don't you take a boost::path, instead of a string? * It seems like redirecting a stream to a file should be supported. Maybe it can be done with inherit, but it isn't immediately obvious and I'd rather not deal with handle::native_type at all. * There are pistream and postream. What about piostream for completeness. * Why do you have to pass a native handle to pipe's constructor. Can't it be overloaded to take a handle? * "On POSIX systems you must use the macros defined in sys/wait.h to interpret exit codes." This forces me to use #ifdefs in my code. Isn't there a way to abstract this better? At least you could wrap the macros and have a trivial implementation for windows (WIFEXITED -> true) * context::setup: The reference specifies boost::function< void()>, but this is obviously only true for posix. It should describe the two different behaviors. * std::string context::work_dir; boost::path? * boost::iostreams has a file_descriptor class that is very similar to handle. * What is the behavior of find_executable_in_path W.R.T. extensions. On windows, does it use PATHEXT? I'd like to see this spelled out explicitly. * What about file associations/#!xxx for create_child? i.e. can I run create_child("script.pl") or do I need to use shell? I can live with it either way, but the reference needs to specify it. In Christ, Steven Watanabe
AMDG On 2/7/2011 10:22 AM, Steven Watanabe wrote:
* context::setup: The reference specifies boost::function< void()>, but this is obviously only true for posix. It should describe the two different behaviors.
I see that the doxygen comments do have this. It looks like a BoostBook problem. I'll see what I can do about it. In Christ, Steven Watanabe
On 07/02/2011 19:22, Steven Watanabe wrote:
* There are pistream and postream. What about piostream for completeness.
Pipes are either read-only or write-only; there could however be a mechanism to aggregate two pipes into a single iostream.
On 02/07/2011 12:30 PM, Mathias Gaunard wrote:
On 07/02/2011 19:22, Steven Watanabe wrote:
* There are pistream and postream. What about piostream for completeness.
Pipes are either read-only or write-only; there could however be a mechanism to aggregate two pipes into a single iostream.
I know that this is a rather late reply, but socketpair() is a common mechanism for constructing bi-directional pipes between processes (at least on Unix). I don't think it is necessary to hold up the release of this library, but Steven's suggestion certainly has merit. Rob
AMDG Review part 2: implementation boost/process.hpp: boost/process/all.hpp: boost/process/child.hpp: The PP guard on the constructor that takes a handle, should be #if defined(BOOST_WINDOWS_API) || defined(BOOST_PROCESS_DOXYGEN) so that it appears in the reference. The map should be passed by const std::map<stream_id, handle>& (No need to make extraneous copies.) windows.h is not needed. boost/process/config.hpp: Can you use BOOST_THROW_EXCEPTION instead of rolling your own file/line info? boost/process/context.hpp: boost/process/environment.hpp: boost/process/handle.hpp: The semantics of handle::dont_close appear to be the same as the original boost::iostreams::file_descriptor, in that an explicit call to close still closes the handle. This led to some problems, so Daniel James changed it to have three options. See ticket #3517. I'm wondering whether similar problems are liable to come up here. If the handle is supposed to be closed and construction fails, the native handle should be closed. Otherwise, it's difficult make code using it exception safe. boost/process/operations.hpp: find_executable_in_path: I'm not sure that asserting is the right response when the name contains a "/." It's not unreasonable to expect that the name can come from outside the program, and thus may not be valid. If you do go with assert, the precondition needs to be documented. On Windows, it's possible to have a path longer than MAX_PATH. You should probably use the size returned from SearchPathA to allocate a buffer that's big enough and try again. Cygwin executables end with .exe, just like windows. create_child: The context should be passed by const reference. The requirements on Context and Arguments need to be documented. Is not exception safe with BOOST_POSIX_API because if environment_to_envp throws, the memory allocated by collection_to_argv will not be released. I'd like everything after the child process is started to be no-throw, so I can guarantee that if the child is created, I can get a handle to it. This includes returning the child object. boost/process/pid_type.hpp: boost/process/pipe.hpp: Can you #include the appropriate specific header, rather than the whole boost/asio.hpp? boost/process/pistream.hpp: boost/process/postream.hpp: boost/process/process.hpp: boost/process/self.hpp: boost/process/status.hpp: boost/process/stream_behavior.hpp: boost/process/stream_ends.hpp: boost/process/stream_id.hpp: boost/process/stream_type.hpp: boost/process/detail/basic_status.hpp: Again, #include <boost/asio.hpp> is a lot more than you need. boost/process/detail/basic_status_service.hpp: The constructor is not exception safe. If starting the thread fails, the event will be leaked. work_thread can throw which will terminate the process. Is this the desired behavior or should it be using Boost.Exception to move the exception to a different thread? async_wait can leak the HANDLE created with OpenProcess. condition variables are allowed to have spurious wakeups. You need to track enough state to make sure that you don't wake up early. boost/process/detail/posix_helpers.hpp: environment_to_envp and environment_to_envp are not exception safe. boost/process/detail/status_impl.hpp: operation: If an argument is not used, virtual void operator()(int /* exit_code */) is the best way to suppress warnings. boost/process/detail/systembuf.hpp: I don't think that overflow is correct. According to the MSDN documentation WriteFile can return when: * The number of bytes requested is written. * A read operation releases buffer space on the read end of the pipe (if the write was blocked). For more information, see the Pipes section. * An asynchronous handle is being used and the write is occurring asynchronously. * An error occurs. Note #2 in particular. boost/process/detail/windows_helpers.hpp: Can you #include <cstring> instead of <string.h>? General: The #include <windows/unistd.h> is not consistent. Sometimes there's an extra #else # error "Unsupported platform." #endif sometimes not. In Christ, Steven Watanabe
On Tue, 08 Feb 2011 02:06:09 +0500, Steven Watanabe <watanabesj@gmail.com> wrote:
AMDG
Review part 2: implementation
[snip]
boost/process/config.hpp:
Can you use BOOST_THROW_EXCEPTION instead of rolling your own file/line info?
I think BOOST_PROCESS_SOURCE_LOCATION is my piece of code, which was written because BOOST_THROW_EXCEPTION didn't work (IIRC, there was a problem with BOOST_CURRENT_FUNCTION)
[snip]
On Mon, Feb 7, 2011 at 11:38 PM, Ilya Sokolov <ilyasokol@gmail.com> wrote:
On Tue, 08 Feb 2011 02:06:09 +0500, Steven Watanabe <watanabesj@gmail.com> wrote:
AMDG
Review part 2: implementation
[snip]
boost/process/config.hpp:
Can you use BOOST_THROW_EXCEPTION instead of rolling your own file/line info?
I think BOOST_PROCESS_SOURCE_LOCATION is my piece of code, which was written because BOOST_THROW_EXCEPTION didn't work (IIRC, there was a problem with BOOST_CURRENT_FUNCTION)
I'm not aware of any problems with BOOST_THROW_EXCEPTION or BOOST_CURRENT_FUNCTION. I'd suggest deriving the exception types from boost::exception, and using boost::error_info to indicate things like which system function failed, and any relevant information like executable file name: struct process_system_error: virtual boost::system::system_error, virtual boost::exception { }; BOOST_THROW_EXCEPTION(process_system_error()<<boost::errinfo_api_function("CreateProcess()")<<boost::errinfo_file_name(exe.get())); This not only makes the error info available for the user at the catch site, it'll also show up automatically in the string returned by boost::diagnostic_information()/current_exception_diagnostic_information(), along with the throw location, etc. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
On Mon, 07 Feb 2011 22:06:09 +0100, Steven Watanabe <watanabesj@gmail.com> wrote:
[...]The PP guard on the constructor that takes a handle, should be #if defined(BOOST_WINDOWS_API) || defined(BOOST_PROCESS_DOXYGEN) so that it appears in the reference.
The map should be passed by const std::map<stream_id, handle>& (No need to make extraneous copies.)
windows.h is not needed.
boost/process/config.hpp:
Thanks, all changed locally.
Can you use BOOST_THROW_EXCEPTION instead of rolling your own file/line info?
As Ilya had noticed I took the macro from him. I'll check again whether we can replace it with BOOST_THROW_EXCEPTION.
[...]The semantics of handle::dont_close appear to be the same as the original boost::iostreams::file_descriptor, in that an explicit call to close still closes the handle. This led to some problems, so Daniel James changed it to have three options. See ticket #3517. I'm wondering whether similar problems are liable to come up here.
If the handle is supposed to be closed and construction fails, the native handle should be closed. Otherwise, it's difficult make code using it exception safe.
Ok, I'll check the ticket.
[...]find_executable_in_path:
I'm not sure that asserting is the right response when the name contains a "/." It's not unreasonable to expect that the name can come from outside the program, and thus may not be valid. If you do go with assert, the precondition needs to be documented.
I'm fine changing it.
[...]create_child: [...]I'd like everything after the child process is started to be no-throw, so I can guarantee that if the child is created, I can get a handle to it. This includes returning the child object.
Makes sense - I'll look into it!
[...]Can you #include the appropriate specific header, rather than the whole boost/asio.hpp?
Also noted (after Artyom's mail I'd appreciate more feedback though if support for asynchronous wait operations should be dropped).
[...]boost/process/detail/basic_status_service.hpp:
The constructor is not exception safe. If starting the thread fails, the event will be leaked.
work_thread can throw which will terminate the process. Is this the desired behavior or should it be using Boost.Exception to move the exception to a different thread?
async_wait can leak the HANDLE created with OpenProcess.
condition variables are allowed to have spurious wakeups. You need to track enough state to make sure that you don't wake up early.
The implementation gets so complicated that I wonder whether this gets out of proportion. Even if all of this works at some point a developer could always create a thread himself and call wait(). That's not cross-platform and not as convenient as calling async_wait(). But I can fully understand that Artyom is not happy with the implementation. And it would get even more complicated. :-/
[...]boost/process/detail/windows_helpers.hpp:
Can you #include <cstring> instead of <string.h>?
I think MSDN says to include string.h for those _s functions like memcpy_s. But then you are right that cstring should probably work, too (will test). Thanks again, Boris
On Mon, 07 Feb 2011 23:22:24 +0500, Steven Watanabe <watanabesj@gmail.com> wrote:
AMDG
Review part 1, documentation:
* Introduction: eg. should be e.g.
* create_child: since the argument is the path to the file, why don't you take a boost::path, instead of a string?
Then what type to choose for context::env? I stated my opinion on the i18n problem earlier: http://article.gmane.org/gmane.comp.lib.boost.devel/207745
* It seems like redirecting a stream to a file should be supported. Maybe it can be done with inherit, but it isn't immediately obvious and I'd rather not deal with handle::native_type at all.
+1
[snip]
* Why do you have to pass a native handle to pipe's constructor. Can't it be overloaded to take a handle?
I'm not sure which pipe do you have in mind. Probably that means that the names bp::pipe, bp::stream_behavior and bp::stream_ends are not perfect.
[snip]
* std::string context::work_dir; boost::path?
see above
[snip]
AMDG Review part 1, documentation: * Introduction: eg. should be e.g.
* "On POSIX systems you must use the macros defined in sys/wait.h to interpret exit codes."
This forces me to use #ifdefs in my code. Isn't there a way to abstract this better? At least you could wrap the macros and have a trivial implementation for windows (WIFEXITED -> true)
+1
* context::setup: The reference specifies boost::function< void()>, but this is obviously only true for posix. It should describe the two different behaviors.
Is this yet a limitation of Doxigen? Vicente -- View this message in context: http://boost.2283326.n4.nabble.com/Subject-Formal-Review-of-Proposed-Boost-P... Sent from the Boost - Dev mailing list archive at Nabble.com.
AMDG On 2/7/2011 11:06 PM, Vicente Botet wrote:
* context::setup: The reference specifies boost::function< void()>, but this is obviously only true for posix. It should describe the two different behaviors.
Is this yet a limitation of Doxigen?
The problem is in our stylesheets. I posted a patch to the doc list earlier today. In Christ, Steven Watanabe
My 2 cents, about source code... 1. In class boost::process::child. There is a type 'std::map<stream_id, handle>' that used several times. May be typedef? 2. In config.hpp file. Macros will be easier for maintenance if align backlslashes. Compare: #define BOOST_PROCESS_THROW_LAST_SYSTEM_ERROR(what) \ boost::throw_exception(boost::system::system_error( \ boost::system::error_code(BOOST_PROCESS_LAST_ERROR, \ boost::system::get_system_category()), \ BOOST_PROCESS_SOURCE_LOCATION what)) and #define BOOST_PROCESS_THROW_LAST_SYSTEM_ERROR(what) \ boost::throw_exception(boost::system::system_error( \ boost::system::error_code(BOOST_PROCESS_LAST_ERROR, \ boost::system::get_system_category()), \ BOOST_PROCESS_SOURCE_LOCATION what)) 3. In constructor of class boost::process::context. In this place you insert a values into map, but not search them. So use insert() instead of operator[]. Of course, with boost::assign ;-) using namespace boost::assign; insert( streams )( stdin_id, behavior::inherit(STDIN_FILENO) ) ( stdout_id, behavior::inherit(STDOUT_FILENO) ) ( stderr_id, behavior::inherit(STDERR_FILENO) ) ; 4. In some classes you use public inheritance from boost::noncopyable, but you can use private inheritance instead. 5. In constructors of some classes you forgot 'explicit'. 6. In function boost::process::self::get_environment(). environment e; // ... try { char *env = ms_environ; while (*env) { std::string s = env; std::string::size_type pos = s.find('='); e.insert(environment::value_type(s.substr(0, pos), s.substr(pos + 1))); env += s.size() + 1; } } catch (...) { // ... } And what kind of exceptions can occur here? std::string::find, std::string::substr, std::map::insert, std::string::operator+= std::string::size these functions do not throw exceptions... 7. In class boost::process::detail::basic_status_service. There is a type 'boost::unique_lock<boost::mutex>' that used many times. May be typedef? 8. In some places of code you using std::algorithms. You can use boost::range::algorithms instead, for simplicity. Compare: std::find(impls_.begin(), impls_.end(), impl); and: boost::range::find( impls_, impl ); 9. You can replace some 'native' cycles by BOOST_FOREACH. 10. You can initialize map easier: Compare: std::map<stream_id, handle> parent_ends; parent_ends[stdin_id] = handles[stdin_id].parent; parent_ends[stdout_id] = handles[stdout_id].parent; parent_ends[stderr_id] = handles[stderr_id].parent; and: using namespace boost::assign; std::map<stream_id, handle> parent_ends = map_list_of( stdin_id, handles[stdin_id].parent ) ( stdout_id, handles[stdout_id].parent ) ( stderr_id, handles[stderr_id].parent ) ; 11. In many places of code you using std::map. May be better to use boost::unordered_map, for optimizing the speed of searching? Best regards, Denis.
On Tue, 08 Feb 2011 12:50:59 +0500, Denis Shevchenko <for.dshevchenko@gmail.com> wrote:
[snip]
3. In constructor of class boost::process::context.
In this place you insert a values into map, but not search them. So use insert() instead of operator[].
+1
Of course, with boost::assign ;-)
-1. No need to add a dependency in such simple case
[snip]
6. In function boost::process::self::get_environment().
environment e; // ... try { char *env = ms_environ; while (*env) { std::string s = env; std::string::size_type pos = s.find('='); e.insert(environment::value_type(s.substr(0, pos), s.substr(pos + 1))); env += s.size() + 1; } } catch (...) { // ... }
And what kind of exceptions can occur here? std::string::find, std::string::substr, std::map::insert, std::string::operator+= std::string::size these functions do not throw exceptions...
???
[snip]
8. In some places of code you using std::algorithms. You can use boost::range::algorithms instead, for simplicity.
Compare:
std::find(impls_.begin(), impls_.end(), impl);
and:
boost::range::find( impls_, impl );
9. You can replace some 'native' cycles by BOOST_FOREACH.
10. You can initialize map easier:
[snip]
11. In many places of code you using std::map. May be better to use boost::unordered_map, for optimizing the speed of searching?
see above
08.02.2011 12:37, Ilya Sokolov пишет:
On Tue, 08 Feb 2011 12:50:59 +0500, Denis Shevchenko <for.dshevchenko@gmail.com> wrote:
Of course, with boost::assign ;-)
-1. No need to add a dependency in such simple case
Hm... Dependency from widely used header-only library? So what? Is it problem?
[snip]
6. In function boost::process::self::get_environment().
environment e; // ... try { char *env = ms_environ; while (*env) { std::string s = env; std::string::size_type pos = s.find('='); e.insert(environment::value_type(s.substr(0, pos), s.substr(pos + 1))); env += s.size() + 1; } } catch (...) { // ... }
And what kind of exceptions can occur here? std::string::find, std::string::substr, std::map::insert, std::string::operator+= std::string::size these functions do not throw exceptions...
???
In 'try scope' placed code that never throws... - Denis
On Tue, 08 Feb 2011 13:47:50 +0500, Denis Shevchenko <for.dshevchenko@gmail.com> wrote:
08.02.2011 12:37, Ilya Sokolov пишет:
On Tue, 08 Feb 2011 12:50:59 +0500, Denis Shevchenko <for.dshevchenko@gmail.com> wrote:
Of course, with boost::assign ;-)
-1. No need to add a dependency in such simple case
Hm... Dependency from widely used header-only library?
What dependencies that 'widely used header-only library' has?
So what? Is it problem?
If not, than why not to create boost.hpp header for convenience?
[snip]
6. In function boost::process::self::get_environment().
environment e; // ... try { char *env = ms_environ; while (*env) { std::string s = env; std::string::size_type pos = s.find('='); e.insert(environment::value_type(s.substr(0, pos), s.substr(pos + 1))); env += s.size() + 1; } } catch (...) { // ... }
And what kind of exceptions can occur here? std::string::find, std::string::substr, std::map::insert,
bad_alloc
std::string::operator+=
Don't see it called somewhere.
std::string::size these functions do not throw exceptions...
???
In 'try scope' placed code that never throws...
And std::string::string is missing in your analysis, which also throws bad_alloc. See also 'Improving the standard library’s exception specifications' by Rani Sharoni http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2009/n2815.html '... The current standard library’s exception specifications seems to be lacking in the sense that many operations that expected to have no-fail guarantee under certain conditions are not explicitly specified as such. ...'
On Tue, 08 Feb 2011 08:50:59 +0100, Denis Shevchenko <for.dshevchenko@gmail.com> wrote:
[...] Macros will be easier for maintenance if align backlslashes. Compare:
#define BOOST_PROCESS_THROW_LAST_SYSTEM_ERROR(what) \ boost::throw_exception(boost::system::system_error( \ boost::system::error_code(BOOST_PROCESS_LAST_ERROR, \ boost::system::get_system_category()), \ BOOST_PROCESS_SOURCE_LOCATION what))
and
#define BOOST_PROCESS_THROW_LAST_SYSTEM_ERROR(what) \ boost::throw_exception(boost::system::system_error( \ boost::system::error_code(BOOST_PROCESS_LAST_ERROR, \ boost::system::get_system_category()), \ BOOST_PROCESS_SOURCE_LOCATION what))
Are you saying that the second version is better? I wonder as I see the first version in the code?
3. In constructor of class boost::process::context.
In this place you insert a values into map, but not search them. So use insert() instead of operator[]. Of course, with
Ok.
4. In some classes you use public inheritance from boost::noncopyable, but you can use private inheritance instead.
True, will also be changed.
5. In constructors of some classes you forgot 'explicit'.
Where do you think we should add 'explicit'?
[...]11. In many places of code you using std::map. May be better to use boost::unordered_map, for optimizing the speed of searching?
Again true. I think in most cases ordering doesn't matter. I've added everything to my list of proposed changes (trying to keep track of everything :). I'll send the complete list at the end of the review as there will be probably some more ideas until then. Thanks, Boris
On Mon, 07 Feb 2011 19:22:24 +0100, Steven Watanabe <watanabesj@gmail.com> wrote:
[...]* create_child: since the argument is the path to the file, why don't you take a boost::path, instead of a string?
There is no special reason - I'm fine with anything. I remember that some had argued to depend on as few Boost libraries as possible but that's not necessarily my opinion. boost::path would be fine for me.
* It seems like redirecting a stream to a file should be supported. Maybe it can be done with inherit, but it isn't immediately obvious and I'd rather not deal with handle::native_type at all.
Ok, a stream behavior to redirect to a file could be provided by default.
[...]* Why do you have to pass a native handle to pipe's constructor. Can't it be overloaded to take a handle?
boost::process::pipe is a typedef for boost::asio::posix::stream_descriptor (POSIX) or boost::asio::windows::stream_handle (Windows). As these classes belong to Boost.Asio they don't know boost::process::handle (and even request ownership of native handles).
* "On POSIX systems you must use the macros defined in sys/wait.h to interpret exit codes."
This forces me to use #ifdefs in my code. Isn't there a way to abstract this better? At least you could wrap the macros and have a trivial implementation for windows (WIFEXITED -> true)
I'm not sure whether we can abstract away what I think is a conceptual difference. No matter what we do one developer has to do something extra: The POSIX guy has to use macros and the Windows guy has to call a dummy function which always returns true before he can evaluate the exit code. While it's both not nice I find the macros less obfuscating (as it's POSIX which requires to do something extra). If there is a standard approach to handle WIFEXITED though (what is typically done if WIFEXITED returns false?) maybe we can support this?
[...]* What is the behavior of find_executable_in_path W.R.T. extensions. On windows, does it use PATHEXT? I'd like to see this spelled out explicitly.
This reminds me of an email I had received a while back. The Windows implementation of find_executable_in_path() must be changed. It's currently based on SearchPathA() but we probably have to use FindFirstFile() and FindNextFile(). Then we can also do what we like and use PATHEXT for example.
* What about file associations/#!xxx for create_child? i.e. can I run create_child("script.pl") or do I need to use shell? I can live with it either way, but the reference needs to specify it.
I should test this myself (commands are passed to "/bin/sh -c"). Thanks, Boris
On Wed, 09 Feb 2011 05:14:59 +0500, Boris Schaeling <boris@highscore.de> wrote:
On Mon, 07 Feb 2011 19:22:24 +0100, Steven Watanabe [snip]
* "On POSIX systems you must use the macros defined in sys/wait.h to interpret exit codes."
This forces me to use #ifdefs in my code. Isn't there a way to abstract this better? At least you could wrap the macros and have a trivial implementation for windows (WIFEXITED -> true)
I'm not sure whether we can abstract away what I think is a conceptual difference. No matter what we do one developer has to do something extra: The POSIX guy has to use macros and the Windows guy has to call a dummy function which always returns true before he can evaluate the exit code. While it's both not nice I find the macros less obfuscating (as it's POSIX which requires to do something extra).
+1
[snip]
[...]* What is the behavior of find_executable_in_path W.R.T. extensions. On windows, does it use PATHEXT? I'd like to see this spelled out explicitly.
This reminds me of an email I had received a while back. The Windows implementation of find_executable_in_path() must be changed. It's currently based on SearchPathA() but we probably have to use FindFirstFile() and FindNextFile(). Then we can also do what we like and use PATHEXT for example.
IFAIK, SearchPath() supports PATHEXT through the lpExtension parameter: http://msdn.microsoft.com/en-us/library/aa365527(v=vs.85).aspx Also see the 'Remarks' section about SafeProcessSearchMode and the docs on NeedCurrentDirectoryForExePath() function: http://msdn.microsoft.com/en-us/library/ms684269(v=vs.85).aspx
[snip]
Hi, for the moment I have started with the documentation. Here there are some remarks and questions: I would prefer to see in the Synopsis which functions are available on which specific platforms. Instead of process(handle); document #if defined(BOOST_WINDOWS_API) process(handle); #endif so it will be more clear what can be used. It is not documented which exceptions can be thrown, if any. Should a lib a Boost.Process use the Boost.System library and report failures following a coherent approach? I encapsulate together the executable name and the arguments in a command_line class. The documentation must describe explicitly the requirements of the used concepts. [Context] Requirements must be documented [Arguments] Requirements must be documented and allow efficient implementations. (recall my post [process] Arguments and Context] see bellow) [environment/context::streams_t] This class is too concrete. I would prefer to have a concept behind and that allow efficient implementations. [StreamBehavior] Requirements must be documented [handle] handle is more than RAII, it tracks shared ownership to the native handle but this is not stated clearly on the doc. The sentence "user needs to use release() to transfer ownership" lets think to some kind of move semantics. Does it means that the access to handles that had shared ownership will have access to an invalid handle after ownership has been transferred? It's like if we had a shared_ptr<unique_ptr<>>. Do we really need shared ownership? Isn't unique ownership enough? What is behind a handle, a process handle or a stream end handle? The fact that the underlying interface manages with non-typed handles, doesn't means that the C++ interface must follows the un-typed form. We can define a strong-type interface to avoid bad uses that will result in run-time errors. Until we need to store heterogeneous handles, I would prefer specific types for each type of handle. Shouldn't the public constructor from a native_handle be reserved to the implementation? What about using the bool idiom conversion instead of valid()? If the role of handle is to close the native handle at construction, when do we need to create handles with dont_close? Who will close the native handle? What happens when it closes the native handle and we access to one of this orphaned handles? private functions like invalid_handle don't need to be documented. Shouldn't the nested impl class be private? I don't know if we can have a better design by separating the handle class into the handle itself and a class that provides the kind of ownership we need to have (either shared either unique) [process::process] How can I create a process instance? From where I can get the pid_type or the handle? I think that these constructors must be protected. What about renaming it base_process to avoid ambiguities with the namespace. [child]
From where I can get the constructor pid_type ? I think that this constructor must be protected and the create_child function declared friend.
[pid_type] Which are the operations that can be applied on this type? What about renaming it to native_pid_type, so the user see that he is manipulating a non portable entity. Can pid_type be shared between process? [pistream/postream] class pistream : public std::istream, public boost::noncopyable * inheritance missing on the documentation. * constructor must be documented explicit [status] inheritance missing from the documentation : public boost::asio::basic_io_object<Service> The reference interface don't show how it can be constructed? [async_pipe] The documentation shouldn't show this typedef as in windows it is not the case. typedef pipe async_pipe; Does it means that on POSIX systems pipe and async_pipe is the same? [executable_to_progname] What about executable_to_program_name? [pipe] pipe is defined like typedef boost::asio::posix::stream_descriptor pipe; or typedef boost::asio::windows::stream_handle pipe; Do these classes provide the same interface? If not it is not good to give them the same name, by the same reason asio has not done it? [std_stream_id/stream_id] What about defining an opaque class that accepts only the values stdin_id, stdout_id, stderr_id on Windows and more on Posix systems? class stream_id { enum type { stdin, stdout, stderr }; #if defined WINDOWS typedef type value_type; #elif defined POSIX typedef int value_type; #else #error #endif stream_id(); // incalid id stream_id(value_type v); // check for negative values on Posix #ifdef POSIX stream_id(type v); // don't need to check #endif operator value_type(); ... }; [self] How self().wait() behaves? Blocks forever? throw exception? We have already ::terminate(). Why do we need self().terminate()? Doesn't Boost.Filesystem provides already get_work_dir, current_path? How do you change to another directory? rename get_work_dir to get_working_directory? What about Boost.ProgramOptions environment_iterator class instead of get_environment()? Is get_instance() really needed? If yes, is it the implementation thread safe? The single function that left is get_id(). What about moving it to the namespace level and just remove this class? Best, Vicente Extract from post [process] Arguments and Context concepts http://boost.2283326.n4.nabble.com/process-Arguments-and-Context-concepts-tt... The problem I see is that you are using std::string in the public data types of the fields, which avoids to have an efficient implementation and requiring containers that have nothing to be with the ones needed by the underlying platform interface. The current function to create follows this prototype: template<typename Arguments, typename Context> child create_child(const std::string & executable, Arguments args, Context ctx); Arguments must be a container of std::string, and the process_name will be inserted at the beginning of the container if given explicitly on the Context. Context has 3 fields that depend on std::string std::string process_name; std::string work_dir; environment env; env field must have as type an associative container mapping std::string to std::string. I would move the process_name data to the Arguments concept. For what I have see on the implementation there are two main way to pass arguments at process creation: C-like: using const char** args Windows: Using a const char* command line following a specific grammar I would try to abstract both strategies in something like template <std::size_t N=0> struct arguments { arguments(); arguments(const char* process_name); ~arguments(); const char* get_process_name() const; void set_process_name(const char* process_name); void add(const char* arg); const char* set_command_line(); const char* get_command_line(); std::size_t argc() const; const char** args() const; private: std::size_t argc_; const char* args_[N+1]; const char* command_line_; bool args_must_be_released_; bool command_line_must_be_released_; }; Users that use to work on C-like systems could use the add() function. arguments <2> args("pn"); args.add("-l"); args.add("-r"); create_child(find_executable_in_path("pn"), args); Windows users will prefer the set_command_line function. arguments <2> args(); args.set_command_line("pn -l -r"); For these two use cases, the preceding class can be implemented in a way that is as efficient as possible avoiding copies, allocations, releases. User that write portable programs would need to choose between one of the two ways to pass the arguments. Of course the program could use some conditional compilation that could use the most efficient. If the user uses the add interface on Windows, the implementation will be as efficient as now. If the user uses the set_command_line on c-like systems, the class need to tokenize the arguments. Copies, allocation and releases will be needed in these cases as it is done now. The same applies to the Environment Concept. Whether we have multiple overload, we use an essence pattern or Boost.Parameter can be decided later. What we need are two concepts for Arguments and Environment that can be implemented as efficiently as we would do when using the platform specific interfaces. -- View this message in context: http://boost.2283326.n4.nabble.com/Subject-Formal-Review-of-Proposed-Boost-P... Sent from the Boost - Dev mailing list archive at Nabble.com.
AMDG On 2/7/2011 11:09 AM, Vicente Botet wrote:
for the moment I have started with the documentation. Here there are some remarks and questions:
I would prefer to see in the Synopsis which functions are available on which specific platforms. Instead of
process(handle);
document
#if defined(BOOST_WINDOWS_API) process(handle); #endif
so it will be more clear what can be used.
This isn't possible with the Doxygen/BoostBook toolchain. In Christ, Steven Watanabe
Steven Watanabe-4 wrote:
AMDG
On 2/7/2011 11:09 AM, Vicente Botet wrote:
for the moment I have started with the documentation. Here there are some remarks and questions:
I would prefer to see in the Synopsis which functions are available on which specific platforms. Instead of
process(handle);
document
#if defined(BOOST_WINDOWS_API) process(handle); #endif
so it will be more clear what can be used.
This isn't possible with the Doxygen/BoostBook toolchain.
Hrrr. It's a shame :( Well I will then be happy with a table compiling the differences. Vicente -- View this message in context: http://boost.2283326.n4.nabble.com/Subject-Formal-Review-of-Proposed-Boost-P... Sent from the Boost - Dev mailing list archive at Nabble.com.
AMDG On 2/7/2011 11:09 AM, Vicente Botet wrote:
private functions like invalid_handle don't need to be documented. Shouldn't the nested impl class be private?
impl is private. I usually wrap private members in \cond \endcond to avoid this. It should probably be fixed in the doxygen to boostbook conversion, however.
[pistream/postream] class pistream : public std::istream, public boost::noncopyable
* inheritance missing on the documentation.
It's lost by doxygen. I don't know whether there's an easy way to fix it.
* constructor must be documented explicit
Fixed in trunk BoostBook. In Christ, Steven Watanabe
Steven Watanabe-4 wrote:
AMDG
On 2/7/2011 11:09 AM, Vicente Botet wrote:
private functions like invalid_handle don't need to be documented. Shouldn't the nested impl class be private?
impl is private. I usually wrap private members in \cond \endcond to avoid this. It should probably be fixed in the doxygen to boostbook conversion, however.
There are no \cond \endcond on the code. Doesn't Doxigen have a generation flag to state which level is documented?
[pistream/postream] class pistream : public std::istream, public boost::noncopyable
* inheritance missing on the documentation.
It's lost by doxygen. I don't know whether there's an easy way to fix it. Maybe we need to include a specific forward declaration file while building the documentation with Doxigen?
* constructor must be documented explicit
Fixed in trunk BoostBook. It would be great if we had a wiki page on which we state explicitly all the limitation and good usage of Doxigen. Thanks, Vicente -- View this message in context: http://boost.2283326.n4.nabble.com/Subject-Formal-Review-of-Proposed-Boost-P... Sent from the Boost - Dev mailing list archive at Nabble.com.
On Tue, 08 Feb 2011 00:09:25 +0500, Vicente Botet <vicente.botet@wanadoo.fr> wrote:
Hi,
[snip]
[handle] handle is more than RAII, it tracks shared ownership to the native handle but this is not stated clearly on the doc. The sentence "user needs to use release() to transfer ownership" lets think to some kind of move semantics. Does it means that the access to handles that had shared ownership will have access to an invalid handle after ownership has been transferred? It's like if we had a shared_ptr<unique_ptr<>>. Do we really need shared ownership? Isn't unique ownership enough?
It is enough, I think, but we still don't have move-enabled containers.
What is behind a handle, a process handle or a stream end handle? The fact that the underlying interface manages with non-typed handles, doesn't means that the C++ interface must follows the un-typed form. We can define a strong-type interface to avoid bad uses that will result in run-time errors. Until we need to store heterogeneous handles, I would prefer specific types for each type of handle.
Boost.Process uses bp::handle publicly as file (descriptor/handle) only. Could we rename it to 'file' then?
Shouldn't the public constructor from a native_handle be reserved to the implementation?
What about using the bool idiom conversion instead of valid()?
If the role of handle is to close the native handle at construction, when do we need to create handles with dont_close? Who will close the native handle?
Nobody in some cases, and that is not a problem (for the result of GetStdHandle() function, at least).
[snip]
[process::process] How can I create a process instance? From where I can get the pid_type or the handle? I think that these constructors must be protected.
What about renaming it base_process to avoid ambiguities with the namespace.
Or, better yet, rename the namespace with plural 'processes'.
[child]
From where I can get the constructor pid_type ? I think that this constructor must be protected and the create_child function declared friend.
That will increase coupling.
[pid_type] Which are the operations that can be applied on this type? What about renaming it to native_pid_type, so the user see that he is manipulating a non portable entity.
I suggest to drop process::id_ member of type pid_type and add process::native_ of type native_type, typedefed to int/HANDLE;
[snip]
typedef pipe async_pipe;
Does it means that on POSIX systems pipe and async_pipe is the same?
Yes
[executable_to_progname] What about executable_to_program_name?
[pipe] pipe is defined like
typedef boost::asio::posix::stream_descriptor pipe;
or
typedef boost::asio::windows::stream_handle pipe;
bp::pipe is documented as 'type definition for stream-based classes defined by Boost.Asio', so why it can't be named 'asio_stream'?
Do these classes provide the same interface? If not it is not good to give them the same name, by the same reason asio has not done it?
[std_stream_id/stream_id]
What about defining an opaque class that accepts only the values stdin_id, stdout_id, stderr_id on Windows and more on Posix systems?
[snip] - I answered this on another reply.
[self]
How self().wait() behaves? Blocks forever? throw exception?
We have already ::terminate(). Why do we need self().terminate()?
Doesn't Boost.Filesystem provides already get_work_dir, current_path? How do you change to another directory? rename get_work_dir to get_working_directory?
What about Boost.ProgramOptions environment_iterator class instead of get_environment()?
Is get_instance() really needed? If yes, is it the implementation thread safe?
The single function that left is get_id(). What about moving it to the namespace level and just remove this class?
+1. BTW, bp::self::get_work_dir() relies on BOOST_PROCESS_POSIX_PATH_MAX, but boost::filesystem::initial_path() doesn't.
Extract from post [process] Arguments and Context concepts http://boost.2283326.n4.nabble.com/process-Arguments-and-Context-concepts-tt...
[snip]
Windows users will prefer the set_command_line function.
I doubt it ), IMO one interface is enough.
[snip]
User that write portable programs would need to choose between one of the two ways to pass the arguments. Of course the program could use some conditional compilation that could use the most efficient.
If the user uses the add interface on Windows, the implementation will be as efficient as now.
No need to be efficient here, as all savings will be eaten by the CreateProcess() call.
If the user uses the set_command_line on c-like systems, the class need to tokenize the arguments.
(split_winmain() and split_unix() in boost::program_options)
[snip]
See my alternative implementation in the attachment.
Ilya Sokolov-2 wrote:
On Tue, 08 Feb 2011 00:09:25 +0500, Vicente Botet <vicente.botet@wanadoo.fr> wrote:
Hi,
[snip]
[handle] handle is more than RAII, it tracks shared ownership to the native handle but this is not stated clearly on the doc. The sentence "user needs to use release() to transfer ownership" lets think to some kind of move semantics. Does it means that the access to handles that had shared ownership will have access to an invalid handle after ownership has been transferred? It's like if we had a shared_ptr<unique_ptr<>>. Do we really need shared ownership? Isn't unique ownership enough?
It is enough, I think, but we still don't have move-enabled containers.
Great to see that unique ownership could be enough. We will have move-enabled containers soon with Boost.Move and Boost.Containers, at least I hope so.
What is behind a handle, a process handle or a stream end handle? The fact that the underlying interface manages with non-typed handles, doesn't means that the C++ interface must follows the un-typed form. We can define a strong-type interface to avoid bad uses that will result in run-time errors. Until we need to store heterogeneous handles, I would prefer specific types for each type of handle.
Boost.Process uses bp::handle publicly as file (descriptor/handle) only. Could we rename it to 'file' then?
The handle class is used as parameter of process and stream_ends if I'm not wrong. Couldn't we have two classes so we don't use a stream handle to construct a process and vice-versa?
Shouldn't the public constructor from a native_handle be reserved to the implementation?
Could you comment this.
What about using the bool idiom conversion instead of valid()?
If the role of handle is to close the native handle at construction, when do we need to create handles with dont_close? Who will close the native handle?
Nobody in some cases, and that is not a problem (for the result of GetStdHandle() function, at least).
Do you mean that the close of the handle is not really needed in this specific case or always?
[snip]
[process::process] How can I create a process instance? From where I can get the pid_type or the handle? I think that these constructors must be protected.
What about renaming it base_process to avoid ambiguities with the namespace.
Or, better yet, rename the namespace with plural 'processes'.
[child]
From where I can get the constructor pid_type ? I think that this constructor must be protected and the create_child function declared friend.
That will increase coupling.
You are right, but we don't have a portable way to get pid_type and the user is not intended to use these constructors. If this is a public interface, the user could be tempted to use them and will need to get the pid_type using non portable interfaces, which we should avoid.
[pid_type] Which are the operations that can be applied on this type? What about renaming it to native_pid_type, so the user see that he is manipulating a non portable entity.
I suggest to drop process::id_ member of type pid_type and add process::native_ of type native_type, typedefed to int/HANDLE;
Great.
[executable_to_progname] What about executable_to_program_name?
[pipe] pipe is defined like
typedef boost::asio::posix::stream_descriptor pipe;
or
typedef boost::asio::windows::stream_handle pipe;
bp::pipe is documented as 'type definition for stream-based classes defined by Boost.Asio', so why it can't be named 'asio_stream'?
Do these classes provide the same interface? If not it is not good to give them the same name, by the same reason asio has not done it?
Could you comment this? IMHO any class that has a specific interface should be prefixed with native as the user could need conditional compilation that depends on the platform to use it.
[self]
How self().wait() behaves? Blocks forever? throw exception?
We have already ::terminate(). Why do we need self().terminate()?
Doesn't Boost.Filesystem provides already get_work_dir, current_path? How do you change to another directory? rename get_work_dir to get_working_directory?
What about Boost.ProgramOptions environment_iterator class instead of get_environment()?
Is get_instance() really needed? If yes, is it the implementation thread safe?
The single function that left is get_id(). What about moving it to the namespace level and just remove this class?
+1.
BTW, bp::self::get_work_dir() relies on BOOST_PROCESS_POSIX_PATH_MAX, but boost::filesystem::initial_path() doesn't.
Great.
Extract from post [process] Arguments and Context concepts http://boost.2283326.n4.nabble.com/process-Arguments-and-Context-concepts-tt...
[snip]
Windows users will prefer the set_command_line function.
I doubt it ), IMO one interface is enough.
We disagree here ;-) Why the container based interface would be always better than the command_line?
[snip]
User that write portable programs would need to choose between one of the two ways to pass the arguments. Of course the program could use some conditional compilation that could use the most efficient.
If the user uses the add interface on Windows, the implementation will be as efficient as now.
No need to be efficient here, as all savings will be eaten by the CreateProcess() call.
You mean we don't need to be efficient here, isn't it? You are right, the time taken by the process creation is some order magnitude the time can be spared.
If the user uses the set_command_line on c-like systems, the class need to tokenize the arguments.
(split_winmain() and split_unix() in boost::program_options)
Do you mean that the user can use these functions in his application, using conditional compilation?
See my alternative implementation in the attachment.
This is better, but it forces the implementation to return the arguments as a std::vector<std::string>. I would move the functions that make the decoding and let the interface return a char**, that is what the implementation needs. const char** arguments() const Best, Vicente -- View this message in context: http://boost.2283326.n4.nabble.com/Subject-Formal-Review-of-Proposed-Boost-P... Sent from the Boost - Dev mailing list archive at Nabble.com.
On Tue, 08 Feb 2011 14:22:08 +0500, Vicente Botet <vicente.botet@wanadoo.fr> wrote:
Ilya Sokolov-2 wrote:
Boost.Process uses bp::handle publicly as file (descriptor/handle) only. Could we rename it to 'file' then?
The handle class is used as parameter of process
as implementation detail
and stream_ends if I'm notwrong. Couldn't we have two classes so we don't use a stream handle to construct a process and vice-versa?
I'm fine with it as is, except of the name.
Shouldn't the public constructor from a native_handle be reserved to the implementation?
Could you comment this.
IMO no need to be too clever/restrictive.
If the role of handle is to close the native handle at construction, when do we need to create handles with dont_close? Who will close the native handle?
Nobody in some cases, and that is not a problem (for the result of GetStdHandle() function, at least).
Do you mean that the close of the handle is not really needed in this specific case or always?
Not sure what you mean by 'always', but in this specific case (GetStdHandle()) it is strictly not needed.
we don't have a portable way to get pid_type and the user is not intended to use these constructors.
Why?
If this is a public interface, the user could be tempted to use them and will need to get the pid_type using non portable interfaces,
or portable interfaces written by himself for his own purposes
which we should avoid.
No
[pipe] pipe is defined like
typedef boost::asio::posix::stream_descriptor pipe;
or
typedef boost::asio::windows::stream_handle pipe;
bp::pipe is documented as 'type definition for stream-based classes defined by Boost.Asio', so why it can't be named 'asio_stream'?
Do these classes provide the same interface?
Almost (at least async_[read/write]_some are identical)
If not it is not good to give them the same name, by the same reason asio has not done it?
It is hard to convince windows/posix guy to name something stream_descriptor/stream_handle, accordingly ).
Could you comment this? IMHO any class that has a specific interface should be prefixed with native as the user could need conditional compilation that depends on the platform to use it.
I'm fine with or without 'native_' prefix.
Extract from post [process] Arguments and Context concepts http://boost.2283326.n4.nabble.com/process-Arguments-and-Context-concepts-tt...
[snip]
Windows users will prefer the set_command_line function.
I doubt it ), IMO one interface is enough.
We disagree here ;-) Why the container based interface would be always better than the command_line?
I don't know anybody who wishes to remember quoting rules.
[snip]
User that write portable programs would need to choose between one of the two ways to pass the arguments. Of course the program could use some conditional compilation that could use the most efficient.
If the user uses the add interface on Windows, the implementation will be as efficient as now.
No need to be efficient here, as all savings will be eaten by the CreateProcess() call.
You mean we don't need to be efficient here, isn't it? You are right, the time taken by the process creation is some order magnitude the time can be spared.
Yes.
If the user uses the set_command_line on c-like systems, the class need to tokenize the arguments.
(split_winmain() and split_unix() in boost::program_options)
Do you mean that the user can use these functions in his application, using conditional compilation?
Yes, and passing proper separators, quotes, etc to split_unix().
See my alternative implementation in the attachment.
This is better, but it forces the implementation to return the arguments as a std::vector<std::string>. I would move the functions that make the decoding and let the interface return a char**, that is what the implementation needs.
const char** arguments() const
Who will own the result?
Ilya Sokolov-2 wrote:
On Tue, 08 Feb 2011 14:22:08 +0500, Vicente Botet <vicente.botet@wanadoo.fr> wrote:
Ilya Sokolov-2 wrote:
Boost.Process uses bp::handle publicly as file (descriptor/handle) only. Could we rename it to 'file' then?
The handle class is used as parameter of process
as implementation detail
and stream_ends if I'm not wrong. Couldn't we have two classes so we don't use a stream handle to construct a process and vice-versa?
I'm fine with it as is, except of the name.
Shouldn't the public constructor from a native_handle be reserved to the implementation?
Could you comment this.
IMO no need to be too clever/restrictive.
If the role of handle is to close the native handle at construction, when do we need to create handles with dont_close? Who will close the native handle?
we don't have a portable way to get pid_type and the user is not intended to use these constructors.
Why?
If this is a public interface, the user could be tempted to use them and will need to get the pid_type using non portable interfaces,
or portable interfaces written by himself for his own purposes
which we should avoid.
No
In this case all these interface need to document in which state the native type need to be. For example, if I'm not wrong, the handle class expect its native handle to be open, isn't it?
[pipe] pipe is defined like
typedef boost::asio::posix::stream_descriptor pipe;
or
typedef boost::asio::windows::stream_handle pipe;
bp::pipe is documented as 'type definition for stream-based classes defined by Boost.Asio', so why it can't be named 'asio_stream'?
Do these classes provide the same interface?
Almost (at least async_[read/write]_some are identical)
If not it is not good to give them the same name, by the same reason asio has not done it?
It is hard to convince windows/posix guy to name something stream_descriptor/stream_handle, accordingly ).
I would have no problem to accept bp::windows::pipe and bp::posix::pipe.
Extract from post [process] Arguments and Context concepts http://boost.2283326.n4.nabble.com/process-Arguments-and-Context-concepts-tt...
[snip]
Windows users will prefer the set_command_line function.
I doubt it ), IMO one interface is enough.
We disagree here ;-) Why the container based interface would be always better than the command_line?
I don't know anybody who wishes to remember quoting rules.
Note that not all the command lines need quotation, and most of the times they don't need it. At least for this case the set_command_line will simplify the user task.
See my alternative implementation in the attachment.
This is better, but it forces the implementation to return the arguments as a std::vector<std::string>. I would move the functions that make the decoding and let the interface return a char**, that is what the implementation needs.
const char** arguments() const
Who will own the result?
The command_line instance of course. This is the same as string::c_str() which returns const char* and owns its data. Best, Vicente -- View this message in context: http://boost.2283326.n4.nabble.com/Subject-Formal-Review-of-Proposed-Boost-P... Sent from the Boost - Dev mailing list archive at Nabble.com.
This is only a short review. I installed boost.process on ubuntu and ran some of the examples. The library appears well documented and works correctly. I have previously used a process library I created myself. Boost process has much more functionality than my library, and I will use it if it accepted into boost. I would prefer it if the library had some simple helpers for simple uses. In particular I would like an example which runs a simple program like "ls" and captures all output and the return value. In conclusion, I would believe from my brief review boost.process should be accepted. I would like to see more examples showing simple uses, and if these examples turn out to be more than a couple of lines, simple wrapper functions which help users with simple use cases. Chris On 7 Feb 2011, at 06:18, Marshall Clow wrote:
It's my pleasure to announce that the review of Boris Schäling's Process library starts tomorrow, February 7th and lasts until February 20, 2011, unless an extension occurs.
What is it? ===========
Boost.Process is a library to manage system processes. It can be used to:
• create child processes • run shell commands • setup environment variables for child processes • setup standard streams for child processes (and other streams on POSIX platforms) • communicate with child processes through standard streams (synchronously or asynchronously) • wait for processes to exit (synchronously or asynchronously) • terminate processes
Getting the library ===================
The library is available at: http://www.highscore.de/boost/gsoc2010/process.zip with documentation at: http://www.highscore.de/boost/gsoc2010/
There was a quite involved "informal review" on the mailing list this past summer. That thread is archived here: http://thread.gmane.org/gmane.comp.lib.boost.devel/207594
Writing a review ================
If you feel this is an interesting library, then please submit your review to the developer list (preferably), or to the review manager (me!)
Here are some questions you might want to answer in your review:
- What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick - reading? In-depth study? - Are you knowledgeable about the problem domain?
And finally, every review should answer this question:
- Do you think the library should be accepted as a Boost library?
Be sure to say this explicitly so that your other comments don't obscure your overall opinion.
-- Marshall Review Manager for the proposed Boost.Process library
_______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users
Hi, I want to discuss here of how transparent would be the user code to be able to use non portable extension with the current interface. The way extensions are provided means that a user needs to use the BOOST_POSIX_API, BOOST_WINDOWS_API in his own code when he uses an extension. Could the documentation include some explicitly examples. The interface of the setup callback is different so the user will need to define two possible setup functions. #if defined(BOOST_POSIX_API) void setup() { ... } #elif defined(BOOST_WINDOWS_API) void setup(STARTUPINFOA &sainfo) { ... } #endif Note that these setup functions are called on a different context either on the child either on the parent process, but both will be assigned to the same context field. Any setting of the ctx.streams with fd >2 should be protected ctx.streams[3] = ... While the preceding code doesn't have any sens under WINDOWS, the library don't provide an interface that forbids it, but the current implementation just ignore it. I'm not sure if this is good in this case as the application could not work in the same way in POSIX or WINDOWS systems. If the class stream_id I proposed in this thread is used as domain of the map, the code will not compile as a stream_id cannot be constructed from fd>2 on WINDOWS. The opposite applies to behaviors whose constructor have stream_type parameter. These constructors are available only for POSIX, but I don't think adding them will change the perception of the user has of his application. Couldn't the behaviors provide a uniform interface for these constructors? BTW all the constructor taking a stream_type as parameter are not documented. Instead of just defining them for POSIX #if defined(BOOST_POSIX_API) pipe() : stype_(unknown_stream) { } pipe(stream_type stype) : stype_(stype) { } #endif You can add the respective constructors for WINDOWS, even if the parameter is completely ignored. #if defined(BOOST_WINDOWS_API) pipe() { } pipe(stream_type /*stype*/) { } #endif The same applies to all the behaviors using stream_type. Could a table of extension be included on the documentation? Best, Vicente -- View this message in context: http://boost.2283326.n4.nabble.com/Subject-Formal-Review-of-Proposed-Boost-P... Sent from the Boost - Dev mailing list archive at Nabble.com.
On Tue, 08 Feb 2011 03:26:57 +0500, Vicente Botet <vicente.botet@wanadoo.fr> wrote:
Any setting of the ctx.streams with fd >2 should be protected
ctx.streams[3] = ...
While the preceding code doesn't have any sens under WINDOWS, the library don't provide an interface that forbids it, but the current implementation just ignore it. I'm not sure if this is good in this case as the application could not work in the same way in POSIX or WINDOWS systems. If the class stream_id I proposed in this thread is used as domain of the map, the code will not compile as a stream_id cannot be constructed from fd>2 on WINDOWS.
I think it is enough to assert on valid stream_ids in the create_child() function.
The opposite applies to behaviors whose constructor have stream_type parameter. These constructors are available only for POSIX, but I don't think adding them will change the perception of the user has of his application. Couldn't the behaviors provide a uniform interface for these constructors? BTW all the constructor taking a stream_type as parameter are not documented.
Instead of just defining them for POSIX
#if defined(BOOST_POSIX_API) pipe() : stype_(unknown_stream) { } pipe(stream_type stype) : stype_(stype) { } #endif
You can add the respective constructors for WINDOWS, even if the parameter is completely ignored.
#if defined(BOOST_WINDOWS_API) pipe() { } pipe(stream_type /*stype*/) { } #endif
The same applies to all the behaviors using stream_type.
Those constructors have a sense on windows, no need to ignore anything.
On Mon, 07 Feb 2011 23:26:57 +0100, Vicente Botet <vicente.botet@wanadoo.fr> wrote: Hi Vicente,
I want to discuss here of how transparent would be the user code to be able to use non portable extension with the current interface. The way extensions are provided means that a user needs to use the BOOST_POSIX_API, BOOST_WINDOWS_API in his own code when he uses an extension. Could the documentation include some explicitly examples.
there is an example at https://svn.boost.org/svn/boost/sandbox/SOC/2010/process/libs/process/exampl... (for POSIX) and at https://svn.boost.org/svn/boost/sandbox/SOC/2010/process/libs/process/exampl... (for Windows). If the documentation doesn't refer to them clearly I'll have to improve it. :)
The interface of the setup callback is different so the user will need to define two possible setup functions.
Yes.
[...]Note that these setup functions are called on a different context either on the child either on the parent process, but both will be assigned to the same context field.
True (and it's good that you noticed; that makes me think the documentation is not that bad :).
Any setting of the ctx.streams with fd >2 should be protected
ctx.streams[3] = ...
While the preceding code doesn't have any sens under WINDOWS, the library don't provide an interface that forbids it, but the current implementation just ignore it. I'm not sure if this is good in this case as the application could not work in the same way in POSIX or WINDOWS systems. If the class stream_id I proposed in this thread is used as domain of the map, the code will not compile as a stream_id cannot be constructed from fd>2 on WINDOWS.
I have to read your other email still. The operator[] is rather new though. We had three get functions before to access the handles for stdin, stdout and stderr of the child process. I had argued that these are the only cross-platform streams. However the counter-argument was that there was no way to pass fds > 2 to a child process on POSIX - not even with an extension point. Developers would need to look for another library to do that. So we tried to create an interface which would make sense for Windows developers and would provide developers on POSIX the flexibility they need. That's how we got here.
[...]Instead of just defining them for POSIX
#if defined(BOOST_POSIX_API) pipe() : stype_(unknown_stream) { } pipe(stream_type stype) : stype_(stype) { } #endif
You can add the respective constructors for WINDOWS, even if the parameter is completely ignored.
#if defined(BOOST_WINDOWS_API) pipe() { } pipe(stream_type /*stype*/) { } #endif
The same applies to all the behaviors using stream_type.
The use case would be to create a stand-alone pipe outside of Boost.Process? If so I wonder whether we should (additionally?) provide something else than stream behaviors. They are pretty much tailored for Boost.Process. I'm fine with making the constructors available on all platforms. But it's not nice that you get a stream_ends object with a parent and child end when you actually want to create a pipe with a read and write end?
Could a table of extension be included on the documentation?
Do you mean extension points? Or sample programs like the two above? Thanks, Boris
Boris Schaeling wrote:
On Mon, 07 Feb 2011 23:26:57 +0100, Vicente Botet <vicente.botet@wanadoo.fr> wrote:
Hi Vicente,
Hi Boris,
I want to discuss here of how transparent would be the user code to be able to use non portable extension with the current interface. The way extensions are provided means that a user needs to use the BOOST_POSIX_API, BOOST_WINDOWS_API in his own code when he uses an extension. Could the documentation include some explicitly examples.
there is an example at https://svn.boost.org/svn/boost/sandbox/SOC/2010/process/libs/process/exampl... (for POSIX) and at https://svn.boost.org/svn/boost/sandbox/SOC/2010/process/libs/process/exampl... (for Windows). If the documentation doesn't refer to them clearly I'll have to improve it. :)
I was looking for an example of portable code that uses the two functions. That is merge the two examples in one.
[...]Note that these setup functions are called on a different context either on the child either on the parent process, but both will be assigned to the same context field.
True (and it's good that you noticed; that makes me think the documentation is not that bad :).
Any setting of the ctx.streams with fd >2 should be protected
ctx.streams[3] = ...
While the preceding code doesn't have any sens under WINDOWS, the library don't provide an interface that forbids it, but the current implementation just ignore it. I'm not sure if this is good in this case as the application could not work in the same way in POSIX or WINDOWS systems. If the class stream_id I proposed in this thread is used as domain of the map, the code will not compile as a stream_id cannot be constructed from fd>2 on WINDOWS.
I have to read your other email still. The operator[] is rather new though. We had three get functions before to access the handles for stdin, stdout and stderr of the child process. I had argued that these are the only cross-platform streams. However the counter-argument was that there was no way to pass fds > 2 to a child process on POSIX - not even with an extension point. Developers would need to look for another library to do that. So we tried to create an interface which would make sense for Windows developers and would provide developers on POSIX the flexibility they need. That's how we got here.
Thanks for the exlanation. I was aware by reading the thread of september-october :)
[...]Instead of just defining them for POSIX
#if defined(BOOST_POSIX_API) pipe() : stype_(unknown_stream) { } pipe(stream_type stype) : stype_(stype) { } #endif
You can add the respective constructors for WINDOWS, even if the parameter is completely ignored.
#if defined(BOOST_WINDOWS_API) pipe() { } pipe(stream_type /*stype*/) { } #endif
The same applies to all the behaviors using stream_type.
The use case would be to create a stand-alone pipe outside of Boost.Process? If so I wonder whether we should (additionally?) provide something else than stream behaviors. They are pretty much tailored for Boost.Process. I'm fine with making the constructors available on all platforms. But it's not nice that you get a stream_ends object with a parent and child end when you actually want to create a pipe with a read and write end?
Yes this exactly why I was proposing that these constructors must be protected and used only by the implementation as the correct use can be possible as undocumented. Read on the other post.
Could a table of extension be included on the documentation?
Do you mean extension points? Or sample programs like the two above?
Extension points. As Doxigen don't allows to show it in the synopsis, a table showing the specific extensions points will help. Best, Vicente -- View this message in context: http://boost.2283326.n4.nabble.com/Subject-Formal-Review-of-Proposed-Boost-P... Sent from the Boost - Dev mailing list archive at Nabble.com.
On Wed, 09 Feb 2011 04:23:55 +0500, Vicente Botet wrote:
Boris Schaeling wrote:
[snip]
[...]Instead of just defining them for POSIX
#if defined(BOOST_POSIX_API) pipe() : stype_(unknown_stream) { } pipe(stream_type stype) : stype_(stype) { } #endif
You can add the respective constructors for WINDOWS, even if the parameter is completely ignored.
#if defined(BOOST_WINDOWS_API) pipe() { } pipe(stream_type /*stype*/) { } #endif
The same applies to all the behaviors using stream_type.
The use case would be to create a stand-alone pipe outside of Boost.Process? If so I wonder whether we should (additionally?) provide something else than stream behaviors. They are pretty much tailored for Boost.Process. I'm fine with making the constructors available on all platforms. But it's not nice that you get a stream_ends object with a parent and child end when you actually want to create a pipe with a read and write end?
Yes this exactly why I was proposing that these constructors must be protected and used only by the implementation as the correct use can be possible as undocumented.
AFAICS, you proposed something else, not that 'these constructors must be protected...'
[snip]
Ilya Sokolov-2 wrote:
On Wed, 09 Feb 2011 04:23:55 +0500, Vicente Botet wrote:
Boris Schaeling wrote:
[snip]
[...]Instead of just defining them for POSIX
#if defined(BOOST_POSIX_API) pipe() : stype_(unknown_stream) { } pipe(stream_type stype) : stype_(stype) { } #endif
You can add the respective constructors for WINDOWS, even if the parameter is completely ignored.
#if defined(BOOST_WINDOWS_API) pipe() { } pipe(stream_type /*stype*/) { } #endif
The same applies to all the behaviors using stream_type.
The use case would be to create a stand-alone pipe outside of Boost.Process? If so I wonder whether we should (additionally?) provide something else than stream behaviors. They are pretty much tailored for Boost.Process. I'm fine with making the constructors available on all platforms. But it's not nice that you get a stream_ends object with a parent and child end when you actually want to create a pipe with a read and write end?
Yes this exactly why I was proposing that these constructors must be protected and used only by the implementation as the correct use can be possible as undocumented.
AFAICS, you proposed something else, not that 'these constructors must be protected...'
You're right I didn't do it. I was mixing with what I said for other constructors that have native types as parameters. Sorry for the noise. Vicente " [process::process] How can I create a process instance? From where I can get the pid_type or the handle? I think that these constructors must be protected. [child]
From where I can get the constructor pid_type ? I think that this constructor must be protected and the create_child function declared friend.
[handle] Shouldn't the public constructor from a native_handle be reserved to the implementation? " Vicente -- View this message in context: http://boost.2283326.n4.nabble.com/Subject-Formal-Review-of-Proposed-Boost-P... Sent from the Boost - Dev mailing list archive at Nabble.com.
On Wed, 09 Feb 2011 03:57:43 +0500, Boris Schaeling wrote:
On Mon, 07 Feb 2011 23:26:57 +0100, Vicente Botet [snip]
[...]Instead of just defining them for POSIX
#if defined(BOOST_POSIX_API) pipe() : stype_(unknown_stream) { } pipe(stream_type stype) : stype_(stype) { } #endif
You can add the respective constructors for WINDOWS, even if the parameter is completely ignored.
#if defined(BOOST_WINDOWS_API) pipe() { } pipe(stream_type /*stype*/) { } #endif
The same applies to all the behaviors using stream_type.
The use case would be to create a stand-alone pipe outside of Boost.Process?
Another use case would be to support pipes between child and parent with unusual direction, i.e. stdin used for output and stdout/strerr used for input. In current version it is possible on posix, but impossible on windows.
[snip]
It's my pleasure to announce that the review of Boris Schäling's Process library starts tomorrow, February 7th and lasts until February 20, 2011, unless an extension occurs.
Hello All, --------------------------------------------------------------------- - What is your evaluation of the potential usefulness of the library? --------------------------------------------------------------------- It is potentially very useful library that its kind should exist in boost. --------------------------------------------------------------------- - Did you try to use the library? With what compiler? Did you have any problems? --------------------------------------------------------------------- I've tried to run several examples using Linux x86_64 gcc-4.3 and cross compile several examples for windows uning gcc-4.2 I had minor compilation problems with MinGW. --------------------------------------------------------------------- - How much effort did you put into your evaluation? A glance? A quick - reading? In-depth study? --------------------------------------------------------------------- I've spend about 3 hours mostly reviewing the code looking at system calls and how various operations are implemented. I was looking to use it for managing processes like staring compiler and integration it to asio's event loop. --------------------------------------------------------------------- - Are you knowledgeable about the problem domain? --------------------------------------------------------------------- I had deal with many tools/issues implemented in this library: - Process creation, termination, asynchronous waiting - Working with pipes - Working with custom streambuffers etc. --------------------------------------------------------------------- - What is your evaluation of the documentation? --------------------------------------------------------------------- I have following issues with documentation Documentation lacks of following points: 1. Rationale Part: a) Why the library was designed this way, for example: why boost::function is used in context::streams very unclear b) What tools are used for implementation so the user would understand how the underlying tools work. 2. Tutorial should be improved a little to provide better explanation of what exactly happens, but overall it is fine. 3. It should be marked id documentation explicitly that -D_WIN32_WINNT=0x501 or soemthing like that should be given I had major failures to compile Boost.Process under MinGW platform. GetProcessId and UuidCreateSequential was not found. Until I figured this missing define. --------------------------------------------------------------------- - What is your evaluation of the design? --------------------------------------------------------------------- I would mostly relate to API's design, I'll talk about implementation separately. - process object should be polymorphic, as self and child are derived and there is a good chance for using code like auto_ptr<process> p(new self()); Which may cause bad things. - The use of self::instance() is unclear: 1. It is not thread safe 2. It seems to not being destroyed 3. When should it be used over just creating self() ? - Boost.Process uses sometimes type names ended with "_t", such suffixes are reserved by POSIX in global namespace and generally should be avoided It is better to use _type as STL does. - Overall the API seems to be clean but context class requires much better explanations of its rationale. --------------------------------------------------------------------- - What is your evaluation of the implementation? --------------------------------------------------------------------- This is the major problem with the library, I had found too many faults in the implementation in too short time that finally caused me to abort more in deep review. Minor Issues: ------------- 1. Don't include <boost/asio.hpp> try to include only abolutly nessary headers. Asio is painfully long to compile so try to avoid its full inclusion Medium Level Issues: -------------------- 1. systembuf It does not handle EINTR error which something that can happen. read and write operations should be restarted in this event I had already mentioned this be previous mini-review, I hoped it would be fixed for formal review. 2. Windows ANSI API/Wide API should issues should be addressed in one of following ways: - provide wide alternative API (not so good idea) - use std::locale::facet for converting paths, command parameters to wide api using either: a) Global codecvt facet to user can install UTF-8 facet in global locale simillary to what boost::filesystem::v3 does. b) Provide encoding option for narrow strings c) Use UTF-8 - but this is out of scope of this project. However it should be handled. 3. Use of getcwd. In the code you try to discover the side of the path using pathconf. Unfortunatelly it may return values that actually can-not be allocated and be virtually unlimited. So this approach is not correct. You need to use something like this: 1. Allocate a initial buffer size (lets say 256) 2. call getcwd 3. if ok return the path 4. if errno==ERANGE 5. resize buffer to twice its current size 6. go to step 2 7. else 8. fail Major Issues - Showstoppers --------------------------- 1. Asynchronous wait design under POSIX system has major problems that can cause system to deadlock. It uses wrong system API and its implementation is wrong. How it is designed: 1. Create a thread for asio service 2. Call pid = wait(&status) <-- Problem is here 3. Notify the event loop on childs that finished. Deadlock Scenario: ------------------ 1. I start two asio::io_service instances A, B 2. I create child a and wait for it in A service. 3. I create child b and wait for it in B service. 4. io_service A gets status of children B. 5. service B never terminates as wait would not exit. Race Condition Scenario: ------------------------ 1. I start process A and start waiting for its termination Asynchronously. 2. I start process B and it terminates 3. wait() receives it status and pid is deleted 4. I wait for B Synchronously. 5. I got a error as PID does not exits. It is badly designed and has major bugs It is likely should be implemented by using sigaction on SIGCHLD and allow to actually implement it without additional thread. 2. The code of thread interruption under POSIX system, it is just no go. void interrupt_work_thread() { // By creating a child process which immediately exits // we interrupt wait(). std::vector<std::string> args; args.push_back("-c"); args.push_back("'exit'"); interrupt_pid_ = create_child("/bin/sh", args).get_id(); } a) Why do you assume that "/bin/sh" exists? What I if work in chroot enviroment? calling fork(), exit(1) would be much simpler but still I don't think you should fork at all as overall you should not use "wait()" b) Performance, starting an additonal process to make something to stop is horrible so if I start and stop io_service frequently it would be very slow. After discovering the issues above I had aborted the review process as " bug to review time ratio" was too high. --------------------------------------------------------------------- - Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. --------------------------------------------------------------------- Unfortunately, No, it shouldn't. 1. I don't think that in current state it can be included in Boost. 2. I do think that after fixing major and medium level issues it should be resubmitted for next review. 3. I would rather prefer this library to have less features that can be implemented in future but it should be 100% clear and done right. 4. I like overall design but it has too many critical flaws. 5. Boost needs such library so I do encourage the author to improve it and resubmit for the review. Best Regards, Artyom Beilis ____________________________________________________________________________________ Looking for earth-friendly autos? Browse Top Cars by "Green Rating" at Yahoo! Autos' Green Center. http://autos.yahoo.com/green_center/
hi Artyom. 1. Create a thread for asio service 2. Call pid = wait(&status) <-- Problem is here 3. Notify the event loop on childs that finished. for solved this problem two or more io_service in same process need use this system call* signalfd(2) ? *If use signalfd(2) can receive all signal and* *can possible receive SIGCHILD if a child terminate.
From: bruno romano <brommul@gmail.com>
hi Artyom.
1. Create a thread for asio service 2. Call pid = wait(&status) <-- Problem is here 3. Notify the event loop on childs that finished.
for solved this problem two or more io_service in same process need use this system call* signalfd(2) ? *If use signalfd(2) can receive all signal and* *can possible receive SIGCHILD if a child terminate.
a) signalfd is Linux specific b) In same way you can use sigwaitinfo for SIGCHLD but it still does not solve the problem. c) You need to provide a signal handler that would notify all active io_services and once they notified they should poll for current processes that it waits for them to check if they are terminated. Or some signal chaining may be used... It is not simple, but nobody says that is should be so. Artyom
On Wed, 09 Feb 2011 12:00:12 +0100, Artyom <artyomtnk@yahoo.com> wrote: Hi Artyom,
[...]I had minor compilation problems with MinGW.
I'm glad to hear that the library could be used on MinGW. There were some problems reported before but I never tested it myself on that platform.
[...]3. It should be marked id documentation explicitly that -D_WIN32_WINNT=0x501 or soemthing like that should be given
I had major failures to compile Boost.Process under MinGW platform.
Yes, it looks like Boost.Process should emit the same warning as Boost.Asio if _WIN32_WINNT isn't defined.
[...]1. systembuf
It does not handle EINTR error which something that can happen. read and write operations should be restarted in this event I had already mentioned this be previous mini-review, I hoped it would be fixed for formal review.
It's noted but no one (including me) wrote the code yet. :)
2. Windows ANSI API/Wide API should issues should be addressed in one of following ways: [...] However it should be handled.
While this is indeed something where I would also prefer a more flexible interface I'm not sure if this problem should be tackled by Boost.Process. On the one hand I'm waiting for something like Boost.Locale. On the other hand I know that Boost.Filesystem v3 provides that flexibility. But what's the recommendation for all the other Boost libraries?
3. Use of getcwd.
In the code you try to discover the side of the path using pathconf. Unfortunatelly it may return values that actually can-not be allocated and be virtually unlimited. So this approach is not correct.
You need to use something like this:
1. Allocate a initial buffer size (lets say 256) 2. call getcwd 3. if ok return the path 4. if errno==ERANGE 5. resize buffer to twice its current size 6. go to step 2 7. else 8. fail
Thanks, I'll look into it.
[...] Deadlock Scenario: ------------------ 1. I start two asio::io_service instances A, B 2. I create child a and wait for it in A service. 3. I create child b and wait for it in B service. 4. io_service A gets status of children B. 5. service B never terminates as wait would not exit.
Race Condition Scenario: ------------------------
1. I start process A and start waiting for its termination Asynchronously. 2. I start process B and it terminates 3. wait() receives it status and pid is deleted 4. I wait for B Synchronously. 5. I got a error as PID does not exits.
It is badly designed and has major bugs
The only bug I see is then in the documentation where it should be stated clearly that this is not and was never intended to be supported (as it's indeed impossible with the current implementation).
It is likely should be implemented by using sigaction on SIGCHLD and allow to actually implement it without additional thread.
I wish adding support for asynchronous operations would be much more lightweight. What we currently have is a first implementation which works (under certain restrictions like the one you described above). It's already bad enough that the implementation is so different for Windows and POSIX. I guess there can be even more implementations as some platforms support some system calls we could use (like signalfd Bruno mentioned). However I don't think using SIGCHLD is a good idea. As a library developer I don't really want to steal the signal handler for any signals as we have no idea if there are other parts in a program which actually wait for SIGCHLD, too. We don't know either if the signal handler is reset after we set it. It should be really the application and not external libraries which manage signals. As there can be even more problems like blocked signals using Boost.Process could affect a program in ways a developer would probably not even think about.
2. The code of thread interruption under POSIX system, it is just no go.
void interrupt_work_thread() { // By creating a child process which immediately exits // we interrupt wait(). std::vector<std::string> args; args.push_back("-c"); args.push_back("'exit'"); interrupt_pid_ = create_child("/bin/sh", args).get_id(); } a) Why do you assume that "/bin/sh" exists? What I if work in chroot enviroment?
calling fork(), exit(1) would be much simpler but still I don't think you should fork at all as overall you should not use "wait()"
b) Performance, starting an additonal process to make something to stop is horrible so if I start and stop io_service frequently it would be very slow.
Yes, it's horrible. Still I was glad to interrupt the blocking call to wait() at least somehow. Again I wish there was a more suitable POSIX interface I could use to emulate an asynchronous wait operation. But the system calls are as they are. If there are no other ideas and most of you feel that such a horrible implementation should indeed not be shipped with a Boost library, support for the asynchronous wait operation can always be removed. The whole implementation is bit better on Windows. But it would probably not make much sense to support an asynchronous wait operation for one platform only. Thanks, Boris
[...]
[...] Deadlock Scenario: ------------------ 1. I start two asio::io_service instances A, B 2. I create child a and wait for it in A service. 3. I create child b and wait for it in B service. 4. io_service A gets status of children B. 5. service B never terminates as wait would not exit.
Race Condition Scenario: ------------------------
1. I start process A and start waiting for its termination Asynchronously. 2. I start process B and it terminates 3. wait() receives it status and pid is deleted 4. I wait for B Synchronously. 5. I got a error as PID does not exits.
It is badly designed and has major bugs
The only bug I see is then in the documentation where it should be stated clearly that this is not and was never intended to be supported (as it's indeed impossible with the current implementation).
I think if you can't implement this properly just don't implement. IMHO it is important to have less features that work then half working onces.
It is likely should be implemented by using sigaction on SIGCHLD and allow to actually implement it without additional thread.
[snip]
It's already bad enough that the implementation is so different for Windows and POSIX. I guess there can be even more implementations as some platforms support some system calls we could use (like signalfd Bruno mentioned).
Small notice, signalfd has nothing to do with this. It is yet another method to wait for signal but it still uses signal.
However I don't think using SIGCHLD is a good idea. As a library developer I don't really want to steal the signal handler for any signals as we have no idea if there are other parts in a program which actually wait for SIGCHLD, too.
SIGCHLD is used for wait operation on the child to exit, it is its purpose. So it is better to state in the documentation that it uses SIGCHLD then to say that a) Only one io_service can do async waiting b) You can't use wait in other parts of the code. Also you don't have to steal the handler you can call it (sigaction returns previous handler).
We don't know either if the signal handler is reset after we set it.
No it is not. It is basic knowledge about POSIX/Unix programming. Also there are more ways to wait for signal, for example if you use pselect, it exits when signal is delivered (of course you need to use sigprocmask etc but it is reasonable price for this) Just study this topic there are lots of things that can be done.
It should be really the application and not external libraries which manage signals. As there can be even more problems like blocked signals using Boost.Process could affect a program in ways a developer would probably not even think about.
I think if you state this clearly in documentation it is OK as SIGCLD is "Unix" way to wait for child. Also there are some other tricks but they less portable and error prone like passing open pipe to the process and wait for it being closed (It has some other issue as what happens if the process does something like for(fd=0;fd<256;fd++) close(fd); etc)
2. The code of thread interruption under POSIX system, it is just no go.
void interrupt_work_thread() { // By creating a child process which immediately exits // we interrupt wait(). std::vector<std::string> args; args.push_back("-c"); args.push_back("'exit'"); interrupt_pid_ = create_child("/bin/sh", args).get_id(); } a) Why do you assume that "/bin/sh" exists? What I if work in chroot enviroment?
calling fork(), exit(1) would be much simpler but still I don't think you should fork at all as overall you should not use "wait()"
b) Performance, starting an additonal process to make something to stop is horrible so if I start and stop io_service frequently it would be very slow.
Yes, it's horrible. Still I was glad to interrupt the blocking call to wait() at least somehow. Again I wish there was a more suitable POSIX interface I could use to emulate an asynchronous wait operation.
POSIX API for this is: sigaction, sigwaitinfo etc.
But it would probably not make much sense to support an asynchronous wait operation for one platform only.
Best Regards, Artyom
Artyom wrote:
However I don't think using SIGCHLD is a good idea. As a library developer I don't really want to steal the signal handler for any signals as we have no idea if there are other parts in a program which actually wait for SIGCHLD, too.
SIGCHLD is used for wait operation on the child to exit, it is its purpose.
Sure, but the application may have reasons to use it, too, and may use a different signal API.
So it is better to state in the documentation that it uses SIGCHLD then to say that
a) Only one io_service can do async waiting b) You can't use wait in other parts of the code.
Also you don't have to steal the handler you can call it (sigaction returns previous handler).
Unfortunately, if the application installs a SIGCHLD handler after Boost.Process, it may not behave so well. Perhaps the better scheme is to provide a signal handler and require the application to install it, within the application's own scheme and code, so that the handler's management is more explicit. You'd need to document the need for the handler to be installed for dependent features to work, give example code for how to install it in cooperation with an application's own handler, and even provide an API that does the work for those applications that otherwise don't care about SIGCHLD.
It should be really the application and not external libraries which manage signals. As there can be even more problems like blocked signals using Boost.Process could affect a program in ways a developer would probably not even think about.
I think if you state this clearly in documentation it is OK as SIGCLD is "Unix" way to wait for child.
Documentation can certainly justify the library when the application fails to follow the prescribed form and gets unwanted behavior, but that doesn't help the wayward developer solve the application bug. It seems better for the library to not function as desired without explicit action from the application than to silently work in some scenarios and not others. That would drive the developer to the documentation to learn why the desired feature isn't working. _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
----- Original Message ----
From: "Stewart, Robert" <Robert.Stewart@sig.com> To: "boost@lists.boost.org" <boost@lists.boost.org> Sent: Thu, February 10, 2011 3:31:00 PM Subject: Re: [boost] Subject: Formal Review of Proposed Boost.Process library starts tomorrow
Artyom wrote:
However I don't think using SIGCHLD is a good idea. As a library developer I don't really want to steal the signal handler for any signals as we have no idea if there are other parts in a program which actually wait for SIGCHLD, too.
SIGCHLD is used for wait operation on the child to exit, it is its purpose.
Sure, but the application may have reasons to use it, too, and may use a different signal API.
Agree
So it is better to state in the documentation that it uses SIGCHLD then to say that
a) Only one io_service can do async waiting b) You can't use wait in other parts of the code.
Also you don't have to steal the handler you can call it (sigaction returns previous handler).
Unfortunately, if the application installs a SIGCHLD handler after Boost.Process, it may not behave so well. Perhaps the better scheme is to provide a signal handler and require the application to install it, within the application's own scheme and code. so that the handler's management is more explicit. You'd need to document the need for the handler to be installed for dependent features to work, give example code for how to install it in cooperation with an application's own handler, and even provide an API that does the work for those applications that otherwise don't care about SIGCHLD.
Yes I think this approach is good one. In any case current Boost.Process way can't remain. Other possible (but very wasteful) way to handle this if user does not want to mess with signals and don't want them to happen at all is to create a thread per asynchronous waiting that calls waitpid() and detach it immediately it may be even created with small stack size that does something like void *thread_func(void *) { ... waitpid(...) lock_some_shared_data if(io_service is still alive) io_service->notify_on_pid else pass unlock_some_shared_data } But this is actually just in case user really does not want to handle signals or don't want Boost.Process to install signals for him. Probably it would be ok to provide some parameter on how to wait asynchronously: typedef enum { wait_default, // using thread+waitpid per-process on POSIX OS wait_using_sigaction, // install signal handler wait_using_sigwaitinfo, // install signal handler wait_polling, // poll all waiting pids for end, when // requested - for example by user's signal // handler } wait_method_type; void async_wait(pid ,method_type m = wait_default) #ifdef BOOST_POSIX void poll_handlers(); // check if child completed // called by user when receives sigchld endif Artyom
Am 10.02.2011 15:03, schrieb Artyom:
Probably it would be ok to provide some parameter on how to wait asynchronously:
typedef enum { wait_default, // using thread+waitpid per-process on POSIX OS wait_using_sigaction, // install signal handler wait_using_sigwaitinfo, // install signal handler wait_polling, // poll all waiting pids for end, when // requested - for example by user's signal // handler } wait_method_type;
void async_wait(pid ,method_type m = wait_default) #ifdef BOOST_POSIX void poll_handlers(); // check if child completed // called by user when receives sigchld endif
I believe this won't work because: On POSIX the interaction of signal-handlers and threads is not defined (it is not defined which thread gets the signal delivered). The recommended solution is to block the signals for all threads and declare a special thread which blocks in sigwait() for the desired signals. Oliver
Am 10.02.2011 15:03, schrieb Artyom:
Probably it would be ok to provide some parameter on how to wait asynchronously:
typedef enum { wait_default, // using thread+waitpid per-process on POSIX OS wait_using_sigaction, // install signal handler wait_using_sigwaitinfo, // install signal handler wait_polling, // poll all waiting pids for end, when // requested - for example by user's signal // handler } wait_method_type;
void async_wait(pid ,method_type m = wait_default) #ifdef BOOST_POSIX void poll_handlers(); // check if child completed // called by user when receives sigchld endif
what about this solution (without error handling) on POSIX in one special worker-thread: sigset_t set; sigemptyset( & set), sigaddset( & set, SIGCHLD); // SIGTERM, SIGINT, SIGQUIT pthread_sigmask(SIG_BLOCK, & set, ...); int signo( 0); sigwait( set, & signo); switch ( signo) { case SIGCHLD: while (true) { int status( 0); pid_t child = waitpid( -1, % status, WUNTRACED|WCONTINUED); if ( -1 == child && ECHILD != child) throw runtime_error("waitpid() failed"); // select data/callabck associated with child // from internal storage } case: ... }; Oliver
----- Original Message ----
From: Oliver Kowalke <k-oli@gmx.de>
Am 10.02.2011 15:03, schrieb Artyom:
Probably it would be ok to provide some parameter on how to wait asynchronously:
typedef enum { wait_default, // using thread+waitpid per-process on POSIX OS wait_using_sigaction, // install signal handler wait_using_sigwaitinfo, // install signal handler wait_polling, // poll all waiting pids for end, when // requested - for example by user's signal // handler } wait_method_type;
void async_wait(pid ,method_type m = wait_default) #ifdef BOOST_POSIX void poll_handlers(); // check if child completed // called by user when receives sigchld endif
what about this solution (without error handling) on POSIX in one special worker-thread:
sigset_t set; sigemptyset( & set), sigaddset( & set, SIGCHLD); // SIGTERM, SIGINT, SIGQUIT pthread_sigmask(SIG_BLOCK, & set, ...); int signo( 0); sigwait( set, & signo); switch ( signo) { case SIGCHLD: while (true) { int status( 0); pid_t child = waitpid( -1, % status, WUNTRACED|WCONTINUED); if ( -1 == child && ECHILD != child) throw runtime_error("waitpid() failed"); // select data/callabck associated with child // from internal storage } case: ... };
Oliver
Something like this is fine but and this what I mean when I had written: wait_using_sigwaitinfo Small notes: 1. waitpid should be done for specific pid I waiting for to allow other threads to use wait 2. You need to use WNOHANG so it would not be blocking, and allow to check specific ids. So basically such thread need forever ( sigwait(sigchld) foreach pid in waiting set if(waitpid(pid, WNOHANG) = done) notify io_service remove pid from waiting set } It may be a single global thread that handles or some kind of singletone thread that exits if # io_service > 0 Of course user should still be aware of fact that it can't just do what he wants with SIGCHLD Artyom
Am 10.02.2011 17:11, schrieb Artyom:
Small notes:
1. waitpid should be done for specific pid I waiting for to allow other threads to use wait 2. You need to use WNOHANG so it would not be blocking, and allow to check specific ids.
You would create for each child process you want to wait for a separat thread? Wouldn't this degrade the performance of the system if you have to wait for many processes? Why not using only one thread which handles SIGCHLD and catches all child stati?
So basically such thread need
forever ( sigwait(sigchld) foreach pid in waiting set if(waitpid(pid, WNOHANG) = done) notify io_service remove pid from waiting set }
you iterate over the complete set of pids you are waiting for. why not let waitpid( -1,...) let tell which child has changed its status. Shouldn't the wait-functionality be interrupt-able? How to do a grace full shutdown if forever blocking? Oliver
On 02/10/2011 08:29 AM, Oliver Kowalke wrote:
Am 10.02.2011 17:11, schrieb Artyom:
Small notes:
1. waitpid should be done for specific pid I waiting for to allow other threads to use wait 2. You need to use WNOHANG so it would not be blocking, and allow to check specific ids.
You would create for each child process you want to wait for a separat thread? Wouldn't this degrade the performance of the system if you have to wait for many processes? Why not using only one thread which handles SIGCHLD and catches all child stati?
This is ultimately the best (and only reasonable) approach (where the use of a dedicated thread, as opposed to no additional threads and just letting the signal handler be called in an arbitrary thread, would be optional). However, it does place a high burden on Boost.Process: since under this method Boost.Process will need exclusive control over SIGCHLD and all waiting on child processes, it must provide appropriate interfaces to interoperate will all other application and library code that may wish to deal with child processes. I believe it should be possible to make Boost.Process itself not depend on a dedicated signal handling thread, but potentially other application/library code that wants to deal with child proceses/sigchld might need the signal handler to run in a dedicated thread. (And potentially, the application or some other library may wish to handle SIGCHLD and call wait directly, in which case there should be some interface by which this code can notify boost.process appropriately.)
So basically such thread need
forever ( sigwait(sigchld) foreach pid in waiting set if(waitpid(pid, WNOHANG) = done) notify io_service remove pid from waiting set }
you iterate over the complete set of pids you are waiting for. why not let waitpid( -1,...) let tell which child has changed its status.
Because it can accidentally catch an exit code of normal wait for example consider that other thread waits synchronously, this was the problem I've told in the original review notes the race condition between waiting thread and other thread that makes synchornous wait. This is why you can't use waitpid with -1 or wait that catches any child.
Shouldn't the wait-functionality be interrupt-able? How to do a grace full shutdown if forever blocking?
It can be done with other signal - you can use real-time signals to notify the thread on interruption. Artyom
Am 10.02.2011 17:11, schrieb Artyom:
sigset_t set; sigemptyset(& set), sigaddset(& set, SIGCHLD); // SIGTERM, SIGINT, SIGQUIT pthread_sigmask(SIG_BLOCK,& set, ...); int signo( 0); sigwait( set,& signo); switch ( signo) { case SIGCHLD: while (true) { int status( 0); pid_t child = waitpid( -1, % status, WUNTRACED|WCONTINUED); if ( -1 == child&& ECHILD != child) throw runtime_error("waitpid() failed"); // select data/callabck associated with child // from internal storage } case: ... };
correction: if ( -1 == child) { if ( ECHILD == child) { break; // return to sigwait() else { throw runtime_error("waitpid() failed"); } } // select data/callabck associated with child // from internal storage I suggest to wait for SIGUSR1 in sigwait() which indicates that the wait function should be canceld. pthread_kill() can be used to send the SIGUSR1 to the worker-thread. Oliver
On Thu, 10 Feb 2011 06:49:36 +0100, Artyom <artyomtnk@yahoo.com> wrote:
[...]I think if you can't implement this properly just don't implement. IMHO it is important to have less features that work then half working onces.
I agree (although for a different but somehow related reason; the complexity of the implementation bothers me more).
[...]
However I don't think using SIGCHLD is a good idea. As a library developer I don't really want to steal the signal handler for any signals as we have no idea if there are other parts in a program which actually wait for SIGCHLD, too.
SIGCHLD is used for wait operation on the child to exit, it is its purpose.
So it is better to state in the documentation that it uses SIGCHLD then to say that
a) Only one io_service can do async waiting
You have the same problem with a SIGCHLD-based implementation. If an io_service installs a signal handler you can't use a second io_service as it would install a new signal handler.
Also you don't have to steal the handler you can call it (sigaction returns previous handler).
We don't know either if the signal handler is reset after we set it.
No it is not. It is basic knowledge about POSIX/Unix programming.
I think we misunderstand each other here. But Robert and Jeremy have already expressed what I tried to say.
[...]I think if you state this clearly in documentation it is OK as SIGCLD is "Unix" way to wait for child.
Here's a new proposal: We remove boost::process::status. While it's tempting to use Boost.Asio I come to the conclusion that it isn't and can't be the framework of choice for all asynchronous operations. When I think about my attempts to create a Boost.Asio extension to handle signals I feel only confirmed. However my similarly complicated signal handler was based on a clean implementation from Dmitry Goncharov. He had tried to create a Boost.Asio extension which was never added to Boost.Asio though - probably because his I/O object doesn't work and look like all the others. But then why not adding it to Boost.Process? Dmitry's code can be found at https://svn.boost.org/trac/boost/ticket/2879 (it's only two header files). It needs an io_service object but that's all what it has in common with other I/O objects. His code would not only make it possible to wait for child processes but handle all signals. Then Boost.Process would give up the goal of supporting only cross-platform features. But if I had to choose between the current implementation of boost::process::status, a library with no support at all to wait for child processes and a non-asio-like class to handle signals I'd choose the latter. I'm not so sure what we should do on Windows. Maybe boost::process::status should not be dropped completely as Windows developers might want to use it. If anyone knows whether it's possible to wait for a child process on Windows using existing Boost.Asio services the implementation could be simplified a lot. Comments? Boris
[...]
Hi, I use Boost::process::status, and it's a very simple and efficient implementation for working with asynchronous operations on process. But have restriction for use, as cited. I think you should give a chance. And all implementations use ASIO, Is no longer a different implementation, more code more bug, and if some people need understand or create a new feature and already know boost.asio, is simple. I think try implements this idea is good. And Windows I don't know =). typedef enum { wait_default, // using thread+waitpid per-process on POSIX OS wait_using_sigaction, // install signal handler wait_using_sigwaitinfo, // install signal handler wait_polling, // poll all waiting pids for end, when // requested - for example by user's signal // handler } wait_method_type; Bruno
[...]I think if you can't implement this properly just don't implement. IMHO it is important to have less features that work then half working onces.
I agree (although for a different but somehow related reason; the complexity of the implementation bothers me more).
IMHO it is not as complex as it may look like, there already was bunch of possible solutions in this thread. Async wait is very valuable feature.
[...]
However I don't think using SIGCHLD is a good idea. As a library developer I don't really want to steal the signal handler for any signals as we have no idea if there are other parts in a program which actually wait for SIGCHLD, too.
SIGCHLD is used for wait operation on the child to exit, it is its purpose.
So it is better to state in the documentation that it uses SIGCHLD then to say that
a) Only one io_service can do async waiting
You have the same problem with a SIGCHLD-based implementation. If an io_service installs a signal handler you can't use a second io_service as it would install a new signal handler.
I had already suggested that you need a singletone "waiter" thread that would use sigwaitinfo that would allow to do the job for any number of io_services. Note it would even allow to execute installed signal handlers if you need them and notify io_services. asio is very well designed asynchrous event loop that IMHO should be used for any asynchronous operations as it allows to do in "async" way almost any task.
[...]I think if you state this clearly in documentation it is OK as SIGCLD is "Unix" way to wait for child.
Here's a new proposal:
We remove boost::process::status. While it's tempting to use Boost.Asio
See comment above.
I come to the conclusion that it isn't and can't be the framework of choice for all asynchronous operations. When I think about my attempts to create a Boost.Asio extension to handle signals I feel only confirmed.
It is not straight forward as signal unlike select/epoll/kqueue is process resource and naturally should be handled in single location. But it is notification only issue, Asio provides you event loop that allows dispatch various asynchronous events and it is very valuable and such notifications can be integrated to Asio.
Then Boost.Process would give up the goal of supporting only cross-platform features. But if I had to choose between the current implementation of boost::process::status, a library with no support at all to wait for child processes and a non-asio-like class to handle signals I'd choose the latter.
I don't think that the fact that it is not as straight forward on POSIX platform as on Windows platform you should give up. I come from Unix world and If I was thinking this way then 99% of my project would not support Windows :-) Artyom
On Fri, 11 Feb 2011 08:07:34 +0100, Artyom <artyomtnk@yahoo.com> wrote:
[...]asio is very well designed asynchrous event loop that IMHO should be used for any asynchronous operations as it allows to do in "async" way almost any task.
This was exactly what I always thought, too. Now I believe that I made the classic mistake of seeing everywhere nails because of the Boost.Asio hammer. As much as I like Boost.Asio I don't think anymore that we should try to integrate everything in Boost.Asio just because we managed to add the word "asynchronous" to the description of a feature. There are certain requirements, and if they are not met I think Boost.Asio is the wrong answer. Boost.Asio is based on I/O service objects which provide services to I/O objects. If we create a singleton outside of I/O service objects what's their purpose? We don't need them anymore. We could use the singleton directly. That's what Dmitry Goncharov had proposed with his signal_handler. Or let's imagine Boost.Asio wouldn't exist and we would think about a signal handler. Everyone would probably agree that we need a singleton. But would anyone argue that we also need an I/O service object and an I/O object? Given the easy to use Boost.Asio API some developers would still prefer to use it. And it could all still be built on top of such a singleton. I, for one, would always prefer direct access to the singleton though - especially if it's that easy to use as Dmitry's and Boost.Asio can't provide me anything extra. Boris
[...]
On 02/11/2011 04:34 PM, Boris Schaeling wrote:
On Fri, 11 Feb 2011 08:07:34 +0100, Artyom <artyomtnk@yahoo.com> wrote:
[...]asio is very well designed asynchrous event loop that IMHO should be used for any asynchronous operations as it allows to do in "async" way almost any task.
This was exactly what I always thought, too. Now I believe that I made the classic mistake of seeing everywhere nails because of the Boost.Asio hammer. As much as I like Boost.Asio I don't think anymore that we should try to integrate everything in Boost.Asio just because we managed to add the word "asynchronous" to the description of a feature. There are certain requirements, and if they are not met I think Boost.Asio is the wrong answer.
Boost.Asio is based on I/O service objects which provide services to I/O objects. If we create a singleton outside of I/O service objects what's their purpose? We don't need them anymore. We could use the singleton directly. That's what Dmitry Goncharov had proposed with his signal_handler.
Or let's imagine Boost.Asio wouldn't exist and we would think about a signal handler. Everyone would probably agree that we need a singleton. But would anyone argue that we also need an I/O service object and an I/O object?
Given the easy to use Boost.Asio API some developers would still prefer to use it. And it could all still be built on top of such a singleton. I, for one, would always prefer direct access to the singleton though - especially if it's that easy to use as Dmitry's and Boost.Asio can't provide me anything extra.
If a dedicated thread is used to handle SIGCHLD, then that thread could also invoke the asynchronous notifications, and Boost Asio wouldn't add anything. However, users may instead wish to not use a dedicated thread, or to receive notifications in a different thread, in which case Boost Asio would seem to have some use.
Am 10.02.2011 23:29, schrieb Boris Schaeling:
Here's a new proposal:
We remove boost::process::status. While it's tempting to use Boost.Asio I come to the conclusion that it isn't and can't be the framework of choice for all asynchronous operations. When I think about my attempts to create a Boost.Asio extension to handle signals I feel only confirmed.
However my similarly complicated signal handler was based on a clean implementation from Dmitry Goncharov. He had tried to create a Boost.Asio extension which was never added to Boost.Asio though - probably because his I/O object doesn't work and look like all the others. But then why not adding it to Boost.Process?
Dmitry's code can be found at https://svn.boost.org/trac/boost/ticket/2879 (it's only two header files). It needs an io_service object but that's all what it has in common with other I/O objects. His code would not only make it possible to wait for child processes but handle all signals.
Then Boost.Process would give up the goal of supporting only cross-platform features. But if I had to choose between the current implementation of boost::process::status, a library with no support at all to wait for child processes and a non-asio-like class to handle signals I'd choose the latter.
I'm not so sure what we should do on Windows. Maybe boost::process::status should not be dropped completely as Windows developers might want to use it. If anyone knows whether it's possible to wait for a child process on Windows using existing Boost.Asio services the implementation could be simplified a lot.
Comments?
I would vote for incorporating Dimitrys signal-handler into boost.process. At the first look I'm not sure if it will work properly in a multi-thread env. I'd like to see the possibility to send signals to the child process too. posix_child pchild( child); pchild.signal_sigstop(); pchild.signal_sigcontinue(); etc. Oliver
On 02/11/2011 03:14 AM, Oliver Kowalke wrote: [snip]
I'd like to see the possibility to send signals to the child process too.
posix_child pchild( child); pchild.signal_sigstop(); pchild.signal_sigcontinue(); etc.
Sending signals to the child process is quite trivial: simply invoke the POSIX kill function with the pid of the child. You don't even have to worry about looping to handle EINTR. Potentially boost.process could provide an interface for this, but as it is specific to POSIX, and it is basically impossible to have a simpler interface than already provided by POSIX, it seems rather pointless. In contrast, handling signals, and SIGCHLD in particular, is quite complicated, and therefore it would be quite useful for Boost.Process to provide proper handling, as already discussed.
Am 11.02.2011 21:10, schrieb Jeremy Maitin-Shepard:
On 02/11/2011 03:14 AM, Oliver Kowalke wrote: [snip]
I'd like to see the possibility to send signals to the child process too.
posix_child pchild( child); pchild.signal_sigstop(); pchild.signal_sigcontinue(); etc.
Sending signals to the child process is quite trivial: simply invoke the POSIX kill function with the pid of the child. You don't even have to worry about looping to handle EINTR. Potentially boost.process could provide an interface for this, but as it is specific to POSIX, and it is basically impossible to have a simpler interface than already provided by POSIX, it seems rather pointless.
The interface is simple but it is not pointless if boost.process implements the boost.asio signal handler. Sending signals makes the interface of boost.process more complete. Especially if Boris adopts its last suggestion regarding to Dmitry's code (https://svn.boost.org/trac/boost/ticket/2879). If boost.process handles arbitrary signals delivered to the process why not provide an interface which allows to send those signals (even if the implementation is trivial)?
On 10/02/11 00:05, Boris Schaeling wrote:
On Wed, 09 Feb 2011 12:00:12 +0100, Artyom <artyomtnk@yahoo.com> wrote:
[...] Deadlock Scenario: ------------------ 1. I start two asio::io_service instances A, B 2. I create child a and wait for it in A service. 3. I create child b and wait for it in B service. 4. io_service A gets status of children B. 5. service B never terminates as wait would not exit.
Race Condition Scenario: ------------------------
1. I start process A and start waiting for its termination Asynchronously. 2. I start process B and it terminates 3. wait() receives it status and pid is deleted 4. I wait for B Synchronously. 5. I got a error as PID does not exits.
It is badly designed and has major bugs
The only bug I see is then in the documentation where it should be stated clearly that this is not and was never intended to be supported (as it's indeed impossible with the current implementation).
I don't pretend to be an expert, but I thought that one could safely get around these sorts of problems with wait by using waitpid instead, rather than having to use signal handlers. Does that solution apply in this case? John Bytheway
On 02/10/2011 01:36 AM, John Bytheway wrote:
I don't pretend to be an expert, but I thought that one could safely get around these sorts of problems with wait by using waitpid instead, rather than having to use signal handlers. Does that solution apply in this case?
No, there are still two problems: it is necessary then for boost.process to have one thread waiting per child process, which is rather wasteful, and furthermore the form of wait that waits for any (rather than a specific) child pid cannot be used at all within the program by other code, as otherwise that other wait call might accidentally get notified even for a boost.process pid. Thus, any code that interoperates with Boost.process must use the same wasteful method of waiting.
This is my first attempt at doing a review. Please let me know if I did something wrong or was incorrect in my observations. I had limited time to look at the precise details of everything I mentioned and may be wrong. ------------------------------ * What is your evaluation of the design? Overall the design is quite simple which is a good thing. I feel usage can be a bit verbose at times but some wrappers for common scenarios may help this a lot though I am not sure if extra helper functions is out of scope for this library and we just want to provide the primitives that other code can be built on. 1) Requiring #ifdef's to handle the standard case of the exit status is nasty. I dont want to HAVE to use #ifdef statements for the most common usage. A better abstraction should be made as has been mentioned in previous reviews. 2) There should probably be one extra common behaviour for file redirection of std in,out,err. I.e. We have null, pipe, inherit. I think that file is a very important one that should be available in the base library 3) There is code to send common termination signals to child processes, I also think a simple abstraction could be provided for child processes to use to register handlers for the normal termination events (On POSIX Handling sigterm at leastfor example). 4) There should probably be one or more higher level abstractions that allow common use cases. I wont go into examples here as it may be out of scope for the library. ------------------------------ * What is your evaluation of the implementation? I am not an expert on this, but I think there are possibly a few issues with the POSIX implementation. Please review these and let me know if I am wrong. 5) With the fork/exec implementation in create_child() it is not possible to determine the difference between errors generated by a exec'ed child process from errors that occur between the fork and the exec in the child. For example the if (chdir(work_dir) == -1) { write(STDERR_FILENO, "chdir() failed\n", 15); _exit(127); } should behave like other errors where they throw using: BOOST_PROCESS_THROW_LAST_SYSTEM_ERROR("fork(2) failed"); I think this is possible by creating an anonymous pipe before the fork that is used to communicate errors from the child to the parent that may occur before the exec has been called. The FD of this pipe in the child should be marked to close on exec and thus will close on successful exec and if exec fails can be used to send that error back to the parent as well. The parent should then read these errors from the pipe and throw an exception. Otherwise you rely on writing to stderr in the child (bad, what if i dont want these errors printed to stderr) and exiting with a status and this looks like the exec'ed process failed, not the process of creating an execed child failed. It does mean that the parent will need to synchronise with the exec of the client possibly waiting for a short period of time (waiting for the error pipe to close before moving on). 6) I didnt look very closely and do testing under linux, but it looks like in windows we hold a handle reference to the process to prevent it being cleaned up so we can get the exit status of the process. This is a ref counted thing. On POSIX, should we also have special cleanup to prevent zombie processes? I.e. Is it valid on windows to create a child and not wait for it, but on POSIX won't that just create zombies? Having behaviour that is valid on one system but not on another should probably be avoided if possible. ------------------------------ * What is your evaluation of the documentation? I liked the User Guide, the reference docs seems a bit sparse in information when i went looking for details. 7) The reference docs seem very sparse in information and formatted porely 8) I assume that context::env is populated on construction from the current process env vars? I cant find this mentioned in the docs. 9) I didnt see in the docs any mention of what is done with the process_name by default. I assume it is set to the exe name. This would also mean that argv[0] cant be an empty string (No one is likely to want this anyway). 10) I didnt get a good feel for what the error handling strategies are for this library from the docs. What errors can be expected where? Not just process creation, but what about the stream usage etc? I noticed docs for some things mentioned throws system errors, but for example reference docs for create_child() do not say anything. Any possible errors from the construction of the context::env etc? What about exception safety guarantees (Not sure if these are required for boost library submissions)? 11) child::terminate() has some docs that say: After this call, accessing this object can be dangerous because the process identifier may have been reused by a different process. It might still be valid, though, if the process has refused to die. I.e. Calling sigterm or sigkill, you mean the pid is no longer valid to wait on to get the exit status? In what cases can this happen? I didnt think this was the case. 12) I didnt see much in the way of documentation on what happens with open file descriptors on exec of the process. It seems that on POSIX, the library will close all the open file descriptors except the ones in the handles vector. It should be mentioned somewhere in the docs along with info on how to prevent file descriptors from being closed if so desired. ------------------------------ * What is your evaluation of the potential usefulness of the library? I think this library is a necessary part of boost. It requires a few changes though before acceptance IMO as outlined in the summary of this review. ------------------------------ * Did you try to use the library? * With what compiler? * Did you have any problems? I built and ran some of the examples under Windows 7 using MSVC 2010. No issues. I have not had time to try and write something with it yet. Hopefully I will get that chance soon. ------------------------------ * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I read through all the docs, had a glance at the examples and parts of the implementation that took my fancy. I have not looked much at the streams and async IO code as I am not yet very familiar with ASIO. ------------------------------ * Are you knowledgeable about the problem domain? I think so. I have written plenty of applications that require process creation in C and C++ ------------------------------ * Do you think the library should be accepted as a Boost library? Yes, on the condition that points 1, 5, 6 and 11 have been reviewed and possibly acted on. I feel the other points are *less* important. Thanks, Brendon. On 7 February 2011 17:18, Marshall Clow <mclow.lists@gmail.com> wrote:
It's my pleasure to announce that the review of Boris Schäling's Process library starts tomorrow, February 7th and lasts until February 20, 2011, unless an extension occurs.
What is it? ===========
Boost.Process is a library to manage system processes. It can be used to:
• create child processes • run shell commands • setup environment variables for child processes • setup standard streams for child processes (and other streams on POSIX platforms) • communicate with child processes through standard streams (synchronously or asynchronously) • wait for processes to exit (synchronously or asynchronously) • terminate processes
Getting the library ===================
The library is available at: http://www.highscore.de/boost/gsoc2010/process.zip with documentation at: http://www.highscore.de/boost/gsoc2010/
There was a quite involved "informal review" on the mailing list this past summer. That thread is archived here: http://thread.gmane.org/gmane.comp.lib.boost.devel/207594
Writing a review ================
If you feel this is an interesting library, then please submit your review to the developer list (preferably), or to the review manager (me!)
Here are some questions you might want to answer in your review:
- What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the library? - Did you try to use the library? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick - reading? In-depth study? - Are you knowledgeable about the problem domain?
And finally, every review should answer this question:
- Do you think the library should be accepted as a Boost library?
Be sure to say this explicitly so that your other comments don't obscure your overall opinion.
-- Marshall Review Manager for the proposed Boost.Process library
_______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost-announce
On Wed, 16 Feb 2011 11:28:41 +0100, Brendon Costa <brendon.j.costa@gmail.com> wrote: Hi Brendon, thanks for your review! I'll quickly go through it.
[...]1) Requiring #ifdef's to handle the standard case of the exit status is nasty. I dont want to HAVE to use #ifdef statements for the most common usage. A better abstraction should be made as has been mentioned in previous reviews.
I think that's fine - as long as we can agree on what the most common usage is. :) The question about the exit code and the POSIX macros is: Can Boost.Process do anything by default most POSIX developers would be happy with if WIFEXITED returns false?
[...]3) There is code to send common termination signals to child processes, I also think a simple abstraction could be provided for child processes to use to register handlers for the normal termination events (On POSIX Handling sigterm at leastfor example).
I think a signal handler would be a nice contribution. I'll send another email at the end of the review though to explain the rationale and what conclusions this review helped me to draw.
[...]I think this is possible by creating an anonymous pipe before the fork that is used to communicate errors from the child to the parent that may occur before the exec has been called. The FD of this pipe in the child should be marked to close on exec and thus will close on successful exec and if exec fails can be used to send that error back to the parent as well. The parent should then read these errors from the pipe and throw an exception. Otherwise you rely on writing to stderr in the child (bad, what if i dont want these errors printed to stderr) and exiting with a status and this looks like the exec'ed process failed, not the process of creating an execed child failed. It does mean that the parent will need to synchronise with the exec of the client possibly waiting for a short period of time (waiting for the error pipe to close before moving on).
Thanks for this new idea! That's interesting as it would allow the parent process to detect if something goes wrong (and what). Noted!
6) I didnt look very closely and do testing under linux, but it looks like in windows we hold a handle reference to the process to prevent it being cleaned up so we can get the exit status of the process. This
Yes, exactly.
[...]10) I didnt get a good feel for what the error handling strategies are for this library from the docs. What errors can be expected where? Not
Good point, also noted. Thanks, Boris
[...]
participants (17)
-
Artyom -
Boris Schaeling -
Brendon Costa -
bruno romano -
Christopher Jefferson -
Denis Shevchenko -
Emil Dotchevski -
Ilya Sokolov -
Jeremy Maitin-Shepard -
John Bytheway -
Marshall Clow -
Mathias Gaunard -
Oliver Kowalke -
Rob Riggs -
Steven Watanabe -
Stewart, Robert -
Vicente Botet