You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by jo...@apache.org on 2016/04/22 23:09:28 UTC
[1/2] mesos git commit: Stout: [2/2] Transitioned reap.cpp to
`os::waitpid`.
Repository: mesos
Updated Branches:
refs/heads/master 60dcd72c3 -> 97bc9cab1
Stout: [2/2] Transitioned reap.cpp to `os::waitpid`.
Review: https://reviews.apache.org/r/46341/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/97bc9cab
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/97bc9cab
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/97bc9cab
Branch: refs/heads/master
Commit: 97bc9cab168441c25f8ea6943edcc628e274fa1c
Parents: 66ee32a
Author: Alex Clemmer <cl...@gmail.com>
Authored: Fri Apr 22 12:50:11 2016 -0700
Committer: Joris Van Remoortere <jo...@gmail.com>
Committed: Fri Apr 22 14:09:10 2016 -0700
----------------------------------------------------------------------
3rdparty/libprocess/src/reap.cpp | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/97bc9cab/3rdparty/libprocess/src/reap.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/reap.cpp b/3rdparty/libprocess/src/reap.cpp
index 1103868..69b33eb 100644
--- a/3rdparty/libprocess/src/reap.cpp
+++ b/3rdparty/libprocess/src/reap.cpp
@@ -13,7 +13,9 @@
#include <glog/logging.h>
#include <sys/types.h>
+#ifndef __WINDOWS__
#include <sys/wait.h>
+#endif
#include <process/delay.hpp>
#include <process/future.hpp>
@@ -93,7 +95,8 @@ protected:
// zombie; it will be reaped by us on the next loop.
foreach (pid_t pid, promises.keys()) {
int status;
- if (waitpid(pid, &status, WNOHANG) > 0) {
+ Result<pid_t> child_pid = os::waitpid(pid, &status, WNOHANG);
+ if (child_pid.isSome()) {
// We have reaped a child.
notify(pid, status);
} else if (!os::exists(pid)) {
[2/2] mesos git commit: Stout: [1/2] Implement `os::waitpid`.
Posted by jo...@apache.org.
Stout: [1/2] Implement `os::waitpid`.
Review: https://reviews.apache.org/r/46340/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/66ee32a8
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/66ee32a8
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/66ee32a8
Branch: refs/heads/master
Commit: 66ee32a839976ab804040f9bfd8b8e1029d8abc6
Parents: 60dcd72
Author: Alex Clemmer <cl...@gmail.com>
Authored: Fri Apr 22 12:49:58 2016 -0700
Committer: Joris Van Remoortere <jo...@gmail.com>
Committed: Fri Apr 22 14:09:10 2016 -0700
----------------------------------------------------------------------
.../3rdparty/stout/include/stout/posix/os.hpp | 47 +++++++
.../3rdparty/stout/include/stout/windows.hpp | 13 +-
.../3rdparty/stout/include/stout/windows/os.hpp | 126 +++++++++++++++++++
3 files changed, 184 insertions(+), 2 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/66ee32a8/3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp b/3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp
index 700d704..0f71f66 100644
--- a/3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp
+++ b/3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp
@@ -97,6 +97,53 @@ using ::hstrerror;
inline Try<Nothing> utime(const std::string&);
inline Try<std::list<Process>> processes();
+
+// Suspends execution of the calling process until a child specified by `pid`
+// has changed state. Unlike the POSIX standard function `::waitpid`, this
+// function does not use -1 and 0 to signify errors and nonblocking return.
+// Instead, we return `Result<pid_t>`:
+// * In case of error, we return `Error` rather than -1. For example, we
+// would return an `Error` in case of `EINVAL`.
+// * In case of nonblocking return, we return `None` rather than 0. For
+// example, if we pass `WNOHANG` in the `options`, we would expect 0 to be
+// returned in the case that children specified by `pid` exist, but have
+// not changed state yet. In this case we return `None` instead.
+//
+// NOTE: There are important differences between the POSIX and Windows
+// implementations of this function:
+// * On POSIX, `pid_t` is a signed number, but on Windows, PIDs are `DWORD`,
+// which is `unsigned long`. Thus, if we use `DWORD` to represent the `pid`
+// argument, we would not be able to pass -1 as the `pid`.
+// * Since it is important to be able to detect -1 has been passed to
+// `os::waitpid`, as a matter of practicality, we choose to:
+// (1) Use `long` to represent the `pid` argument.
+// (2) Disable using any value <= 0 for `pid` on Windows.
+// * This decision is pragmatic. The reasoning is:
+// (1) The Windows code paths call `os::waitpid` in only a handful of
+// places, and in none of these conditions do we need `-1` as a value.
+// (2) Since PIDs virtually never take on values outside the range of
+// vanilla signed `long` it is likely that an accidental conversion
+// will never happen.
+// (3) Even though it is not formalized in the C specification, the
+// implementation of `long` on the vast majority of production servers
+// is 2's complement, so we expect that when we accidentally do
+// implicitly convert from `unsigned long` to `long`, we will "wrap
+// around" to negative values. And since we've disabled the negative
+// `pid` in the Windows implementation, we should error out.
+inline Result<pid_t> waitpid(pid_t pid, int* status, int options)
+{
+ const pid_t child_pid = ::waitpid(pid, status, options);
+
+ if (child_pid == 0) {
+ return None();
+ } else if (child_pid < 0) {
+ return ErrnoError("os::waitpid: Call to `waitpid` failed");
+ } else {
+ return child_pid;
+ }
+}
+
+
// Sets the value associated with the specified key in the set of
// environment variables.
inline void setenv(const std::string& key,
http://git-wip-us.apache.org/repos/asf/mesos/blob/66ee32a8/3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp b/3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp
index 3828c53..c2d1dc1 100644
--- a/3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp
+++ b/3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp
@@ -402,11 +402,20 @@ decltype(_access(fileName, accessMode))
// `os::system` returns -1 if the processor cannot be started
// therefore any return value indicates the process has been started
#ifndef WIFEXITED
- #define WIFEXITED(x) ((x) != -1)
+#define WIFEXITED(x) ((x) != -1)
#endif // WIFWXITED
+// Returns the exit status of the child.
#ifndef WEXITSTATUS
- #define WEXITSTATUS(x) (x)
+#define WEXITSTATUS(x) (x & 0xFF)
#endif // WEXITSTATUS
+#define SIGPIPE 100
+
+// Specifies that `::waitpid` should return immediately rather than blocking
+// and waiting for child to notify of state change.
+#ifndef WNOHANG
+#define WNOHANG 1
+#endif // WNOHANG
+
#endif // __STOUT_WINDOWS_HPP__
http://git-wip-us.apache.org/repos/asf/mesos/blob/66ee32a8/3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp b/3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp
index 5281a2e..261768e 100644
--- a/3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp
+++ b/3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp
@@ -205,6 +205,132 @@ inline void unsetenv(const std::string& key)
}
+// Suspends execution of the calling process until a child specified by `pid`
+// has changed state. Unlike the POSIX standard function `::waitpid`, this
+// function does not use -1 and 0 to signify errors and nonblocking return.
+// Instead, we return `Result<pid_t>`:
+// * In case of error, we return `Error` rather than -1. For example, we
+// would return an `Error` in case of `EINVAL`.
+// * In case of nonblocking return, we return `None` rather than 0. For
+// example, if we pass `WNOHANG` in the `options`, we would expect 0 to be
+// returned in the case that children specified by `pid` exist, but have
+// not changed state yet. In this case we return `None` instead.
+//
+// NOTE: There are important differences between the POSIX and Windows
+// implementations of this function:
+// * On POSIX, `pid_t` is a signed number, but on Windows, PIDs are `DWORD`,
+// which is `unsigned long`. Thus, if we use `DWORD` to represent the `pid`
+// argument, passing -1 as the `pid` would (on most modern servers)
+// silently convert to a really large `pid`. This is undesirable.
+// * Since it is important to be able to detect -1 has been passed to
+// `os::waitpid`, as a matter of practicality, we choose to:
+// (1) Use `long` to represent the `pid` argument.
+// (2) Disable using any value <= 0 for `pid` on Windows.
+// * This decision is pragmatic. The reasoning is:
+// (1) The Windows code paths call `os::waitpid` in only a handful of
+// places, and in none of these conditions do we need `-1` as a value.
+// (2) Since PIDs virtually never take on values outside the range of
+// vanilla signed `long` it is likely that an accidental conversion
+// will never happen.
+// (3) Even though it is not formalized in the C specification, the
+// implementation of `long` on the vast majority of production servers
+// is 2's complement, so we expect that when we accidentally do
+// implicitly convert from `unsigned long` to `long`, we will "wrap
+// around" to negative values. And since we've disabled the negative
+// `pid` in the Windows implementation, we should error out.
+// * Finally, on Windows, we currently do not check that the process we are
+// attempting to await is a child process.
+inline Result<pid_t> waitpid(long pid, int* status, int options)
+{
+ const bool wait_for_child = (options & WNOHANG) == 0;
+
+ // NOTE: Windows does not implement pids <= 0.
+ if (pid <= 0) {
+ errno = ENOSYS;
+ return ErrnoError(
+ "os::waitpid: Value of pid is '" + stringify(pid) +
+ "'; the Windows implementation currently does not allow values <= 0");
+ } else if (options != 0 && options != WNOHANG) {
+ // NOTE: We only support `options == 0` or `options == WNOHANG`. On Windows
+ // no flags other than `WNOHANG` are supported.
+ errno = ENOSYS;
+ return ErrnoError(
+ "os::waitpid: Only flag `WNOHANG` is implemented on Windows");
+ }
+
+ // TODO(hausdorff): Check that `pid` is one of the child processes. If not,
+ // set `errno` to `ECHILD` and return -1.
+
+ // Open the child process as a safe `SharedHandle`.
+ const HANDLE process = ::OpenProcess(
+ PROCESS_QUERY_INFORMATION | SYNCHRONIZE,
+ FALSE,
+ static_cast<DWORD>(pid));
+
+ if (process == NULL) {
+ return WindowsError("os::waitpid: Failed to open process for pid '" +
+ stringify(pid) + "'");
+ }
+
+ SharedHandle scoped_process(process, ::CloseHandle);
+
+ // If `WNOHANG` flag is set, don't wait. Otherwise, wait for child to
+ // terminate.
+ const DWORD wait_time = wait_for_child ? INFINITE : 0;
+ const DWORD wait_results = ::WaitForSingleObject(
+ scoped_process.get(),
+ wait_time);
+
+ // Verify our wait exited correctly.
+ const bool state_signaled = wait_results == WAIT_OBJECT_0;
+ if (options == 0 && !state_signaled) {
+ // If `WNOHANG` is not set, then we should have stopped waiting only for a
+ // state change in `scoped_process`.
+ errno = ECHILD;
+ return WindowsError(
+ "os::waitpid: Failed to wait for pid '" + stringify(pid) +
+ "'. `::WaitForSingleObject` should have waited for child process to " +
+ "exit, but returned code '" + stringify(wait_results) +
+ "' instead");
+ } else if (wait_for_child && !state_signaled &&
+ wait_results != WAIT_TIMEOUT) {
+ // If `WNOHANG` is set, then a successful wait should report either a
+ // timeout (since we set the time to wait to `0`), or a successful state
+ // change of `scoped_process`. Anything else is an error.
+ errno = ECHILD;
+ return WindowsError(
+ "os::waitpid: Failed to wait for pid '" + stringify(pid) +
+ "'. `ENOHANG` flag was passed in, so `::WaitForSingleObject` should " +
+ "have either returned `WAIT_OBJECT_0` or `WAIT_TIMEOUT` (the " +
+ "timeout was set to 0, because we are not waiting for the child), " +
+ "but instead returned code '" + stringify(wait_results) + "'");
+ }
+
+ if (!wait_for_child && wait_results == WAIT_TIMEOUT) {
+ // Success. `ENOHANG` was set and we got a timeout, so return `None` (POSIX
+ // `::waitpid` would return 0 here).
+ return None();
+ }
+
+ // Attempt to retrieve exit code from child process. Store that exit code in
+ // the `status` variable if it's `NULL`.
+ DWORD child_exit_code = 0;
+ if (!::GetExitCodeProcess(scoped_process.get(), &child_exit_code)) {
+ errno = ECHILD;
+ return WindowsError(
+ "os::waitpid: Successfully waited on child process with pid '" +
+ std::to_string(pid) + "', but could not retrieve exit code");
+ }
+
+ if (status != NULL) {
+ *status = child_exit_code;
+ }
+
+ // Success. Return pid of the child process for which the status is reported.
+ return pid;
+}
+
+
inline std::string hstrerror(int err)
{
char buffer[1024];