Re: [boost] [Boost-users] Subject: Formal Review of Proposed Boost.Process library starts tomorrow

Subject: Re: [boost] [Boost-users] Subject: Formal Review of Proposed Boost.Process library starts tomorrow
From: Boris Schaeling (boris_at_[hidden])
Date: 2011-02-09 19:46:31

On Mon, 07 Feb 2011 22:06:09 +0100, Steven Watanabe <watanabesj_at_[hidden]>  
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