You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by an...@apache.org on 2018/02/21 22:57:15 UTC
[4/5] mesos git commit: Windows: Updated
`internal::process:createChildProcess`.
Windows: Updated `internal::process:createChildProcess`.
The interface of `internal::windows:create_process` was changed to
accept a `std::array<os::WindowsFD, 3>` instead of a `std::tuple`.
Furthermore, the error handling in `subprocess` was incorrect. We only
need to check the success of `createChildProcess`; we do not need to
check if `pid == -1` on Windows. We also now return the actual error
from `create_process` if it errored, instead of the whole
`Try<process_data>`.
The TODO about closing the child-ends of the file descriptors was
resolved. We now close these in `createChildProcess`, immediately after
`create_process`. This means we only have to do it once.
Review: https://reviews.apache.org/r/65469
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b1e9d423
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b1e9d423
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b1e9d423
Branch: refs/heads/master
Commit: b1e9d4232e5931bcd0e1cd568a280a07828ff205
Parents: f946e53
Author: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Authored: Thu Feb 8 10:41:19 2018 -0800
Committer: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Committed: Wed Feb 21 14:00:11 2018 -0800
----------------------------------------------------------------------
3rdparty/libprocess/src/subprocess.cpp | 21 +++------------
3rdparty/libprocess/src/subprocess_windows.hpp | 29 ++++++++++++++-------
2 files changed, 24 insertions(+), 26 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/b1e9d423/3rdparty/libprocess/src/subprocess.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/subprocess.cpp b/3rdparty/libprocess/src/subprocess.cpp
index 785e2e1..8983263 100644
--- a/3rdparty/libprocess/src/subprocess.cpp
+++ b/3rdparty/libprocess/src/subprocess.cpp
@@ -426,26 +426,13 @@ Try<Subprocess> subprocess(
stderrfds);
if (process_data.isError()) {
- process::internal::close(stdinfds, stdoutfds, stderrfds);
- return Error(
- "Could not launch child process: " + process_data.error());
+ // NOTE: `createChildProcess` either succeeds entirely or returns an
+ // `Error`. There is no need to check the value of the PID.
+ return Error(process_data.error());
}
- if (process_data.get().pid == -1) {
- // Save the errno as 'close' below might overwrite it.
- ErrnoError error("Failed to clone");
- process::internal::close(stdinfds, stdoutfds, stderrfds);
- return error;
- }
-
- // Close the child-ends of the file descriptors that are created
- // by this function.
- // TODO(jieyu): We should move the closing of FDs to
- // 'createChildProcess' to be consistent with the posix path.
- internal::close({stdinfds.read, stdoutfds.write, stderrfds.write});
-
process.data->process_data = process_data.get();
- process.data->pid = process_data.get().pid;
+ process.data->pid = process_data->pid;
#endif // __WINDOWS__
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/b1e9d423/3rdparty/libprocess/src/subprocess_windows.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/subprocess_windows.hpp b/3rdparty/libprocess/src/subprocess_windows.hpp
index 0183bb4..4cc5675 100644
--- a/3rdparty/libprocess/src/subprocess_windows.hpp
+++ b/3rdparty/libprocess/src/subprocess_windows.hpp
@@ -15,8 +15,8 @@
#include <signal.h>
+#include <array>
#include <string>
-#include <tuple>
#include <glog/logging.h>
@@ -53,41 +53,52 @@ inline Try<::internal::windows::ProcessData> createChildProcess(
const OutputFileDescriptors stdoutfds,
const OutputFileDescriptors stderrfds)
{
+ const std::array<os::WindowsFD, 3> fds{
+ stdinfds.read, stdoutfds.write, stderrfds.write};
+
Try<::internal::windows::ProcessData> process_data =
::internal::windows::create_process(
path,
argv,
environment,
true, // Create suspended.
- std::make_tuple(stdinfds.read, stdoutfds.write, stderrfds.write));
+ fds);
+
+ // Close the child-ends of the file descriptors that are created
+ // by this function.
+ foreach (const os::WindowsFD& fd, fds) {
+ if (fd >= 0) {
+ os::close(fd);
+ }
+ }
if (process_data.isError()) {
- return process_data;
+ return Error(process_data.error());
}
// Run the parent hooks.
- const pid_t pid = process_data.get().pid;
+ const pid_t pid = process_data->pid;
foreach (const Subprocess::ParentHook& hook, parent_hooks) {
- Try<Nothing> parentSetup = hook.parent_setup(pid);
+ Try<Nothing> parent_setup = hook.parent_setup(pid);
// If the hook callback fails, we shouldn't proceed with the
// execution and hence the child process should be killed.
- if (parentSetup.isError()) {
+ if (parent_setup.isError()) {
// Attempt to kill the process. Since it is still in suspended state, we
// do not need to kill any descendents. We also can't use `os::kill_job`
// because this process is not in a Job Object unless one of the parent
// hooks added it.
- ::TerminateProcess(process_data.get().process_handle.get_handle(), 1);
+ ::TerminateProcess(process_data->process_handle.get_handle(), 1);
return Error(
"Failed to execute Parent Hook in child '" + stringify(pid) +
"' with command '" + stringify(argv) + "': " +
- parentSetup.error());
+ parent_setup.error());
}
}
// Start child process.
- if (::ResumeThread(process_data.get().thread_handle.get_handle()) == -1) {
+ if (::ResumeThread(process_data->thread_handle.get_handle()) == -1) {
return WindowsError(
"Failed to resume child process with command '" +
stringify(argv) + "'");