Boost logo

Boost :

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]>

> [...]The PP guard on the constructor that takes
> a handle, should be
> 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,

Boost list run by bdawes at, gregod at, cpdaniel at, john at