----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17306/#review33111 -----------------------------------------------------------
Ship it! Great stuff Ben, great tests! 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment62387> s/Subprocess/subprocess/ Also, maybe you want to be even more explicit with your wording: The input / output file descriptors are only closed after both (1) the subprocess has terminated and (2) there are no longer any references to the associated Subprocess object. 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment62389> It is a little ironic that you went with in(), out(), err() as functions but still called the fields _stdin, _stdout, and _stderr instead of just in, out, and err. ;) 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment62399> And also mention that discarding this future has no action? I.e., to kill a subprocess you must explicitly kill the pid. 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment62398> Is this because you assume process::reap will never discard a process? If that's your assumption, we should really add that as a comment in process/reap.hpp as part of the semantics. 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment62391> This comment is helpful, but could be less esoteric for someone else with a few more words. ;) 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment62390> Why not return Error? 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment62392> If you do turn this into an Error, consider what the ~Subprocess destructor will do here. 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment62394> ... safe manner (but assuming 'strlen' is async-signal safe too). 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment62396> Awesome freaking comment! 3rdparty/libprocess/include/process/subprocess.hpp <https://reviews.apache.org/r/17306/#comment62397> We need to bind a copy of this Subprocess into the onAny callback below to ensure that we don't close the file descriptors before the subprocess has terminated (i.e., because the caller doesn't keep a copy of this Subprocess around themselves). 3rdparty/libprocess/src/tests/subprocess_tests.cpp <https://reviews.apache.org/r/17306/#comment62400> Can this comment be killed with https://reviews.apache.org/r/17480? - Benjamin Hindman On Jan. 29, 2014, 3:39 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17306/ > ----------------------------------------------------------- > > (Updated Jan. 29, 2014, 3:39 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 > >
