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:
> [...]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
> 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;
> 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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk