Re: [boost] [boost.process] 0.6 Redesign

Subject: Re: [boost] [boost.process] 0.6 Redesign
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2016-04-18 17:49:44

On 18 Apr 2016 at 17:35, Klemens Morgenstern wrote:

> At the current state, the tests pass on linux as well as windows (gcc-5 
> & MSVC-14). Requirements are C++, boost.fusion, boost.asio, 
> boost.iostreams, boost.filesystem and boost.system.
> 
> I really could use some feedback and hope you're interested.

Firstly, well done for working on this. Last contract I got annoyed 
with Boost.Process and ended up replacing it with a hacked shoe in 
based on ASIO. More recently, I needed a child process spawner for my 
ACCU presentation I'm giving this week and ended up again reinventing 
Boost.Process, only this edition is a "bare metal" reinvention based 
on Outcomes [1]. So I'm all for this in principle.

However I'm not sure if I'm for your specific formulation. Here are 
my top issues:

1. You should rip out all usage of Boost.Iostreams. It's been without 
maintainer for years now, and has a fundamentally broken design for 
async i/o. Nobody should be encouraged to use it in new code.

2. You should completely eliminate all synchronous i/o as that is 
also fundamentally broken for speaking to child processes. Everything 
needs to be async 100% of the time, it's the only sane design choice 
[2]. You can absolutely present publicly a device which appears to 
quack and waddle like a synchronous pipe for compatibility purposes, 
indeed AFIO v2 presents asynchronous i/o devices as synchronous ones 
if you use the synchronous APIs even though underneath the i/o 
service's run() loop gets pumped during i/o blocks. But underneath it 
needs to be 100% async, and therefore probably ASIO.

3. Instead of inventing your own i/o objects, I think you need to 
provide:

(a) Async and sync objects extending ASIO's base objects with 
child_stdin, child_stdout and child_stderr - or whatever your 
preferred naming.

(b) std::istream and std::ostream wrappers. These are not hard, ASIO 
helps you a lot with these once you have the native ASIO objects.

4. Child processes are not like threads and should not be represented 
as a first order object. They should instead be an opaque object 
represented by an abstract base class publicly and managed by a RAII 
managing class from whom the opaque object can be detached, assigned, 
transferred etc.

5. Replace all the on_exit() machinery with future continuations i.e. 
launching a process *always* returns a future. If someone wants to 
hook code onto when the process exits, a future continuation is the 
right tool. Similarly for fetching return codes, or detaching oneself 
from the child. Python's new subprocess.run() returns exactly the 
struct your future also needs to return.

6. Looking through your source code, I see references to 
boost::fusion and lots of other stuff. Great, but most people wanting 
a Process management library don't want to drag in a copy of Boost to 
get one. It's easier to just roll your own. So drop the Boost 
dependency.

7. Looking through your source code, I am struck about how much 
functionality is done elsewhere by other libraries, especially ASIO. 
I think less is more for Boost.Process, I always personally greatly 
preferred the pre-peer-review Boost.Process even with its warts over 
the post-peer-review one which had become too "flowery" and "ornate" 
if that makes sense. The latter became unintuitive to program 
against, I kept having to look up the documentation and that annoys 
me. This stuff should be blindingly obvious to use. It should "just 
work".

I conclude my mini-review by suggesting "less is more" for 
Boost.Process. 99% of users want the absolute *minimum* featureset. 
Look at Python 3.5's new subprocess module, that is a very good API 
design and featureset to follow. It's intuitive, it gets the job done 
quickly, but it exposes enough depth if you really need it to write a 
really custom solution. I'd *strongly* recommend you copy that API 
design for Boost.Python and dispense with the current API design 
entirely. The absolute clincher in Python's subprocess is you can 
never, ever race nor deadlock stdout and stderr. That makes an 
underlying async i/o implementation unavoidable. I'd personally 
suggest save yourself a ton of hassle and use ASIO's pipe/unix socket 
support facilities, it's becoming the Networking TS anyway.

Hope this is helpful.

Niall

[1]: 
https://github.com/ned14/boost.afio/blob/master/include/boost/afio/v2/
detail/child_process.hpp

[2]: I refer to the stdout/stderr deadlock problem which is the 
biggest reason anyone reaches for a process management library 
instead of just using the 
syscalls directy. The internals of the child i/o needs to be 100% 
async to 
prevent deadlocking. You can absolutely present publicly a device 
which appears 
to quack and waddle like a synchronous pipe for compatibility 
purposes, indeed AFIO v2 presents asynchronous i/o devices as 
synchronous ones if you use the synchronous APIs even though 
underneath the i/o service's run() loop gets pumped during i/o 
blocks.

-- 
ned Productions Limited Consulting
http://www.nedproductions.biz/ 
http://ie.linkedin.com/in/nialldouglas/