[boost] Subject: Formal Review of Proposed Boost.Process library starts tomorrow
Marshall Clow
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
_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Oliver Kowalke
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
Steven Watanabe
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
Steven Watanabe
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.
Vicente Botet
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-tt3225411.html
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-Process-library-starts-tomorrow-tp3263607p3264839.html
Sent from the Boost - Dev mailing list archive at Nabble.com.
Oliver Kowalke
POSIX platform:
bool is_posix_platform();
if ( boost::process::is_posix_platform() ) {
...
}
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).
posix_status ps( status);
if ( ps.terminated() ) int sig = ps.termination_signal();
if ( ps.has_coredump() ) ...
Sending signals (SIGSTOP, SIGCONT, SIGKILL, etc.) to the process would
also be usefull.
You could raise an assertion (BOOST_ASSERT) if this functionality will
be invoked by a non-POSIX platform.
Mathias Gaunard
> * 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.
Christopher Jefferson
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
> Boost-users mailing list
> Boost...@lists.boost.org
> http://lists.boost.org/mailman/listinfo.cgi/boost-users
Steven Watanabe
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.
Vicente Botet
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-Process-library-starts-tomorrow-tp3263607p3265304.html
Sent from the Boost - Dev mailing list archive at Nabble.com.
Steven Watanabe
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
Vicente Botet
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-Process-library-starts-tomorrow-tp3263607p3265350.html
Sent from the Boost - Dev mailing list archive at Nabble.com.
Steven Watanabe
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
Vicente Botet
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-Process-library-starts-tomorrow-tp3263607p3270826.html
Sent from the Boost - Dev mailing list archive at Nabble.com.
Ilya Sokolov
<vicent...@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.
Ilya Sokolov
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]
Vicente Botet
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-Process-library-starts-tomorrow-tp3263607p3273421.html
Sent from the Boost - Dev mailing list archive at Nabble.com.
Steven Watanabe
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
Ilya Sokolov
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]
Denis Shevchenko
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.
Ilya Sokolov
<vicent...@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-tt3225411.html
>
> [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
<for.dsh...@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
Denis Shevchenko
> On Tue, 08 Feb 2011 12:50:59 +0500, Denis Shevchenko
> <for.dsh...@gmail.com> wrote:
>
>> Of course, with boost::assign ;-)
>
> -1. No need to add a dependency in such simple case
>
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
Ilya Sokolov
<for.dsh...@gmail.com> wrote:
> 08.02.2011 12:37, Ilya Sokolov пишет:
>> On Tue, 08 Feb 2011 12:50:59 +0500, Denis Shevchenko
>> <for.dsh...@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. ...'
Denis Shevchenko
>
> bad_alloc
>
Hm.. Yes, you are right, hypothetically it's possible...
- Denis
Vicente Botet
Ilya Sokolov-2 wrote:
>
> On Tue, 08 Feb 2011 00:09:25 +0500, Vicente Botet
> <vicent...@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-tt3225411.html
>>
>> [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-Process-library-starts-tomorrow-tp3263607p3275577.html
Mateusz Loskot
> As Marshall announced that the formal review of my library has
> started I'd like to provide some information which is maybe helpful
> for those interested in submitting a review. [...]
This may be off-topic in this thread, but I think it's a good occasion
to touch this subject.
Why the review is discussed on the boost-users mailing list?
The review manager is supposed to announce review on boost and
boost-announce lists *only*. The results however go also to boost-users.
I don't understand why the review is being conducted on boost-users.
Even worse, it's cross-posted!
Signal to noise ratio on the Boost mailing list has been worse than ever
recently and if we start cross-posting reviews, it's going to become a
chaos impossible to monitor and participate in. I am not trying to
mother to anyone. I simply regret there is lack of self-discipline in
the Community regarding the ml participation.
Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Charter Member of OSGeo, http://osgeo.org
Member of ACCU, http://accu.org
Ilya Sokolov
<vicent...@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-tt3225411.html
>>>
>>> [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?
Vicente Botet
Ilya Sokolov-2 wrote:
>
> On Tue, 08 Feb 2011 14:22:08 +0500, Vicente Botet
> <vicent...@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-tt3225411.html
>>>>
>>>> [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-Process-library-starts-tomorrow-tp3263607p3275728.html
Sent from the Boost - Dev mailing list archive at Nabble.com.
Emil Dotchevski
> On Tue, 08 Feb 2011 02:06:09 +0500, Steven Watanabe <watan...@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
Boris Schaeling
<vicent...@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/example/chroot_setup.cpp
(for POSIX) and at
https://svn.boost.org/svn/boost/sandbox/SOC/2010/process/libs/process/example/startupinfo_setup.cpp
(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
Vicente Botet
Boris Schaeling wrote:
>
> On Mon, 07 Feb 2011 23:26:57 +0100, Vicente Botet
> <vicent...@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/example/chroot_setup.cpp
> (for POSIX) and at
> https://svn.boost.org/svn/boost/sandbox/SOC/2010/process/libs/process/example/startupinfo_setup.cpp
> (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.
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-Process-library-starts-tomorrow-tp3263607p3283561.html
Sent from the Boost - Dev mailing list archive at Nabble.com.
Boris Schaeling
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
Ilya Sokolov
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]
Ilya Sokolov
> On Mon, 07 Feb 2011 23:26:57 +0100, Vicente Botet
>
>> [...]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]
Ilya Sokolov
> 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]
Vicente Botet
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-Process-library-starts-tomorrow-tp3263607p3296707.html
Sent from the Boost - Dev mailing list archive at Nabble.com.
Artyom
> 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/
bruno romano
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.
Artyom
>
> 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
Boris Schaeling
<for.dsh...@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
Boris Schaeling
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
> [...]
Boris Schaeling
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
Artyom
> > ------------------
> > 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
John Bytheway
> On Wed, 09 Feb 2011 12:00:12 +0100, Artyom <arty...@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
Stewart, Robert
>
> > 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....@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.
Artyom
----- Original Message ----
> From: "Stewart, Robert" <Robert....@sig.com>
> To: "bo...@lists.boost.org" <bo...@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
Oliver Kowalke
> 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
Oliver Kowalke
> 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
Artyom
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
Oliver Kowalke
> 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
Oliver Kowalke
>> 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);
>> 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
Jeremy Maitin-Shepard
> 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.
Jeremy Maitin-Shepard
> 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.)
Artyom
> >
> > 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
Boris Schaeling
> [...]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
> [...]
bruno romano
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
Artyom
> > [...]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
Oliver Kowalke
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
Jeremy Maitin-Shepard
[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.
Oliver Kowalke
> 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)?
Boris Schaeling
> [...]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
> [...]
Darren Garvey
> On Fri, 11 Feb 2011 16:39:35 +0100, Jeff Flinn <
> TriumphS...@hotmail.com> wrote:
>
> [...]I think with the right abstractions the seemingly contradictory goals
>> can be accommodated. That's what I'm attempting to find. As stated at a
>> previous boostcon: "... abstractions are discovered not invented..."
>>
>>
>> IMHO, previous version of process have not discovered the proper
>> abstractions. I'm sure that the entirely different paradigms between windows
>> and posix have contributed to the difficulties so far. It's difficult to be
>> a master of the subject matter in both domains.
>>
>> I think building upon existing boost filesystem and iostreams libraries
>> and their abstractions that do successfully address platform differences
>> will make the effort more successful. Additionally I think it's mandatory
>> for a design to directly acknowledge the multi-phase constraints of the
>> fork/exec paradigm. Also it's mandatory to break down the historical
>> monolithic approaches into more tenable pieces.
>>
>
> Thanks for your comments, Jeff, Ilya and Nat! How would you (and others)
> feel about a Boost.Process library which only aims to replace std::system()
> with a more powerful function? You could configure the child process a bit
> (its streams, environment variables and a few more things) and that's all.
> This would be a less ambitious goal but obviously more easily to reach. I
> was under the impression that we want more? Maybe we still want more but
> settle realistically for this goal?
I've not caught up on all of the discussion about the pending review yet,
but it seems to be hitting a few familiar issues this time around,
unfortunately.
I'd like Boost.Process to either mimic the Python subprocess module (as Nat
mentioned) or to provide a few low-level pieces:
1. std::system replacement allowing me to do the call in an async way and
capture the output. Ideally implemented in such a way that a "process
pipeline" library could be implemented on top of Boost.Process. Pipelining
input / output between different work items is a more general domain than
fits in Boost.Process, IMHO.
2. Way to get process id in a platform independent way.
3. Way to look up location of processes and get a Boost.Filesystem path.
4. Version of "std::system" that works with Boost.Filesystem
5. Version of std::system that allows passing a custom set of environment
variables
6. Expose a "native_handle_type" from the library, whatever that native
handle type is.
There seems to be a lot of justified disagreement about what the high-level
view of the library should be, yet all but point 1 are almost trivially
implemented in a cross-platform way and entirely orthogonal to the rest of
any Boost.Process library. I'd like to see just points 2-6 addressed,
reviewed and released before tackling the bigger picture.
The disagreements over scope are a showstopper for any progress and threaten
to lead Boost.Process astray for another another couple of years unless they
are worked around, ie. by reducing scope, perhaps to the point that
Boost.Process starts life as a basic utility library.
Evolutionary rewrites and breaking changes are much better than nothing.
After all, we're up to V2.x of Boost.Spirit and V3 of Boost.Filesystem and
all versions have been immensely useful for us end users.
Kind Regards,
Darren
Jeremy Maitin-Shepard
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.
Oliver Kowalke
I agree - boost.process should provide only the sync. wait (async. wait
should be left).
Maybe boost.process or another library can provide the async. wait
facility and the community has more time to discuss it.
Jeremy Maitin-Shepard
>[snip]
> I agree - boost.process should provide only the sync. wait (async. wait
> should be left).
> Maybe boost.process or another library can provide the async. wait
> facility and the community has more time to discuss it.
With only synchronous waiting, the library is fairly limited in utility.
I would think that the sort of scripting tasks that synchronous
waiting might be useful for would not likely be done in C++ to begin with.
Oliver Kowalke
> On 02/11/2011 11:33 PM, Oliver Kowalke wrote:
>> [snip]
>> I agree - boost.process should provide only the sync. wait (async. wait
>> should be left).
>> Maybe boost.process or another library can provide the async. wait
>> facility and the community has more time to discuss it.
>
> With only synchronous waiting, the library is fairly limited in utility.
> I would think that the sort of scripting tasks that synchronous waiting
> might be useful for would not likely be done in C++ to begin with.
you could implement async. wait with thread and future + sync. waiting
(I know this solution has its limitations).
the async. waiting shouldn't a show stopper for boost.process - as this
thread shows more discussion is required for a careful design of async.
waiting (at least on POSIX -> interaction with other signals).
It can be added in the prospective development of boost.process.
Klaim - Joël Lamotte
> Am 12.02.2011 19:51, schrieb Jeremy Maitin-Shepard:
>
> On 02/11/2011 11:33 PM, Oliver Kowalke wrote:
>>
>>> [snip]
>>> I agree - boost.process should provide only the sync. wait (async. wait
>>> should be left).
>>> Maybe boost.process or another library can provide the async. wait
>>> facility and the community has more time to discuss it.
>>>
>>
>> With only synchronous waiting, the library is fairly limited in utility.
>> I would think that the sort of scripting tasks that synchronous waiting
>> might be useful for would not likely be done in C++ to begin with.
>>
>
> you could implement async. wait with thread and future + sync. waiting (I
> know this solution has its limitations).
> the async. waiting shouldn't a show stopper for boost.process - as this
> thread shows more discussion is required for a careful design of async.
> waiting (at least on POSIX -> interaction with other signals).
> It can be added in the prospective development of boost.process.
>
Hi,
I'm observing the developpement of Boost.Process for a project for wich I
(will later) need a main application to launch children processes,
communicate with them AND know when they end, whatever the reason
(sucessful, not successful and any kind of crash).
As far as I know, from a previous discussion on this list, async_wait might
be the only (cross-platform) way (without having to add another dependency
than boost to the project...) to achive the last point, knowing when the
child process ends.
About the "thread and future + sync. waiting" way of doing it : would it
work with any kind of child process end (including any kind of crash)? Or
is it part of the limitations you're thinking about?
About the initial question, I've always thought that Boost.Process would
provide :
1. A cross-platform way to manipulate processes (child processes) including
managing their "life-time".
2. Maybe some platform-specific utilites.
3. Easy Inter-process communication "would be cool" but as there is another
library providing this feature, is not a show stopper for me.
That don't include "efficiency" as personnally I wouldnt' assume that an
abstraction of such manipulations would have a chance to be as fast as
platform-specific API usage.
Whatever the implementation (through platform-specific defines or
plateform-specific cpp files), as far as I don't see platfrom specific-code
in my own app and the abstract behaviour is the same on all platforms (if
details changes I don't really care), that's fine.
My other solution --if boost.process isn't stable enough for my need at the
moment I'll use it directly in my app-- being to just isolate myself
plateform-specific API calls, but I'm a bit worried about the child ending
notification feature (because I'm not a specialist at all).
That's the advice of an humble non-boost-dev-level boost-user, so it might
be naive.
Joël Lamotte
Oliver Kowalke
> As far as I know, from a previous discussion on this list, async_wait might
> be the only (cross-platform) way (without having to add another dependency
> than boost to the project...) to achive the last point, knowing when the
> child process ends.
Do you mean with async_wait the feature dealing with SIGCHLD and
sigwait()? If so it is not cross-platform. As discussed in this thread
waiting on signals asynchronously influences the application and other
libs - see installing signal handlers.
IMHO waiting on signals (SIGCHLD, SIGTERM, SIGSTOP, SIGUSR1, ...) should
be implemented in a separate library.
> About the "thread and future + sync. waiting" way of doing it : would it
> work with any kind of child process end (including any kind of crash)? Or
> is it part of the limitations you're thinking about?
Yes - waitpid( child_pid, ...) blocking in one separate thread -
returning the result in a future. Depending on how many threads are
created blocking in waitpid it may reduce the performance (at least at
some amount of threads the scheduling overhead may become significant).
> About the initial question, I've always thought that Boost.Process would
> provide :
> 1. A cross-platform way to manipulate processes (child processes) including
> managing their "life-time".
'Managing life-time' means stop, continue, kill children?
> 2. Maybe some platform-specific utilites.
> 3. Easy Inter-process communication "would be cool" but as there is another
> library providing this feature, is not a show stopper for me.
boost.asio/boost.interprocess
Klaim - Joël Lamotte
>
> About the initial question, I've always thought that Boost.Process would
>> provide :
>> 1. A cross-platform way to manipulate processes (child processes)
>> including
>> managing their "life-time".
>>
>
> 'Managing life-time' means stop, continue, kill children?
>
>
Yes.
>
> 2. Maybe some platform-specific utilites.
>> 3. Easy Inter-process communication "would be cool" but as there is
>> another
>> library providing this feature, is not a show stopper for me.
>>
>
> boost.asio/boost.interprocess
Yes, in fact I'll use another high-level network library (for unrelated
reasons) in addition to interprocess (because I tried with an old version of
boost.process and already have the communication code, but boost.process was
buggy few years ago).
Oliver Kowalke
>> About the initial question, I've always thought that Boost.Process would
>>> provide :
>>> 1. A cross-platform way to manipulate processes (child processes)
>>> including
>>> managing their "life-time".
>>>
>>
>> 'Managing life-time' means stop, continue, kill children?
>>
>>
> Yes.
This was what I was requesting too but I got no answer from Boris.
At least on POSIX we need a framework which deals with sending signals
and async. handling of delivered signals (not only SIGCHLD).
Because we have no conclusion how this should be handled by
boost.process in this review - I suggest that this facility should be
implemented by another library.
boost.process could provide code for waiting synchronously on a child
process (waitpid) - for this purpose the sync. wait could be done in a
separate thread and the result will be transfered via a future == async.
waiting on child process.
Jeremy Maitin-Shepard
> Am 14.02.2011 16:45, schrieb Klaim - Joël Lamotte:
>> As far as I know, from a previous discussion on this list, async_wait
>> might
>> be the only (cross-platform) way (without having to add another
>> dependency
>> than boost to the project...) to achive the last point, knowing when the
>> child process ends.
>
> Do you mean with async_wait the feature dealing with SIGCHLD and
> sigwait()? If so it is not cross-platform. As discussed in this thread
> waiting on signals asynchronously influences the application and other
> libs - see installing signal handlers.
> IMHO waiting on signals (SIGCHLD, SIGTERM, SIGSTOP, SIGUSR1, ...) should
> be implemented in a separate library.
I do not think that a general signal handling library, while potentially
useful, is required for Boost.Process to do asynchronous waiting. The
reason is that simply letting two independent pieces of code be notified
of SIGCHLD is not sufficient, because (in order to be efficient) the
SIGCHLD handler needs to then invoke waitpid(-1, ...), which affects the
global state of the program. Instead, what is needed is basically a
SIGCHLD library, which allows registering a callback to be invoked when
a given PID changes state (exited, suspended, or continued). There
should be an option to invoke the callback directly in the signal
handler, and there should also be a way (possibly built on top of the
first invocation method) to invoke a callback asynchronously using ASIO.
Ideally, Boost.Process could use this SIGCHLD library by default, but
also be able to work with an alternate SIGCHLD multiplexing library.
>
>> About the "thread and future + sync. waiting" way of doing it : would it
>> work with any kind of child process end (including any kind of crash)? Or
>> is it part of the limitations you're thinking about?
>
> Yes - waitpid( child_pid, ...) blocking in one separate thread -
> returning the result in a future. Depending on how many threads are
> created blocking in waitpid it may reduce the performance (at least at
> some amount of threads the scheduling overhead may become significant).
In addition to the overhead of one thread per process (which is rather
substantial overhead), there is the problem that all other code running
in the same process is forced to use this same inefficient waiting
mechanism, as opposed to a multiplexing interface as described above.
For instance, glib has a similar SIGCHLD handling facility, and while I
believe it would be possible to have Boost.Process make use of it, the
inefficient waiting scheme would not inter-operate with the process
launching facility in glib.
Jeff Flinn
> Am 14.02.2011 19:19, schrieb Klaim - Joël Lamotte:
>>> About the initial question, I've always thought that Boost.Process
>>> would
>>>> provide :
>>>> 1. A cross-platform way to manipulate processes (child processes)
>>>> including
>>>> managing their "life-time".
>>>>
>>>
>>> 'Managing life-time' means stop, continue, kill children?
>>>
>>>
>> Yes.
>
> This was what I was requesting too but I got no answer from Boris.
> At least on POSIX we need a framework which deals with sending signals
> and async. handling of delivered signals (not only SIGCHLD).
>
> Because we have no conclusion how this should be handled by
> boost.process in this review - I suggest that this facility should be
> implemented by another library.
+1
> boost.process could provide code for waiting synchronously on a child
> process (waitpid) - for this purpose the sync. wait could be done in a
> separate thread and the result will be transfered via a future == async.
> waiting on child process.
+1
Jeff
Oliver Kowalke
> I do not think that a general signal handling library, while potentially
> useful, is required for Boost.Process to do asynchronous waiting. The
> reason is that simply letting two independent pieces of code be notified
> of SIGCHLD is not sufficient, because (in order to be efficient) the
> SIGCHLD handler needs to then invoke waitpid(-1, ...), which affects the
> global state of the program. Instead, what is needed is basically a
> SIGCHLD library, which allows registering a callback to be invoked when
> a given PID changes state (exited, suspended, or continued). There
> should be an option to invoke the callback directly in the signal
> handler, and there should also be a way (possibly built on top of the
> first invocation method) to invoke a callback asynchronously using ASIO.
> Ideally, Boost.Process could use this SIGCHLD library by default, but
> also be able to work with an alternate SIGCHLD multiplexing library.
This would require that the thread executing async. waiting of
boost.process has to block all signals except SIGCHLD and the other
thread handling the other signals must block SIGCHLD and unblock
signals of interest (SIGTERM, SIGINT, ...). Other worker-threads have to
block all signals.
Wouldn't be the async. waiting of boost.process be a subset of the
signalling library?
Jeremy Maitin-Shepard
I don't think a dedicated thread is necessarily needed at all for
SIGCHLD. Instead, at any point in the program that needs to fork(), use
this sequence:
block SIGCHLD in current thread
wait until a lock is obtained on a signal-safe mutex
(the SIGCHLD handler will also wait to lock the safe mutex; this cannot
deadlock, since the mutex is only locked by threads that have blocked
SIGCHLD)
fork()
add child pid to data structure containing the notification list for the
SIGCHLD handler
unlock mutex
reset SIGCHLD blocked status
SIGCHLD handler will repeatedly invoke waitpid(-1, &status, WNOHANG |
WUNTRACED | WCONTINUED), and then invoke any registered handlers. These
handlers need to be designed to be able to run from a signal handler in
an arbitrary thread. If a handler needs to run outside of the signal
handler context, something like ASIO can be notified to invoke the
handler later.
Oliver Kowalke
> SIGCHLD handler will repeatedly invoke waitpid(-1, &status, WNOHANG |
> WUNTRACED | WCONTINUED), and then invoke any registered handlers.
why polling - it waists CPU cycles?
I expect that the thread waiting for child processes state change blocks
(gets suspended) until it is notified by the kernel that something has
happened.
other worker-thread do other stuff like communicating with other process
etc.
Jeremy Maitin-Shepard
> Am 14.02.2011 21:14, schrieb Jeremy Maitin-Shepard:
>> SIGCHLD handler will repeatedly invoke waitpid(-1, &status, WNOHANG |
>> WUNTRACED | WCONTINUED), and then invoke any registered handlers.
>
> why polling - it waists CPU cycles?
> I expect that the thread waiting for child processes state change blocks
> (gets suspended) until it is notified by the kernel that something has
> happened.
> other worker-thread do other stuff like communicating with other process
> etc.
It wouldn't poll indefinitely, as that would lock up the thread in which
the signal handler was invoked. It is simply invoked repeatedly until
the error ECHILD (meaning no more unwaited-for children) is returned.
The notification is in the form of SIGCHLD, but that just means one or
more children need to be waited on.
Jeremy Maitin-Shepard
> On 02/14/2011 12:50 PM, Oliver Kowalke wrote:
>> Am 14.02.2011 21:14, schrieb Jeremy Maitin-Shepard:
>>> SIGCHLD handler will repeatedly invoke waitpid(-1, &status, WNOHANG |
>>> WUNTRACED | WCONTINUED), and then invoke any registered handlers.
>>
>> why polling - it waists CPU cycles?
>> I expect that the thread waiting for child processes state change blocks
>> (gets suspended) until it is notified by the kernel that something has
>> happened.
>> other worker-thread do other stuff like communicating with other process
>> etc.
>
> It wouldn't poll indefinitely, as that would lock up the thread in which
> the signal handler was invoked. It is simply invoked repeatedly until
> the error ECHILD (meaning no more unwaited-for children) is returned.
> The notification is in the form of SIGCHLD, but that just means one or
> more children need to be waited on.
Sorry, it is invoked repeatedly until waitpid returns 0.
Oliver Kowalke
> On 02/14/2011 01:00 PM, Jeremy Maitin-Shepard wrote:
>> On 02/14/2011 12:50 PM, Oliver Kowalke wrote:
>>> Am 14.02.2011 21:14, schrieb Jeremy Maitin-Shepard:
>>>> SIGCHLD handler will repeatedly invoke waitpid(-1, &status, WNOHANG |
>>>> WUNTRACED | WCONTINUED), and then invoke any registered handlers.
>>>
>>> why polling - it waists CPU cycles?
>>> I expect that the thread waiting for child processes state change blocks
>>> (gets suspended) until it is notified by the kernel that something has
>>> happened.
>>> other worker-thread do other stuff like communicating with other process
>>> etc.
>>
>> It wouldn't poll indefinitely, as that would lock up the thread in which
>> the signal handler was invoked. It is simply invoked repeatedly until
>> the error ECHILD (meaning no more unwaited-for children) is returned.
>> The notification is in the form of SIGCHLD, but that just means one or
>> more children need to be waited on.
maybe you misunderstood my concerns.
as I wrote in my previous posts - we have two possibilities in a
multi-threaded env.:
1.) for each child process a thread is created calling waitpid() with
the pid of the forked process only and returning the child state via a
future
2.) only one thread is created dealing with the status of child
processes - it calls sigwait() with SIGCHLD unblocked and waitpid(-1, &
status, WNOHANG | WUNTRACED | WCONTINUED) until waitpid() returns ECHILD
I've some concerns if boost.process would implement the second point
because it is a subset of the functionality of a boost library dealing
with delivered signals (as SIGUSR1, etc.).
Jeremy Maitin-Shepard
There is a third possibility, which is what I was referring to in my
previous message: no additional threads are created to handle SIGCHLD
and instead SIGCHLD is handled in any thread that does not block it (the
user is expected to leave it unblocked in at least one thread).
This would have the lowest overhead, I believe, and therefore be the
best option to use, and it also would not require any coordination as
far as signal handling with the rest of the program (but of course would
require coordination as far as waitpid), except that it will manage
SIGCHLD (but monopolizing SIGCHLD and calling waitpid go hand in hand).
Oliver Kowalke
>> maybe you misunderstood my concerns.
>> as I wrote in my previous posts - we have two possibilities in a
>> multi-threaded env.:
>>
>> 1.) for each child process a thread is created calling waitpid() with
>> the pid of the forked process only and returning the child state via a
>> future
>>
>> 2.) only one thread is created dealing with the status of child
>> processes - it calls sigwait() with SIGCHLD unblocked and waitpid(-1, &
>> status, WNOHANG | WUNTRACED | WCONTINUED) until waitpid() returns ECHILD
>>
>> I've some concerns if boost.process would implement the second point
>> because it is a subset of the functionality of a boost library dealing
>> with delivered signals (as SIGUSR1, etc.).
>
> There is a third possibility, which is what I was referring to in my
> previous message: no additional threads are created to handle SIGCHLD
> and instead SIGCHLD is handled in any thread that does not block it (the
> user is expected to leave it unblocked in at least one thread).
>
> This would have the lowest overhead, I believe, and therefore be the
> best option to use, and it also would not require any coordination as
> far as signal handling with the rest of the program (but of course would
> require coordination as far as waitpid), except that it will manage
> SIGCHLD (but monopolizing SIGCHLD and calling waitpid go hand in hand).
The SIGCHLD signal would be delivered to only one of the threads and you
don't know to which one. you would have to install in all threads the
same code.
Jeremy Maitin-Shepard
Yes, it would be delivered to an arbitrary thread, but as signal handler
settings are shared between threads, it would only be necessary to call
signal/sigaction once.
Oliver Kowalke
> Yes, it would be delivered to an arbitrary thread, but as signal handler
> settings are shared between threads, it would only be necessary to call
> signal/sigaction once.
Why do I need multiple threads doing the same? One thread handling
signals is sufficient. Delivering the signals ansyc. with sigaction()
etc. is dangerous in a multi-threaded app because if the code locks a
mutex and then handles a signal delivered asynchronously with let the
mutex be locked during the signal handling and may let the mutex be
locked forever if the thread exits.
This is not what you want at least how long the mutex remains locked
should not depend on asnyc. signal handlers.
Jeremy Maitin-Shepard
> Am 15.02.2011 02:44, schrieb Jeremy Maitin-Shepard:
>> Yes, it would be delivered to an arbitrary thread, but as signal handler
>> settings are shared between threads, it would only be necessary to call
>> signal/sigaction once.
>
> Why do I need multiple threads doing the same? One thread handling
> signals is sufficient. Delivering the signals ansyc. with sigaction()
> etc. is dangerous in a multi-threaded app because if the code locks a
> mutex and then handles a signal delivered asynchronously with let the
> mutex be locked during the signal handling and may let the mutex be
> locked forever if the thread exits.
Naturally the signal handler shouldn't terminate the thread.
An important case is when there is just a single thread in total, in
which case using a dedicated thread for signal handling would be excessive.
> This is not what you want at least how long the mutex remains locked
> should not depend on asnyc. signal handlers.
Arguably then the signal handler could record that the signal was
delivered rather than actually call waitpid. In any case, waitpid won't
block, so I expect the signal handler would take on the order of a
millisecond (this would really have to be tested, though, to know).
Also, the user could still provide a dedicated thread by blocking
SIGCHLD in every thread but one, without any special support in
boost.process.
Oliver Kowalke
> Arguably then the signal handler could record that the signal was
> delivered rather than actually call waitpid. In any case, waitpid won't
> block, so I expect the signal handler would take on the order of a
> millisecond (this would really have to be tested, though, to know).
>
> Also, the user could still provide a dedicated thread by blocking
> SIGCHLD in every thread but one, without any special support in
> boost.process.
If you call waitpid(-1,...) in aloop inside a signal handler, then you get the pids of the children. What do you do next? Keep in mind that the functions you can call must be obstruction-free/async-safe.
How do you tell the other threads your pid you got from waitpid()?
--
GMX DSL Doppel-Flat ab 19,99 Euro/mtl.! Jetzt mit
gratis Handy-Flat! http://portal.gmx.net/de/go/dsl
Jeremy Maitin-Shepard
>
>> Arguably then the signal handler could record that the signal was
>> delivered rather than actually call waitpid. In any case, waitpid won't
>> block, so I expect the signal handler would take on the order of a
>> millisecond (this would really have to be tested, though, to know).
>>
>> Also, the user could still provide a dedicated thread by blocking
>> SIGCHLD in every thread but one, without any special support in
>> boost.process.
>
> If you call waitpid(-1,...) in aloop inside a signal handler, then you get the pids of the children. What do you do next? Keep in mind that the functions you can call must be obstruction-free/async-safe.
> How do you tell the other threads your pid you got from waitpid()?
The status for each child could be written to a designated pre-allocated
data structure, and the thread could be notified by writing a single
byte in non-blocking mode to a designated pipe. If the write fails
because the pipe buffer is full, it isn't a problem, because it means
the reader end has yet to receive some previous notifications and will
therefore still see the new status.
Oliver Kowalke
Seams not reasonable for me - too much overhead.
The only possible way would be using a atomic variable which will be set
by the signals handler that a SIGCHLD was delivered and the other
threads running through a event-loop will detect the change of the
atomic variable and then one thread would loop over waitpid( -1,...).
Jeremy Maitin-Shepard
The only way to wake up the event loop, as far as I know, is to change
the status of a file descriptor. (The signal itself might cause a
wake-up, but I don't think there is any race-free way to rely on that.)
I suppose your claim is that if all the signal handler is doing is
simply notifying asio that a signal arrived, then it might as well be
done using a generic signal handling library. That is valid, but it is
still an implementation detail, and therefore isn't really a concern.
Doing it in the signal handler has the advantage of keeping the SIGCHLD
handling relatively independent from asio. For instance, it would then
be possible to support a combination of synchronous and asynchronous
waiting, and also perhaps allow interoperability with other libraries
that want to manage processes. It would require testing to know, but
I'm not convinced that doing this work in the signal handler would add
significant latency, e.g. more than would be introduced by random kernel
interrupt handlers, and if latency is very important in certain threads,
SIGCHLD (and all other signals) could simply be blocked in those
threads. The documentation could warn about this.
Brendon Costa
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...@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...@lists.boost.org
> http://lists.boost.org/mailman/listinfo.cgi/boost-users
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost-announce
Boris Schaeling
<brendon...@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
> [...]
Rob Riggs
> 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