> On Jan. 29, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 196
> > <https://reviews.apache.org/r/17306/diff/4/?file=453982#file453982line196>
> >
> >     Being explicit is fine but I believe at this point that the default 
> > assignment operator is sufficient.

Ah thanks, same for the copy constructor. :)


> On Jan. 29, 2014, 11:39 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 124
> > <https://reviews.apache.org/r/17306/diff/4/?file=453982#file453982line124>
> >
> >     Isn't process->data->in,out,err set to random values at this point? Or 
> > worse, they could be set to 0 so we close stdin of the current process! I 
> > think you want to move the lines from 166-168 up above the fork!

Gah, should not have made that last minute change! Thanks for catching this.


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33183
-----------------------------------------------------------


On Jan. 29, 2014, 10:39 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17306/
> -----------------------------------------------------------
> 
> (Updated Jan. 29, 2014, 10:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ian Downes, Jie Yu, and Vinod 
> Kone.
> 
> 
> 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
> 
>

Reply via email to