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-10 00:49:36

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

I think if you can't implement this properly just don't implement. IMHO
it is important to have less features that work then half working
onces.

> >    It is likely should be implemented by  using sigaction on SIGCHLD
> >    and allow to actually implement  it without additional thread.
> 
> [snip]
>
> 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).

Small notice, signalfd has nothing to do with this. It is yet another method
to wait for signal but it still uses signal.

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

SIGCHLD is used for wait operation on the child to exit, it is its
purpose.

So it is better to state in the documentation that it uses SIGCHLD
then to say that 

a) Only one io_service can do async waiting
b) You can't use wait in other parts of the code.

Also you don't have to steal the handler you can call it (sigaction returns
previous handler).

> We don't know either if the signal handler is reset after we set  it.

No it is not. It is basic knowledge about POSIX/Unix programming.

Also there are more ways to wait for signal, for example if you use
pselect, it exits when signal is delivered (of course you need to use
sigprocmask etc but it is reasonable price for this)

Just study this topic there are lots of things that can be done.

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

I think if you state this clearly in documentation it is OK as
SIGCLD is "Unix" way to wait for child.

Also there are some other tricks but they less portable and error prone
like passing open pipe to the process and wait for it being closed

(It has some other issue as what happens if the process does something
 like for(fd=0;fd<256;fd++) close(fd); etc)

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

POSIX API for this is: sigaction, sigwaitinfo etc.

> But it would probably not make  much sense to support 
> an asynchronous wait operation for one platform  only.
> 

Best Regards,

   Artyom