On Wed, 09 Feb 2011 12:00:12 +0100, Artyom <artyomtnk@yahoo.com> 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
[...]