Cody Maloney created MESOS-2811:
-----------------------------------
Summary: <process/subprocess.hpp> API hard to use, extend
Key: MESOS-2811
URL: https://issues.apache.org/jira/browse/MESOS-2811
Project: Mesos
Issue Type: Improvement
Components: slave
Affects Versions: 0.22.1
Reporter: Cody Maloney
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/subprocess.hpp
There are many overloads of subprocess() construction, a lot of them are very
similar.
It passes environment in as an {{Option<std::map<std::string, std::string>>}}
which isn't what stout's os::environment() returns. ({{hashmap<std::string,
std::string> environment()}}. Ideally those should match for easy passing
environments around + manipulating
It isn't possible to tell it not to copy in the environment of running process
(Useful to isolate slave environments from the running process). This becomes
critical when configuring mesos via environment variables. Currently mesos
explicitly unsets LIBPROCESS_IP when launching new processes because that one
is known to upset when mesos launches another libprocess based thing.
ExecEnv is just weird, it isn't great / modern C++, and results in a lot of
unnecessary / useless copies of things as current, doesn't follow modern C++
interface standards.
The code is hard to read / follow:
{code}
// Close the copies. We need to make sure that we do not close the
// file descriptor assigned to stdin/stdout/stderr in case the
// parent has closed stdin/stdout/stderr when calling this
// function (in that case, a dup'ed file descriptor may have the
// same file descriptor number as stdin/stdout/stderr).
if (stdinFd[0] != STDIN_FILENO &&
stdinFd[0] != STDOUT_FILENO &&
stdinFd[0] != STDERR_FILENO) {
while (::close(stdinFd[0]) == -1 && errno == EINTR);
}
if (stdoutFd[1] != STDIN_FILENO &&
stdoutFd[1] != STDOUT_FILENO &&
stdoutFd[1] != STDERR_FILENO) {
while (::close(stdoutFd[1]) == -1 && errno == EINTR);
}
if (stderrFd[1] != STDIN_FILENO &&
stderrFd[1] != STDOUT_FILENO &&
stderrFd[1] != STDERR_FILENO) {
while (::close(stderrFd[1]) == -1 && errno == EINTR);
}
{code}
Why do we switch between fd[0] vs [1]? Why are we hand-coding "While EINTR"
loops over and over? Doesn't stout have an os::close?
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/subprocess.cpp#L165
-- os::execvpe() can fail for perfectly good reasons, we should really log the
name of the command / info that was trying to be run. There shouldn't be a
backtrace printed (which abort does).
A lot of the subprocess overloads re-implement needlessly functionality which
the underlying exec() C APIs provide, using those apis instead of
re-implementing all the variations would be a much better model.
Mesos doesn't use / need most of the subprocess overloads that exist. A lot of
the usage patterns probably could / should be removed.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
