You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2014/04/03 06:18:34 UTC
Re: Review Request 19259: Added ability to process::subprocess to run
function within child context
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19259/#review39393
-----------------------------------------------------------
This looks pretty good Till. Just a few cleanups and then let's get it submitted.
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/19259/#comment71792>
How about:
const Option<lambda::function<int(void)> >& setup = None());
Let's document this as a 'setup' function that can be run after forking but before executing the command that must be async-signal safe. Also mention that if it returns a non-zero error code that's what the status of the Subprocess will be (consider describing it as 'setup && command').
Also, let's make 'environment' be an Option now too since it'll be cleaner to pass 'None()' then an empty map.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/19259/#comment71793>
Let's not do this as it's not standard in the code base and not really needed.
3rdparty/libprocess/src/tests/subprocess_tests.cpp
<https://reviews.apache.org/r/19259/#comment71794>
Knowing what you want to use this for, how about the following as a test. ;)
int setup(const string& directory)
{
// Keep everything async-signal safe.
if (::chdir(directory.c_str()) == -1) {
return errno;
}
if (::setsid() == -1) {
return errno;
}
return 0;
}
TEST(Subprocess, setup)
{
Clock::pause();
Try<string> directory = os::mkdtemp();
ASSERT_SOME(directory);
Try<Subprocess> s = subprocess(
"echo hello world >file && sleep 60",
None(),
lambda::bind(&setup, directory.get()));
ASSERT_SOME(s);
// Verify the process is in a different session.
EXPECT_NE(getsid(0), getsid(s.get().pid()));
kill(s.get().pid(), SIGTERM);
// Advance time until the internal reaper reaps the subprocess.
while (s.get().status().isPending()) {
Clock::advance(Seconds(1));
Clock::settle();
}
AWAIT_ASSERT_READY(s.get().status());
ASSERT_SOME(s.get().status().get());
int status = s.get().status().get().get();
ASSERT_TRUE(WIFSIGNALED(status));
ASSERT_EQ(SIGTERM, WTERMSIG(status));
// Make sure 'file' is there and contains 'hello world'.
const string& path = os::join(directory.get(), "file");
EXPECT_TRUE(os::exists(path));
EXPECT_SOME_EQ("hello world", os::read(path));
os::rmdir(directory.get());
Clock::resume();
}
- Benjamin Hindman
On March 16, 2014, 11:05 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19259/
> -----------------------------------------------------------
>
> (Updated March 16, 2014, 11:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Dominic Hamon, and Ian Downes.
>
>
> Bugs: MESOS-1102
> https://issues.apache.org/jira/browse/MESOS-1102
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds the ability to process::subprocess to run a function (lambda) within the child context, after fork and before exec.
>
> NOTE: Such lambda must not contain any async unsafe code. For details on async safety, see POSIX.1-2004 on async-signal-safe functions, also referenced in the signal man-pages: http://man7.org/linux/man-pages/man7/signal.7.html.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19259/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 19259: Added ability to process::subprocess to run
function within child context
Posted by Till Toenshoff <to...@me.com>.
> On April 3, 2014, 4:18 a.m., Benjamin Hindman wrote:
> > This looks pretty good Till. Just a few cleanups and then let's get it submitted.
Thanks Ben!
> On April 3, 2014, 4:18 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/tests/subprocess_tests.cpp, lines 342-344
> > <https://reviews.apache.org/r/19259/diff/2/?file=520577#file520577line342>
> >
> > Knowing what you want to use this for, how about the following as a test. ;)
> >
> > int setup(const string& directory)
> > {
> > // Keep everything async-signal safe.
> > if (::chdir(directory.c_str()) == -1) {
> > return errno;
> > }
> >
> > if (::setsid() == -1) {
> > return errno;
> > }
> >
> > return 0;
> > }
> >
> >
> > TEST(Subprocess, setup)
> > {
> > Clock::pause();
> >
> > Try<string> directory = os::mkdtemp();
> > ASSERT_SOME(directory);
> >
> > Try<Subprocess> s = subprocess(
> > "echo hello world >file && sleep 60",
> > None(),
> > lambda::bind(&setup, directory.get()));
> >
> > ASSERT_SOME(s);
> >
> > // Verify the process is in a different session.
> > EXPECT_NE(getsid(0), getsid(s.get().pid()));
> >
> > kill(s.get().pid(), SIGTERM);
> >
> > // Advance time until the internal reaper reaps the subprocess.
> > while (s.get().status().isPending()) {
> > Clock::advance(Seconds(1));
> > Clock::settle();
> > }
> >
> > AWAIT_ASSERT_READY(s.get().status());
> > ASSERT_SOME(s.get().status().get());
> >
> > int status = s.get().status().get().get();
> > ASSERT_TRUE(WIFSIGNALED(status));
> > ASSERT_EQ(SIGTERM, WTERMSIG(status));
> >
> > // Make sure 'file' is there and contains 'hello world'.
> > const string& path = os::join(directory.get(), "file");
> > EXPECT_TRUE(os::exists(path));
> > EXPECT_SOME_EQ("hello world", os::read(path));
> >
> > os::rmdir(directory.get());
> >
> > Clock::resume();
> > }
Had to split this up as the SIGTERM was coming in too early, aborting/preventing the echo > file.
Also added another test for covering the 'setup' return value logic.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19259/#review39393
-----------------------------------------------------------
On March 16, 2014, 11:05 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19259/
> -----------------------------------------------------------
>
> (Updated March 16, 2014, 11:05 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Dominic Hamon, and Ian Downes.
>
>
> Bugs: MESOS-1102
> https://issues.apache.org/jira/browse/MESOS-1102
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Adds the ability to process::subprocess to run a function (lambda) within the child context, after fork and before exec.
>
> NOTE: Such lambda must not contain any async unsafe code. For details on async safety, see POSIX.1-2004 on async-signal-safe functions, also referenced in the signal man-pages: http://man7.org/linux/man-pages/man7/signal.7.html.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp d16cbc1e3d464e1784f116ccdb327cf0784f07c2
> 3rdparty/libprocess/src/subprocess.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp d15d4d159105474117c4ea432b215431209ab539
>
> Diff: https://reviews.apache.org/r/19259/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>