Boost logo

Boost :

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/


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk