----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17306/#review32707 -----------------------------------------------------------
This looks good Ben! One high-level thought, I wonder if we want to change any of the semantics around discard == killtree on process. Could someone use subprocess to "fire and forget"? Or when the Subprocess instance goes out of scope will all the pipes get closed and even the only future reference for status get cleaned up thus discarding it? 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61647> Why this naming scheme? 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61648> Can we s/cleanup/internal/ like other files please? 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61649> Not sure what you mean here. Please elaborate comment. 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61650> Seems like ready, failed, and discard could really be condensed to an onAny handler that looks conditionally at the future. 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61651> Print a newline too? 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61652> Can you include a comment that we don't explicitly discard the future we're waiting on from the reaper because we assume the reaper will reap the subprocess and the handlers will get invoked cleaning things up? 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61654> s/references/reference/? ;) 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment61653> I'm not sure if this is sufficient. If someone does a 'then' on the status future and the Subprocess instance gets destructed and the stdout/stderr pipes get closed but the subprocess still tries to write to stdout/stderr, what happens? Does the subprocess get a SIGPIPE? That would be bad. If that's the case we only want to close the pipes after the reaper has reaped the process AND there are no more references to them. 3rdparty/libprocess/src/tests/subprocess_tests.cpp <https://reviews.apache.org/r/17306/#comment61656> s/executor/subprocess/g - Benjamin Hindman 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 > >
