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

Subject: Re: [boost] [Boost-announce] [Boost-users] Subject: Formal Review of Proposed Boost.Process library starts tomorrow
From: Brendon Costa (brendon.j.costa_at_[hidden])
Date: 2011-02-16 05:28:41

This is my first attempt at doing a review. Please let me know if I
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.lists_at_[hidden]> 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-users_at_[hidden]
> http://lists.boost.org/mailman/listinfo.cgi/boost-users
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost-announce
>