You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2014/01/24 08:06:01 UTC
Review Request 17306: Added an asynchronous subprocess utility.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/
-----------------------------------------------------------
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
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 27, 2014, 7:53 a.m., Ian Downes wrote:
> > What about supporting environment variables specific to the child process? This is necessary for distinct environments between different subprocesses and the parent. This could be done by prepending the command with 'env' but it'll be much nicer to take a map<string, string> of environment variables and setenv them after the fork.
> >
> > Along the same lines, what about optionally taking a user and working directory?
>
> Nikita Vetoshkin wrote:
> Sorry for intrusion with comments.
> What about more generic way - a way to pass a callable, that will be invoked after fork. Usually it is called pre_exec_fn.
>
> Ian Downes wrote:
> Yes, this is a more general solution. However, the callable *must* be async-signal-safe so perhaps we don't want to fully expose this.
I could imagine taking environment variables and arguments explicitly, let's take the lazy approach and get this simple version committed to get things started. We'll add functionality as the need arises. :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review32817
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Nikita Vetoshkin <ni...@gmail.com>.
> On Jan. 27, 2014, 7:53 a.m., Ian Downes wrote:
> > What about supporting environment variables specific to the child process? This is necessary for distinct environments between different subprocesses and the parent. This could be done by prepending the command with 'env' but it'll be much nicer to take a map<string, string> of environment variables and setenv them after the fork.
> >
> > Along the same lines, what about optionally taking a user and working directory?
Sorry for intrusion with comments.
What about more generic way - a way to pass a callable, that will be invoked after fork. Usually it is called pre_exec_fn.
- Nikita
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review32817
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 27, 2014, 7:53 a.m., Ian Downes wrote:
> > What about supporting environment variables specific to the child process? This is necessary for distinct environments between different subprocesses and the parent. This could be done by prepending the command with 'env' but it'll be much nicer to take a map<string, string> of environment variables and setenv them after the fork.
> >
> > Along the same lines, what about optionally taking a user and working directory?
>
> Nikita Vetoshkin wrote:
> Sorry for intrusion with comments.
> What about more generic way - a way to pass a callable, that will be invoked after fork. Usually it is called pre_exec_fn.
Yes, this is a more general solution. However, the callable *must* be async-signal-safe so perhaps we don't want to fully expose this.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review32817
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review32817
-----------------------------------------------------------
What about supporting environment variables specific to the child process? This is necessary for distinct environments between different subprocesses and the parent. This could be done by prepending the command with 'env' but it'll be much nicer to take a map<string, string> of environment variables and setenv them after the fork.
Along the same lines, what about optionally taking a user and working directory?
- Ian Downes
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 24, 2014, 8:24 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 50
> > <https://reviews.apache.org/r/17306/diff/1/?file=447773#file447773line50>
> >
> > Can we s/cleanup/internal/ like other files please?
> On Jan. 24, 2014, 8:24 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 34-36
> > <https://reviews.apache.org/r/17306/diff/1/?file=447773#file447773line34>
> >
> > Why this naming scheme?
This was because stdin / stdout / stderr are macros on some systems (specifically, on my OS X laptop).
I've updated this to expose the file descriptors and exit status future via methods, which will hopefully reduce the inclination for caller's to copy the file descriptors without keeping a handle to the Subprocess.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review32707
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 24, 2014, 10:39 p.m., Ian Downes wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 133
> > <https://reviews.apache.org/r/17306/diff/1/?file=447773#file447773line133>
> >
> > I don't think strlen() is async-signal-safe.
I'd like to use it for convenience, my rationale is three-fold:
1. I don't think there are any known implementations of strlen that are not async signal safe, given the functionality of strlen is to count the length of a string argument, I cannot think of a sane implementation that would depend on global state.
2. There have been requests to add it to the list of known async signal safe functions, given 1 above.
http://austingroupbugs.net/view.php?id=692
3. It is highly likely that both clang and gcc will optimize out the need for strlen in these cases since the result can be determined at compile-time.
What do you think? I could be sold either way, but I'd like to keep the code clean and it seems a safe assumption to make.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review32758
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review32758
-----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/17306/#comment61726>
I don't think strlen() is async-signal-safe.
- Ian Downes
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 29, 2014, 5:57 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 184
> > <https://reviews.apache.org/r/17306/diff/3/?file=453457#file453457line184>
> >
> > What about moving the these 'close' calls to the destructor of Data. In that case, you just need to do it once and does not need to check 'data.unique()'.
Sounds good, this lets me kill the Subprocess destructor as well, thanks Jie!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33127
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33127
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/17306/#comment62408>
What about moving the these 'close' calls to the destructor of Data. In that case, you just need to do it once and does not need to check 'data.unique()'.
- Jie Yu
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33183
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/17306/#comment62503>
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!
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/17306/#comment62504>
Being explicit is fine but I believe at this point that the default assignment operator is sufficient.
- Benjamin Hindman
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Feb. 3, 2014, 9:10 a.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 157-159
> > <https://reviews.apache.org/r/17306/diff/5/?file=454081#file454081line157>
> >
> > Hey Ben,
> >
> > These lines fail to compile on GCC 4.8.1:
> > error: ignoring return value of ‘ssize_t write(int, const void*, size_t)’, declared with attribute warn_unused_result [-Werror=unused-result]
>
> Ian Downes wrote:
> This is the same thing I missed when using write. I just explicitly ignored the return value with (void) write(...). Is that an acceptable solution?
Unfortunately not; still breaks.
Think you need to read it:
ssize_t written = ...
(void)written;
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33437
-----------------------------------------------------------
On Jan. 29, 2014, 4:28 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, 4:28 p.m.)
>
>
> Review request for mesos, Benjamin Hindman 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 3, 2014, 5:10 p.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 157-159
> > <https://reviews.apache.org/r/17306/diff/5/?file=454081#file454081line157>
> >
> > Hey Ben,
> >
> > These lines fail to compile on GCC 4.8.1:
> > error: ignoring return value of ‘ssize_t write(int, const void*, size_t)’, declared with attribute warn_unused_result [-Werror=unused-result]
>
> Ian Downes wrote:
> This is the same thing I missed when using write. I just explicitly ignored the return value with (void) write(...). Is that an acceptable solution?
>
> Niklas Nielsen wrote:
> Unfortunately not; still breaks.
>
> Think you need to read it:
>
> ssize_t written = ...
> (void)written;
>
> Ben Mahler wrote:
> Interesting, I don't have this issue with gcc-4.8.2 on OS X 10.8. Perhaps write is only declared with the warn_unused_result attribute on OS X 10.9?
>
> I will likely _exit(1) if write returns -1, as opposed to ignoring the return value, if that sounds good to you guys.
>
> Niklas Nielsen wrote:
> I ran into the problem on "gcc version 4.8.1 (Ubuntu/Linaro 4.8.1-10ubuntu9)"
What about this?
diff --git a/3rdparty/libprocess/include/process/subprocess.hpp b/3rdparty/libprocess/include/process/subprocess.hpp
index db9c96b..452eeea 100644
--- a/3rdparty/libprocess/include/process/subprocess.hpp
+++ b/3rdparty/libprocess/include/process/subprocess.hpp
@@ -154,9 +154,12 @@ inline Try<Subprocess> subprocess(const std::string& command)
// implemented in an unsafe manner:
// http://austingroupbugs.net/view.php?id=692
const char* message = "Failed to execl '/bin sh -c ";
- write(STDERR_FILENO, message, strlen(message));
- write(STDERR_FILENO, command.c_str(), command.size());
- write(STDERR_FILENO, "'\n", strlen("'\n"));
+ while (write(STDERR_FILENO, message, strlen(message)) == -1 &&
+ errno == EINTR);
+ while (write(STDERR_FILENO, command.c_str(), command.size()) == -1 &&
+ errno == EINTR);
+ while (write(STDERR_FILENO, "'\n", strlen("'\n")) == -1 &&
+ errno == EINTR);
_exit(1);
}
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33437
-----------------------------------------------------------
On Jan. 30, 2014, 12:28 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17306/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2014, 12:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 3, 2014, 5:10 p.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 157-159
> > <https://reviews.apache.org/r/17306/diff/5/?file=454081#file454081line157>
> >
> > Hey Ben,
> >
> > These lines fail to compile on GCC 4.8.1:
> > error: ignoring return value of ‘ssize_t write(int, const void*, size_t)’, declared with attribute warn_unused_result [-Werror=unused-result]
>
> Ian Downes wrote:
> This is the same thing I missed when using write. I just explicitly ignored the return value with (void) write(...). Is that an acceptable solution?
>
> Niklas Nielsen wrote:
> Unfortunately not; still breaks.
>
> Think you need to read it:
>
> ssize_t written = ...
> (void)written;
Interesting, I don't have this issue with gcc-4.8.2 on OS X 10.8. Perhaps write is only declared with the warn_unused_result attribute on OS X 10.9?
I will likely _exit(1) if write returns -1, as opposed to ignoring the return value, if that sounds good to you guys.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33437
-----------------------------------------------------------
On Jan. 30, 2014, 12:28 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17306/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2014, 12:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Feb. 3, 2014, 9:10 a.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 157-159
> > <https://reviews.apache.org/r/17306/diff/5/?file=454081#file454081line157>
> >
> > Hey Ben,
> >
> > These lines fail to compile on GCC 4.8.1:
> > error: ignoring return value of ‘ssize_t write(int, const void*, size_t)’, declared with attribute warn_unused_result [-Werror=unused-result]
>
> Ian Downes wrote:
> This is the same thing I missed when using write. I just explicitly ignored the return value with (void) write(...). Is that an acceptable solution?
>
> Niklas Nielsen wrote:
> Unfortunately not; still breaks.
>
> Think you need to read it:
>
> ssize_t written = ...
> (void)written;
>
> Ben Mahler wrote:
> Interesting, I don't have this issue with gcc-4.8.2 on OS X 10.8. Perhaps write is only declared with the warn_unused_result attribute on OS X 10.9?
>
> I will likely _exit(1) if write returns -1, as opposed to ignoring the return value, if that sounds good to you guys.
>
> Niklas Nielsen wrote:
> I ran into the problem on "gcc version 4.8.1 (Ubuntu/Linaro 4.8.1-10ubuntu9)"
>
> Ben Mahler wrote:
> What about this?
>
> diff --git a/3rdparty/libprocess/include/process/subprocess.hpp b/3rdparty/libprocess/include/process/subprocess.hpp
> index db9c96b..452eeea 100644
> --- a/3rdparty/libprocess/include/process/subprocess.hpp
> +++ b/3rdparty/libprocess/include/process/subprocess.hpp
> @@ -154,9 +154,12 @@ inline Try<Subprocess> subprocess(const std::string& command)
> // implemented in an unsafe manner:
> // http://austingroupbugs.net/view.php?id=692
> const char* message = "Failed to execl '/bin sh -c ";
> - write(STDERR_FILENO, message, strlen(message));
> - write(STDERR_FILENO, command.c_str(), command.size());
> - write(STDERR_FILENO, "'\n", strlen("'\n"));
> + while (write(STDERR_FILENO, message, strlen(message)) == -1 &&
> + errno == EINTR);
> + while (write(STDERR_FILENO, command.c_str(), command.size()) == -1 &&
> + errno == EINTR);
> + while (write(STDERR_FILENO, "'\n", strlen("'\n")) == -1 &&
> + errno == EINTR);
>
> _exit(1);
> }
That should work. Thanks!
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33437
-----------------------------------------------------------
On Jan. 29, 2014, 4:28 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, 4:28 p.m.)
>
>
> Review request for mesos, Benjamin Hindman 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Feb. 3, 2014, 9:10 a.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 157-159
> > <https://reviews.apache.org/r/17306/diff/5/?file=454081#file454081line157>
> >
> > Hey Ben,
> >
> > These lines fail to compile on GCC 4.8.1:
> > error: ignoring return value of ‘ssize_t write(int, const void*, size_t)’, declared with attribute warn_unused_result [-Werror=unused-result]
>
> Ian Downes wrote:
> This is the same thing I missed when using write. I just explicitly ignored the return value with (void) write(...). Is that an acceptable solution?
>
> Niklas Nielsen wrote:
> Unfortunately not; still breaks.
>
> Think you need to read it:
>
> ssize_t written = ...
> (void)written;
>
> Ben Mahler wrote:
> Interesting, I don't have this issue with gcc-4.8.2 on OS X 10.8. Perhaps write is only declared with the warn_unused_result attribute on OS X 10.9?
>
> I will likely _exit(1) if write returns -1, as opposed to ignoring the return value, if that sounds good to you guys.
I ran into the problem on "gcc version 4.8.1 (Ubuntu/Linaro 4.8.1-10ubuntu9)"
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33437
-----------------------------------------------------------
On Jan. 29, 2014, 4:28 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, 4:28 p.m.)
>
>
> Review request for mesos, Benjamin Hindman 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ian Downes <ia...@gmail.com>.
> On Feb. 3, 2014, 5:10 p.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 157-159
> > <https://reviews.apache.org/r/17306/diff/5/?file=454081#file454081line157>
> >
> > Hey Ben,
> >
> > These lines fail to compile on GCC 4.8.1:
> > error: ignoring return value of ‘ssize_t write(int, const void*, size_t)’, declared with attribute warn_unused_result [-Werror=unused-result]
This is the same thing I missed when using write. I just explicitly ignored the return value with (void) write(...). Is that an acceptable solution?
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33437
-----------------------------------------------------------
On Jan. 30, 2014, 12:28 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17306/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2014, 12:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33437
-----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/17306/#comment62875>
Hey Ben,
These lines fail to compile on GCC 4.8.1:
error: ignoring return value of ‘ssize_t write(int, const void*, size_t)’, declared with attribute warn_unused_result [-Werror=unused-result]
- Niklas Nielsen
On Jan. 29, 2014, 4:28 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, 4:28 p.m.)
>
>
> Review request for mesos, Benjamin Hindman 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Jason Dusek <ja...@gmail.com>.
> On Feb. 3, 2014, 7:10 p.m., Jason Dusek wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 149
> > <https://reviews.apache.org/r/17306/diff/5/?file=454081#file454081line149>
> >
> > This order of arguments is not likely to work consistently across platforms.
> >
> > :; /bin/sh sh -c "echo x"
> > /bin/sh: /bin/sh: cannot execute binary file
> > :; /bin/sh -c "echo x" sh
> > x
> >
> > Using execvp() to pass an argument vector to `sh` would prevent us from mixing shell semantics in to the core of Mesos:
> >
> > _argv = ["-c", "exec \"$@\"", "sh"] + argv;
> > execvp("/bin/sh", _argv);
> >
> > I would like to request that this review be re-opened until this change is made.
Oh, hey, scratch the comment about argument order...I didn't realize execl() let's you set argv[0].
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33469
-----------------------------------------------------------
On Jan. 30, 2014, 12:28 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17306/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2014, 12:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Jason Dusek <ja...@gmail.com>.
> On Feb. 3, 2014, 7:10 p.m., Jason Dusek wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 149
> > <https://reviews.apache.org/r/17306/diff/5/?file=454081#file454081line149>
> >
> > This order of arguments is not likely to work consistently across platforms.
> >
> > :; /bin/sh sh -c "echo x"
> > /bin/sh: /bin/sh: cannot execute binary file
> > :; /bin/sh -c "echo x" sh
> > x
> >
> > Using execvp() to pass an argument vector to `sh` would prevent us from mixing shell semantics in to the core of Mesos:
> >
> > _argv = ["-c", "exec \"$@\"", "sh"] + argv;
> > execvp("/bin/sh", _argv);
> >
> > I would like to request that this review be re-opened until this change is made.
>
> Jason Dusek wrote:
> Oh, hey, scratch the comment about argument order...I didn't realize execl() let's you set argv[0].
>
> Ben Mahler wrote:
> Ok, no worries!
Using exec() instead of shell is still an important issue, though, and I hope we'll be able to re-open this review to address it.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33469
-----------------------------------------------------------
On Jan. 30, 2014, 12:28 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17306/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2014, 12:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 3, 2014, 7:10 p.m., Jason Dusek wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 149
> > <https://reviews.apache.org/r/17306/diff/5/?file=454081#file454081line149>
> >
> > This order of arguments is not likely to work consistently across platforms.
> >
> > :; /bin/sh sh -c "echo x"
> > /bin/sh: /bin/sh: cannot execute binary file
> > :; /bin/sh -c "echo x" sh
> > x
> >
> > Using execvp() to pass an argument vector to `sh` would prevent us from mixing shell semantics in to the core of Mesos:
> >
> > _argv = ["-c", "exec \"$@\"", "sh"] + argv;
> > execvp("/bin/sh", _argv);
> >
> > I would like to request that this review be re-opened until this change is made.
>
> Jason Dusek wrote:
> Oh, hey, scratch the comment about argument order...I didn't realize execl() let's you set argv[0].
Ok, no worries!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33469
-----------------------------------------------------------
On Jan. 30, 2014, 12:28 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17306/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2014, 12:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 3, 2014, 7:10 p.m., Jason Dusek wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 149
> > <https://reviews.apache.org/r/17306/diff/5/?file=454081#file454081line149>
> >
> > This order of arguments is not likely to work consistently across platforms.
> >
> > :; /bin/sh sh -c "echo x"
> > /bin/sh: /bin/sh: cannot execute binary file
> > :; /bin/sh -c "echo x" sh
> > x
> >
> > Using execvp() to pass an argument vector to `sh` would prevent us from mixing shell semantics in to the core of Mesos:
> >
> > _argv = ["-c", "exec \"$@\"", "sh"] + argv;
> > execvp("/bin/sh", _argv);
> >
> > I would like to request that this review be re-opened until this change is made.
>
> Jason Dusek wrote:
> Oh, hey, scratch the comment about argument order...I didn't realize execl() let's you set argv[0].
>
> Ben Mahler wrote:
> Ok, no worries!
>
> Jason Dusek wrote:
> Using exec() instead of shell is still an important issue, though, and I hope we'll be able to re-open this review to address it.
Ian will be using this utility in his containerization changes shortly, we can certainly update this to have another exec-style interface. There are also some other things Ian may want that were brought up in the previous reviews.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33469
-----------------------------------------------------------
On Jan. 30, 2014, 12:28 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17306/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2014, 12:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Jason Dusek <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33469
-----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/17306/#comment62907>
This order of arguments is not likely to work consistently across platforms.
:; /bin/sh sh -c "echo x"
/bin/sh: /bin/sh: cannot execute binary file
:; /bin/sh -c "echo x" sh
x
Using execvp() to pass an argument vector to `sh` would prevent us from mixing shell semantics in to the core of Mesos:
_argv = ["-c", "exec \"$@\"", "sh"] + argv;
execvp("/bin/sh", _argv);
I would like to request that this review be re-opened until this change is made.
- Jason Dusek
On Jan. 30, 2014, 12:28 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17306/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2014, 12:28 a.m.)
>
>
> Review request for mesos, Benjamin Hindman 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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/
-----------------------------------------------------------
(Updated Jan. 30, 2014, 12:28 a.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Added some close calls that were missed during the last update to the diff.
Removed the copy / assignment constructors in favor of using the defaults.
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 (updated)
-----
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
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Refactored to return a Try<Subprocess> and ensured we close pipes when returning an Error.
Moved the pipe closing code into a Data destructor which cleans up the code a bit.
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 (updated)
-----
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
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 29, 2014, 7:39 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 37
> > <https://reviews.apache.org/r/17306/diff/3/?file=453457#file453457line37>
> >
> > 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. ;)
Alright I've updated the others to have the better names! :)
> On Jan. 29, 2014, 7:39 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 91
> > <https://reviews.apache.org/r/17306/diff/3/?file=453457#file453457line91>
> >
> > This comment is helpful, but could be less esoteric for someone else with a few more words. ;)
Done.
> On Jan. 29, 2014, 7:39 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 97
> > <https://reviews.apache.org/r/17306/diff/3/?file=453457#file453457line97>
> >
> > Why not return Error?
It felt likely that these errors would be considered fatal and the logic was a bit trickier to cleanup when an error was encountered. But I I've updated this to return a Try since I think it's the right thing: give the caller the control.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33111
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ian Downes <ia...@gmail.com>.
> On Jan. 29, 2014, 6:34 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 124
> > <https://reviews.apache.org/r/17306/diff/3/?file=453457#file453457line124>
> >
> > can we just use constants (e.g:. 29 and 3) here instead of strlen and add a note that strlen is not async signal safe? Or maybe calculate lengths using strlen() even before fork()?
[As Ben said earlier] It's highly likely the compiler will optimize strlen away anyway. Alternatives are sizeof("foobar") - 1 or reimplement strlen safely.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33129
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 29, 2014, 6:34 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 124
> > <https://reviews.apache.org/r/17306/diff/3/?file=453457#file453457line124>
> >
> > can we just use constants (e.g:. 29 and 3) here instead of strlen and add a note that strlen is not async signal safe? Or maybe calculate lengths using strlen() even before fork()?
>
> Ian Downes wrote:
> [As Ben said earlier] It's highly likely the compiler will optimize strlen away anyway. Alternatives are sizeof("foobar") - 1 or reimplement strlen safely.
I think it's a safe assumption given my 3 points above in Ian's review, I've added a comment reflecting this, but if you feel we should pre-construct the strings above the fork I can do so (I think we have a lot more async signal safety to worry about in the existing launcher code).
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33129
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review33129
-----------------------------------------------------------
Ship it!
Good stuff!
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/17306/#comment62420>
unused?
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/17306/#comment62412>
can we just use constants (e.g:. 29 and 3) here instead of strlen and add a note that strlen is not async signal safe? Or maybe calculate lengths using strlen() even before fork()?
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/17306/#comment62417>
+1
- Vinod Kone
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Cleaned up includes.
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 (updated)
-----
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
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/
-----------------------------------------------------------
(Updated Jan. 27, 2014, 10:42 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Review comments, primarily this now ensures that the subprocess is terminated before we close the file descriptors by binding the Subprocess into the reap callback to ensure a copy of the Subprocess remains so long as it is running.
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 (updated)
-----
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
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 24, 2014, 6:16 p.m., Ian Downes wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 120
> > <https://reviews.apache.org/r/17306/diff/1/?file=447773#file447773line120>
> >
> > should we check their exit codes since dup2 can fail even with valid descriptors.
I've handled EINTR now, and updating os::close to handle EINTR as well in a separate review.
It doesn't appear that the other error codes are possible here:
EBADF: oldfd was opened above since we check the result of pipe, newfd is in the valid range.
EBUSY: (Linux), doesn't look like we're susceptible to a race condition between open and dup2.
EINTR: Handled this now.
EINVAL: Not possible here.
EMFILE: Since dup2 will close newfd (stdin), it does not seem possible to for dup2 to cause the number of open file descriptors in the subprocess to be exceeded.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review32726
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review32726
-----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/17306/#comment61679>
should we check their exit codes since dup2 can fail even with valid descriptors.
- Ian Downes
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 24, 2014, 8:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 163-165
> > <https://reviews.apache.org/r/17306/diff/1/?file=447773#file447773line163>
> >
> > 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.
I've opted to remove the discard semantics, but I've also omitted a kill(), since the user has access to the pid and so it seems best for now to just let the caller determine how to kill (which signal, kill escalation SIGTERM -> SIGKILL, etc).
> On Jan. 24, 2014, 8:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 30
> > <https://reviews.apache.org/r/17306/diff/1/?file=447773#file447773line30>
> >
> > What's the reason not having a copy constructor?
Added one.
> On Jan. 24, 2014, 8:12 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 157
> > <https://reviews.apache.org/r/17306/diff/1/?file=447773#file447773line157>
> >
> > 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).
Ah perfect, I've already changed this to become process::reap in the previous review, which now uses a lazily initialized Reaper, please take a look! :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17306/#review32740
-----------------------------------------------------------
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
>
>
Re: Review Request 17306: Added an asynchronous subprocess utility.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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
>
>