Repository: mesos Updated Branches: refs/heads/master e48e9288b -> 3c730dc42
Refactored subprocess options [1/2]. Previously the subprocess interface supported a several options for the child process such as setsid. In order to make the interface more flexible we refactored such options into a vector of ChildHooks. In order not to allow arbitrary code inside a ChildHook it has to be constructed via pre-defined factory methods. Review: https://reviews.apache.org/r/45491/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/5ce0e46a Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/5ce0e46a Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/5ce0e46a Branch: refs/heads/master Commit: 5ce0e46aeb083de1af09d53364ac7260441e9e94 Parents: e48e928 Author: Joerg Schad <jo...@mesosphere.io> Authored: Wed Sep 21 09:50:15 2016 -0700 Committer: Jie Yu <yujie....@gmail.com> Committed: Wed Sep 21 09:50:15 2016 -0700 ---------------------------------------------------------------------- .../include/process/posix/subprocess.hpp | 29 +++---- .../libprocess/include/process/ssl/gtest.hpp | 1 - .../include/process/subprocess_base.hpp | 81 +++++++++++++------- 3rdparty/libprocess/src/subprocess.cpp | 38 ++++++++- .../libprocess/src/tests/subprocess_tests.cpp | 7 -- 5 files changed, 95 insertions(+), 61 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/5ce0e46a/3rdparty/libprocess/include/process/posix/subprocess.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/include/process/posix/subprocess.hpp b/3rdparty/libprocess/include/process/posix/subprocess.hpp index 1e9a7b9..d82db3c 100644 --- a/3rdparty/libprocess/include/process/posix/subprocess.hpp +++ b/3rdparty/libprocess/include/process/posix/subprocess.hpp @@ -206,13 +206,12 @@ inline int childMain( const string& path, char** argv, char** envp, - const Setsid set_sid, const InputFileDescriptors& stdinfds, const OutputFileDescriptors& stdoutfds, const OutputFileDescriptors& stderrfds, bool blocking, int pipes[2], - const Option<string>& working_directory, + const vector<Subprocess::ChildHook>& child_hooks, const Watchdog watchdog) { // Close parent's end of the pipes. @@ -274,21 +273,13 @@ inline int childMain( ::close(pipes[0]); } - // Move to a different session (and new process group) so we're - // independent from the caller's session (otherwise children will - // receive SIGHUP if the slave exits). - if (set_sid == SETSID) { - // POSIX guarantees a forked child's pid does not match any existing - // process group id so only a single `setsid()` is required and the - // session id will be the pid. - if (::setsid() == -1) { - ABORT("Failed to put child in a new session"); - } - } + // Run the child hooks. + foreach (const Subprocess::ChildHook& hook, child_hooks) { + Try<Nothing> callback = hook(); - if (working_directory.isSome()) { - if (::chdir(working_directory->c_str()) == -1) { - ABORT("Failed to change directory"); + // If the callback failed, we should abort execution. + if (callback.isError()) { + ABORT("Failed to execute Subprocess::ChildHook: " + callback.error()); } } @@ -311,12 +302,11 @@ inline int childMain( inline Try<pid_t> cloneChild( const string& path, vector<string> argv, - const Setsid set_sid, const Option<map<string, string>>& environment, const Option<lambda::function< pid_t(const lambda::function<int()>&)>>& _clone, const vector<Subprocess::Hook>& parent_hooks, - const Option<string>& working_directory, + const vector<Subprocess::ChildHook>& child_hooks, const Watchdog watchdog, const InputFileDescriptors stdinfds, const OutputFileDescriptors stdoutfds, @@ -373,13 +363,12 @@ inline Try<pid_t> cloneChild( path, _argv, envp, - set_sid, stdinfds, stdoutfds, stderrfds, blocking, pipes, - working_directory, + child_hooks, watchdog)); delete[] _argv; http://git-wip-us.apache.org/repos/asf/mesos/blob/5ce0e46a/3rdparty/libprocess/include/process/ssl/gtest.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/include/process/ssl/gtest.hpp b/3rdparty/libprocess/include/process/ssl/gtest.hpp index 2ad623a..21a0fc4 100644 --- a/3rdparty/libprocess/include/process/ssl/gtest.hpp +++ b/3rdparty/libprocess/include/process/ssl/gtest.hpp @@ -361,7 +361,6 @@ protected: process::Subprocess::PIPE(), process::Subprocess::PIPE(), process::Subprocess::FD(STDERR_FILENO), - process::NO_SETSID, nullptr, environment); } http://git-wip-us.apache.org/repos/asf/mesos/blob/5ce0e46a/3rdparty/libprocess/include/process/subprocess_base.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/include/process/subprocess_base.hpp b/3rdparty/libprocess/include/process/subprocess_base.hpp index 23bc14a..0502f96 100644 --- a/3rdparty/libprocess/include/process/subprocess_base.hpp +++ b/3rdparty/libprocess/include/process/subprocess_base.hpp @@ -34,14 +34,7 @@ namespace process { -// Flag describing whether a new process should generate a new sid. -enum Setsid -{ - SETSID, - NO_SETSID, -}; - -// Flag describing whether a new process should be monitored by a seperate +// Flag describing whether a new process should be monitored by a separate // watch process and be killed in case the parent process dies. enum Watchdog { MONITOR, @@ -61,6 +54,7 @@ class Subprocess public: // Forward declarations. struct Hook; + class ChildHook; /** * Describes how the I/O is redirected for stdin/stdout/stderr. @@ -97,7 +91,7 @@ public: }; /** - * For output file descriptors a child write to the `write` file + * For output file descriptors a child writes to the `write` file * descriptor and a parent may read from the `read` file * descriptor if one is present. * @@ -145,13 +139,12 @@ public: const Subprocess::IO& in, const Subprocess::IO& out, const Subprocess::IO& err, - const Setsid set_sid, const flags::FlagsBase* flags, const Option<std::map<std::string, std::string>>& environment, const Option<lambda::function< pid_t(const lambda::function<int()>&)>>& clone, const std::vector<Subprocess::Hook>& parent_hooks, - const Option<std::string>& working_directory, + const std::vector<Subprocess::ChildHook>& child_hooks, const Watchdog watchdog); IO(const lambda::function<Try<InputFileDescriptors>()>& _input, @@ -185,8 +178,8 @@ public: Hook(const lambda::function<Try<Nothing>(pid_t)>& _parent_callback); /** - * The callback that must be sepcified for execution after the - * child has been cloned, but before it start executing the new + * The callback that must be specified for execution after the + * child has been cloned, but before it starts executing the new * process. This provides access to the child pid after its * initialization to add tracking or modify execution state of * the child before it executes the new process. @@ -196,6 +189,42 @@ public: friend class Subprocess; }; + /** + * A `ChildHook` can be passed to a `subprocess` call. It provides a way to + * inject predefined behavior between the clone and exec calls in the + * child process. + * As such `ChildHooks` have to fulfill certain criteria (especially + * being async safe) the class does not offer a public constructor. + * Instead instances can be created via factory methods. + * NOTE: Returning an error from a childHook causes the child process to + * abort. + */ + class ChildHook + { + public: + /** + * `ChildHook` for changing the working directory. + */ + static ChildHook CHDIR(const std::string& working_directory); + + /** + * `ChildHook` for generating a new session id. + */ + static ChildHook SETSID(); + + /** + * Returns an empty list of `ChildHooks`. + */ + static std::vector<ChildHook> None() { return std::vector<ChildHook>(); } + + Try<Nothing> operator()() const { return child_setup(); } + + private: + ChildHook(const lambda::function<Try<Nothing>()>& _child_setup); + + const lambda::function<Try<Nothing>()> child_setup; + }; + // Some syntactic sugar to create an IO::PIPE redirector. static IO PIPE(); static IO PATH(const std::string& path); @@ -269,13 +298,12 @@ private: const Subprocess::IO& in, const Subprocess::IO& out, const Subprocess::IO& err, - const Setsid setsid, const flags::FlagsBase* flags, const Option<std::map<std::string, std::string>>& environment, const Option<lambda::function< pid_t(const lambda::function<int()>&)>>& clone, const std::vector<Subprocess::Hook>& parent_hooks, - const Option<std::string>& working_directory, + const std::vector<Subprocess::ChildHook>& child_hooks, const Watchdog watchdog); struct Data @@ -331,8 +359,6 @@ private: * @param in Redirection specification for stdin. * @param out Redirection specification for stdout. * @param err Redirection specification for stderr. - * @param set_sid Indicator whether the process should be placed in - * a new session after the 'parent_hooks' have been executed. * @param flags Flags to be stringified and appended to 'argv'. * @param environment Environment variables to use for the new * subprocess or if None (the default) then the new subprocess @@ -341,8 +367,8 @@ private: * subprocess. * @param parent_hooks Hooks that will be executed in the parent * before the child execs. - * @param working_directory Directory in which the process should - * chdir before exec after the 'parent_hooks' have been executed. + * @param child_hooks Hooks that will be executed in the child + * before the child execs but after parent_hooks have executed. * @param watchdog Indicator whether the new process should be monitored * and killed if the parent process terminates. * @return The subprocess or an error if one occurred. @@ -355,14 +381,14 @@ Try<Subprocess> subprocess( const Subprocess::IO& in = Subprocess::FD(STDIN_FILENO), const Subprocess::IO& out = Subprocess::FD(STDOUT_FILENO), const Subprocess::IO& err = Subprocess::FD(STDERR_FILENO), - const Setsid set_sid = NO_SETSID, const flags::FlagsBase* flags = nullptr, const Option<std::map<std::string, std::string>>& environment = None(), const Option<lambda::function< pid_t(const lambda::function<int()>&)>>& clone = None(), const std::vector<Subprocess::Hook>& parent_hooks = Subprocess::Hook::None(), - const Option<std::string>& working_directory = None(), + const std::vector<Subprocess::ChildHook>& child_hooks = + Subprocess::ChildHook::None(), const Watchdog watchdog = NO_MONITOR); @@ -377,8 +403,6 @@ Try<Subprocess> subprocess( * @param in Redirection specification for stdin. * @param out Redirection specification for stdout. * @param err Redirection specification for stderr. - * @param set_sid Indicator whether the process should be placed in - * a new session after the 'parent_hooks' have been executed. * @param environment Environment variables to use for the new * subprocess or if None (the default) then the new subprocess * will inherit the environment of the current process. @@ -386,8 +410,8 @@ Try<Subprocess> subprocess( * subprocess. * @param parent_hooks Hooks that will be executed in the parent * before the child execs. - * @param working_directory Directory in which the process should - * chdir before exec after the 'parent_hooks' have been executed. + * @param child_hooks Hooks that will be executed in the child + * before the child execs but after parent_hooks have executed. * @param watchdog Indicator whether the new process should be monitored * and killed if the parent process terminates. * @return The subprocess or an error if one occurred. @@ -399,13 +423,13 @@ inline Try<Subprocess> subprocess( const Subprocess::IO& in = Subprocess::FD(STDIN_FILENO), const Subprocess::IO& out = Subprocess::FD(STDOUT_FILENO), const Subprocess::IO& err = Subprocess::FD(STDERR_FILENO), - const Setsid set_sid = NO_SETSID, const Option<std::map<std::string, std::string>>& environment = None(), const Option<lambda::function< pid_t(const lambda::function<int()>&)>>& clone = None(), const std::vector<Subprocess::Hook>& parent_hooks = Subprocess::Hook::None(), - const Option<std::string>& working_directory = None(), + const std::vector<Subprocess::ChildHook>& child_hooks = + Subprocess::ChildHook::None(), const Watchdog watchdog = NO_MONITOR) { std::vector<std::string> argv = {os::Shell::arg0, os::Shell::arg1, command}; @@ -416,12 +440,11 @@ inline Try<Subprocess> subprocess( in, out, err, - set_sid, nullptr, environment, clone, parent_hooks, - working_directory, + child_hooks, watchdog); } http://git-wip-us.apache.org/repos/asf/mesos/blob/5ce0e46a/3rdparty/libprocess/src/subprocess.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/subprocess.cpp b/3rdparty/libprocess/src/subprocess.cpp index 8ca61f7..ce5c88f 100644 --- a/3rdparty/libprocess/src/subprocess.cpp +++ b/3rdparty/libprocess/src/subprocess.cpp @@ -53,6 +53,38 @@ Subprocess::Hook::Hook( const lambda::function<Try<Nothing>(pid_t)>& _parent_callback) : parent_callback(_parent_callback) {} + +Subprocess::ChildHook::ChildHook( + const lambda::function<Try<Nothing>()>& _child_setup) + : child_setup(_child_setup) {} + + +Subprocess::ChildHook Subprocess::ChildHook::CHDIR( + const std::string& working_directory) +{ + return Subprocess::ChildHook([working_directory]() -> Try<Nothing> { + if (::chdir(working_directory.c_str()) == -1) { + return Error("Could not chdir"); + } + + return Nothing(); + }); +} + + +Subprocess::ChildHook Subprocess::ChildHook::SETSID() +{ + return Subprocess::ChildHook([]() -> Try<Nothing> { + // Put child into its own process session to prevent slave suicide + // on child process SIGKILL/SIGTERM. + if (::setsid() == -1) { + return Error("Could not setsid"); + } + + return Nothing(); + }); +} + namespace internal { static void cleanup( @@ -92,13 +124,12 @@ Try<Subprocess> subprocess( const Subprocess::IO& in, const Subprocess::IO& out, const Subprocess::IO& err, - const Setsid set_sid, const flags::FlagsBase* flags, const Option<map<string, string>>& environment, const Option<lambda::function< pid_t(const lambda::function<int()>&)>>& _clone, const vector<Subprocess::Hook>& parent_hooks, - const Option<string>& working_directory, + const vector<Subprocess::ChildHook>& child_hooks, const Watchdog watchdog) { // File descriptors for redirecting stdin/stdout/stderr. @@ -172,11 +203,10 @@ Try<Subprocess> subprocess( Try<pid_t> pid = internal::cloneChild( path, argv, - set_sid, environment, _clone, parent_hooks, - working_directory, + child_hooks, watchdog, stdinfds, stdoutfds, http://git-wip-us.apache.org/repos/asf/mesos/blob/5ce0e46a/3rdparty/libprocess/src/tests/subprocess_tests.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/tests/subprocess_tests.cpp b/3rdparty/libprocess/src/tests/subprocess_tests.cpp index 0ff3945..e32f3e5 100644 --- a/3rdparty/libprocess/src/tests/subprocess_tests.cpp +++ b/3rdparty/libprocess/src/tests/subprocess_tests.cpp @@ -222,7 +222,6 @@ TEST_F(SubprocessTest, EnvironmentEcho) Subprocess::FD(STDIN_FILENO), Subprocess::PATH(outfile), Subprocess::FD(STDERR_FILENO), - process::Setsid::NO_SETSID, environment); }); @@ -766,7 +765,6 @@ TEST_F(SubprocessTest, Flags) Subprocess::FD(STDIN_FILENO), Subprocess::PATH(out), Subprocess::FD(STDERR_FILENO), - process::NO_SETSID, &flags); ASSERT_SOME(s); @@ -833,7 +831,6 @@ TEST_F(SubprocessTest, Environment) Subprocess::FD(STDIN_FILENO), Subprocess::PIPE(), Subprocess::FD(STDERR_FILENO), - process::NO_SETSID, environment); ASSERT_SOME(s); @@ -865,7 +862,6 @@ TEST_F(SubprocessTest, Environment) Subprocess::FD(STDIN_FILENO), Subprocess::PIPE(), Subprocess::FD(STDERR_FILENO), - process::NO_SETSID, environment); ASSERT_SOME(s); @@ -900,7 +896,6 @@ TEST_F(SubprocessTest, EnvironmentWithSpaces) Subprocess::FD(STDIN_FILENO), Subprocess::PIPE(), Subprocess::FD(STDERR_FILENO), - process::NO_SETSID, environment); ASSERT_SOME(s); @@ -935,7 +930,6 @@ TEST_F(SubprocessTest, EnvironmentWithSpacesAndQuotes) Subprocess::FD(STDIN_FILENO), Subprocess::PIPE(), Subprocess::FD(STDERR_FILENO), - process::NO_SETSID, environment); ASSERT_SOME(s); @@ -973,7 +967,6 @@ TEST_F(SubprocessTest, EnvironmentOverride) Subprocess::FD(STDIN_FILENO), Subprocess::PIPE(), Subprocess::FD(STDERR_FILENO), - process::NO_SETSID, environment); ASSERT_SOME(s);
