----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17306/#review32740 -----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61701> What's the reason not having a copy constructor? 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61694> Not sure if we need a reaper for each subprocess call? Can we create one reaper for all subprocess calls? I guess you don't want to create a reaper if no subprocess call is made, and you want to cleanup the reaper if all Subprocesses are terminated. But given that you need to handle reaper cleanup logic in any way (_cleanup), why not do it at global level (i.e., lazy initialization, reference counting). 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61705> In what situation this future will get discarded? 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61699> Instead of killing the subprocess when 'status' is discarded, what about having an explicit kill(), because a user who doesn't care about exit status does not mean that he want the subprocess to be killed. 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61704> Ditto BenH's comments. Probably we want to close all these pipes after the subprocess exits. 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61696> Instead of doing that (copying explicitly), what about doing this: struct Subprocess { struct Data { pid_t pid; int stdin; int stdout; int stderr; ... }; memory::shared_ptr<Data> data; } so that you just need to do: data = that.data; - Jie Yu On Jan. 24, 2014, 7:06 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17306/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2014, 7:06 a.m.) > > > Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu. > > > Bugs: MESOS-943 > https://issues.apache.org/jira/browse/MESOS-943 > > > Repository: mesos-git > > > Description > ------- > > This adds an asynchronous mechanism for subprocess execution, per MESOS-943. > > What started simple was made a little more complex due to the following > issues: > > 1. Who is responsible for closing the input / output descriptors? > > Placing this burden onto the caller of subprocess() seems likely to yield > leaked open file descriptors. This introduced the notion of a shared_ptr / > destructor / copy constructor / assignment constructor to ensure that the > file descriptors are closed when the handle to the file descriptors are lost. > However, even with my implementation, one may copy these file descriptors, at > which point they may be deleted from underneath them. > > 2. What does discarding the status entail? Does it kill the process? > > The current implementation kills the process, which requires the use of an > explicit Promise to deal with the discard from the caller not affecting the > reaper's future. If discard() is a no-op, we must still use an explicit > Promise to preserve the notification from the Reaper (so that we can know > when to delete the Reaper). > > > That's about it, I've added tests that demonstrate the ability to communicate > with the subprocess through stdin / stout / stderr. > > Please let me know if you find any simplifications that can be made! (Other > than C++11 lambdas, of course :)) > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459 > 3rdparty/libprocess/include/process/subprocess.hpp PRE-CREATION > 3rdparty/libprocess/src/tests/subprocess_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/17306/diff/ > > > Testing > ------- > > Tests were added and ran in repetition. > > > Thanks, > > Ben Mahler > >
