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