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: Boris Schaeling (boris_at_[hidden])
Date: 2011-02-09 19:05:13

On Wed, 09 Feb 2011 12:00:12 +0100, Artyom <artyomtnk_at_[hidden]> wrote:

Hi Artyom,

> [...]I had minor compilation problems with MinGW.

I'm glad to hear that the library could be used on MinGW. There were some  
problems reported before but I never tested it myself on that platform.

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

Yes, it looks like Boost.Process should emit the same warning as  
Boost.Asio if _WIN32_WINNT isn't defined.

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

It's noted but no one (including me) wrote the code yet. :)

> 2. Windows ANSI API/Wide API should issues should be
>     addressed in one of following ways:
> [...]   However it should be handled.

While this is indeed something where I would also prefer a more flexible  
interface I'm not sure if this problem should be tackled by Boost.Process.  
On the one hand I'm waiting for something like Boost.Locale. On the other  
hand I know that Boost.Filesystem v3 provides that flexibility. But what's  
the recommendation for all the other Boost libraries?

> 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

Thanks, I'll look into it.

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

The only bug I see is then in the documentation where it should be stated  
clearly that this is not and was never intended to be supported (as it's  
indeed impossible with the current implementation).

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

I wish adding support for asynchronous operations would be much more  
lightweight. What we currently have is a first implementation which works  
(under certain restrictions like the one you described above). It's  
already bad enough that the implementation is so different for Windows and  
POSIX. I guess there can be even more implementations as some platforms  
support some system calls we could use (like signalfd Bruno mentioned).  
However I don't think using SIGCHLD is a good idea. As a library developer  
I don't really want to steal the signal handler for any signals as we have  
no idea if there are other parts in a program which actually wait for  
SIGCHLD, too. We don't know either if the signal handler is reset after we  
set it. It should be really the application and not external libraries  
which manage signals. As there can be even more problems like blocked  
signals using Boost.Process could affect a program in ways a developer  
would probably not even think about.

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

Yes, it's horrible. Still I was glad to interrupt the blocking call to  
wait() at least somehow. Again I wish there was a more suitable POSIX  
interface I could use to emulate an asynchronous wait operation. But the  
system calls are as they are. If there are no other ideas and most of you  
feel that such a horrible implementation should indeed not be shipped with  
a Boost library, support for the asynchronous wait operation can always be  
removed. The whole implementation is bit better on Windows. But it would  
probably not make much sense to support an asynchronous wait operation for  
one platform only.

Thanks,
Boris

> [...]