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

Subject: Re: [boost] Subject: Formal Review of Proposed Boost.Process library starts tomorrow
From: Artyom (artyomtnk_at_[hidden])
Date: 2011-02-09 06:00:12

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

Hello All,

---------------------------------------------------------------------
- What is your evaluation of the potential usefulness of the  library?
---------------------------------------------------------------------

It is potentially very useful library that its kind should
exist in boost.

---------------------------------------------------------------------
  - Did you try to use the library? With what compiler? Did you  have
   any problems?
---------------------------------------------------------------------

I've tried to run several examples using Linux x86_64 gcc-4.3 
and cross compile several examples for windows uning gcc-4.2

I had minor compilation problems with MinGW.

---------------------------------------------------------------------
 - How much effort did you put into your evaluation? A  glance? A quick
 - reading? In-depth study?
---------------------------------------------------------------------

I've spend about 3 hours mostly reviewing the code looking
at system calls and how various operations are implemented.

I was looking to use it for managing processes like staring
compiler and integration it to asio's event loop.

---------------------------------------------------------------------
 - Are you knowledgeable about  the problem domain?
---------------------------------------------------------------------

I had deal with many tools/issues implemented in this library:

- Process creation, termination, asynchronous waiting
- Working with pipes
- Working with custom streambuffers etc.


---------------------------------------------------------------------
 - What is your evaluation of the  documentation?
---------------------------------------------------------------------

I have following issues with documentation

Documentation lacks of following points:

1. Rationale Part:

   a) Why the library was designed this way, for example:
      why boost::function is used in context::streams
      very unclear
   b) What tools are used for implementation so the
      user would understand how the underlying tools work.

2. Tutorial should be improved a little to provide
   better explanation of what exactly happens,
   
   but overall it is fine.

3. It should be marked id documentation explicitly that
   -D_WIN32_WINNT=0x501 or soemthing like that should be given

   I had major failures to compile Boost.Process under MinGW platform.

   GetProcessId and UuidCreateSequential  was not found.

   Until I figured this missing define.


---------------------------------------------------------------------
  - What is your evaluation of the design?
---------------------------------------------------------------------

I would mostly relate to API's design, I'll talk about 
implementation separately.


- process object should be polymorphic, as self and child
  are derived and there is a good chance for using code like

  auto_ptr<process> p(new self());

  Which may cause bad things.

- The use of self::instance() is unclear:

  1. It is not thread safe
  2. It seems to not being destroyed
  3. When should it be used over just creating self() ?

- Boost.Process uses sometimes type names ended with "_t",
  such suffixes are reserved by POSIX in global namespace
  and generally should be avoided

  It is better to use _type as STL does.


- Overall the API seems to be clean but context
  class requires much better explanations
  of its rationale.



---------------------------------------------------------------------
  - What is your  evaluation of the implementation?
---------------------------------------------------------------------

This is the major problem with the library, I had
found too many faults in the implementation in too
short time that finally caused me to abort more
in deep review.


Minor Issues:
-------------

1. Don't include <boost/asio.hpp> try to include only
   abolutly nessary headers.

   Asio is painfully long to compile so try
   to avoid its full inclusion


Medium Level Issues:
--------------------

1. systembuf

   It does not handle EINTR error which
   something that can happen. read and write
   operations should be restarted in this event
      
   I had already mentioned this be previous
   mini-review, I hoped it would be fixed for
   formal review.

2. Windows ANSI API/Wide API should issues should be
    addressed in one of following ways:

  - provide wide alternative API (not so good idea)

  - use std::locale::facet for converting paths, 
    command parameters to wide api using either:

    a) Global codecvt facet to user can install 
       UTF-8 facet in global locale

       simillary to what boost::filesystem::v3 does.

    b) Provide encoding option for narrow strings
    c) Use UTF-8 - but this is out of scope of this project.

   However it should be handled.

3. Use of getcwd.

   In the code you try to discover the side of the
   path using pathconf. Unfortunatelly it
   may return values that actually can-not be allocated
   and be virtually unlimited. So this approach
   is not correct.

   You need to use something like this:

     1. Allocate a initial buffer size (lets say 256)
     2. call getcwd
     3. if ok return  the path
     4. if errno==ERANGE
     5.     resize buffer to twice its current size
     6.     go to step 2
     7. else
     8.     fail


Major Issues - Showstoppers
---------------------------

1. Asynchronous wait design under POSIX system
   has major problems that can cause system to
   deadlock.

   It uses wrong system API and its implementation is
   wrong.

   How it is designed:

   1. Create a thread for asio service
   2. Call pid = wait(&status) <-- Problem is here
   3. Notify the event loop on childs that finished.

   Deadlock Scenario:
   ------------------
   
   1. I start two asio::io_service instances A, B
   2. I create child a and wait for it in A service.
   3. I create child b and wait for it in B service.
   4. io_service A gets status of children B.
   5. service B never terminates as wait would not exit.

   Race Condition Scenario:
   ------------------------


   1. I start process A and start waiting for its termination Asynchronously.
   2. I start process B and it terminates
   3. wait() receives it status and pid is deleted
   4. I wait for B Synchronously.
   5. I got a error as PID does not exits.


   It is badly designed and has major bugs

   It is likely should be implemented by using sigaction on SIGCHLD
   and allow to actually implement it without additional thread.

2. The code of thread interruption under POSIX system, it
   is just no go.

    void interrupt_work_thread()
    {
        // By creating a child process which immediately exits
        // we interrupt wait().
        std::vector<std::string> args;
        args.push_back("-c");
        args.push_back("'exit'");
        interrupt_pid_ = create_child("/bin/sh", args).get_id();
    }
   
    a) Why do you assume that "/bin/sh" exists? What I if work
       in chroot enviroment?

       calling fork(), exit(1) would be much simpler but still I don't
       think you should fork at all as overall you should not 
       use "wait()"

    b) Performance, starting an additonal process to make something
       to stop is horrible so if I start and stop io_service frequently
       it would be very slow.


After discovering the issues above I had aborted the review 
process as " bug to review time ratio" was too high.

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


Unfortunately, No, it shouldn't.

1. I don't think that in current state it can be included in Boost.

2. I do think that after fixing major and medium level issues it
   should be resubmitted for next review.

3. I would rather prefer this library to have less features
   that can be implemented in future but it should be 100% clear
   and done right.

4. I like overall design but it has too many critical flaws.

5. Boost needs such library so I do encourage the author to
   improve it and resubmit for the review.




Best Regards,
  Artyom Beilis


 
____________________________________________________________________________________
Looking for earth-friendly autos? 
Browse Top Cars by "Green Rating" at Yahoo! Autos' Green Center.
http://autos.yahoo.com/green_center/