Repository: mesos Updated Branches: refs/heads/master d4a903a4a -> 8e2d6d296
Added `SubprocessTest.PipeLargeOutput`. This new test checks exercises our `Subprocess::PIPE()` logic more thoroughly by specifically piping enough data such that a single memory page is insufficient to hold it. This is especially important on Windows because the OS-allocated buffer is the size of one memory page. On Windows, it also tests that the expected end-of-file condition, `ERROR_BROKEN_PIPE`, is set. We also fix another use of `os::WindowsFD` in place of `int_fd`, the public name of the type; added an important note about why we hold onto the process's handle on Windows; pass the file descriptor structs by const-ref instead of value; and finally check the return value `os::close()` in `createChildProcess`. Review: https://reviews.apache.org/r/66892 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/8e2d6d29 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/8e2d6d29 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/8e2d6d29 Branch: refs/heads/master Commit: 8e2d6d296e055b5617d3ea63dc061ad29c66a1da Parents: 2dcbdeb Author: Andrew Schwartzmeyer <[email protected]> Authored: Tue May 1 14:15:21 2018 -0700 Committer: Andrew Schwartzmeyer <[email protected]> Committed: Tue May 1 18:36:04 2018 -0700 ---------------------------------------------------------------------- 3rdparty/libprocess/src/subprocess.cpp | 3 ++ 3rdparty/libprocess/src/subprocess_windows.hpp | 17 +++--- .../libprocess/src/tests/subprocess_tests.cpp | 57 ++++++++++++++++++++ 3 files changed, 70 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/8e2d6d29/3rdparty/libprocess/src/subprocess.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/subprocess.cpp b/3rdparty/libprocess/src/subprocess.cpp index 42e06da..d7a7253 100644 --- a/3rdparty/libprocess/src/subprocess.cpp +++ b/3rdparty/libprocess/src/subprocess.cpp @@ -438,6 +438,9 @@ Try<Subprocess> subprocess( return Error(process_data.error()); } + // NOTE: We specifically tie the lifetime of the child process to + // the `Subprocess` object by saving the handles here so that + // there is zero chance of the `pid` getting reused. process.data->process_data = process_data.get(); process.data->pid = process_data->pid; #endif // __WINDOWS__ http://git-wip-us.apache.org/repos/asf/mesos/blob/8e2d6d29/3rdparty/libprocess/src/subprocess_windows.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/subprocess_windows.hpp b/3rdparty/libprocess/src/subprocess_windows.hpp index 4cc5675..c7ed0ad 100644 --- a/3rdparty/libprocess/src/subprocess_windows.hpp +++ b/3rdparty/libprocess/src/subprocess_windows.hpp @@ -49,11 +49,11 @@ inline Try<::internal::windows::ProcessData> createChildProcess( const std::vector<std::string>& argv, const Option<std::map<std::string, std::string>>& environment, const std::vector<Subprocess::ParentHook>& parent_hooks, - const InputFileDescriptors stdinfds, - const OutputFileDescriptors stdoutfds, - const OutputFileDescriptors stderrfds) + const InputFileDescriptors& stdinfds, + const OutputFileDescriptors& stdoutfds, + const OutputFileDescriptors& stderrfds) { - const std::array<os::WindowsFD, 3> fds{ + const std::array<int_fd, 3> fds{ stdinfds.read, stdoutfds.write, stderrfds.write}; Try<::internal::windows::ProcessData> process_data = @@ -66,9 +66,12 @@ inline Try<::internal::windows::ProcessData> createChildProcess( // 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); + foreach (const int_fd& fd, fds) { + if (fd.is_valid()) { + Try<Nothing> result = os::close(fd); + if (result.isError()) { + return Error(result.error()); + } } } http://git-wip-us.apache.org/repos/asf/mesos/blob/8e2d6d29/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 4395e8c..269918e 100644 --- a/3rdparty/libprocess/src/tests/subprocess_tests.cpp +++ b/3rdparty/libprocess/src/tests/subprocess_tests.cpp @@ -353,6 +353,63 @@ TEST_F(SubprocessTest, PipeOutput) } +// This test checks that we can open a subprocess, have it write a +// substantial amount of data (two memory pages) to a pipe held by the +// parent process (this test) without hanging, and then check that the +// process exits and is reaped correctly. +TEST_F(SubprocessTest, PipeLargeOutput) +{ + const string output(2 * os::pagesize(), 'c'); + const string outfile = path::join(sandbox.get(), "out.txt"); + ASSERT_SOME(os::write(outfile, output)); + + Try<Subprocess> s = subprocess( +#ifdef __WINDOWS__ + "type " + outfile, +#else + "cat " + outfile, +#endif // __WINDOWS__ + Subprocess::FD(STDIN_FILENO), + Subprocess::PIPE(), + Subprocess::FD(STDERR_FILENO)); + + ASSERT_SOME(s); + ASSERT_SOME(s->out()); + +#ifdef __WINDOWS__ + ::SetLastError(0); +#endif // __WINDOWS__ + + AWAIT_EXPECT_EQ(output, io::read(s->out().get())); + +#ifdef __WINDOWS__ + // NOTE: On Windows, this is the end-of-file condition when reading + // from a pipe being written to by a child process. When it finishes + // writing, the last read will successfully return all the data, and + // the Windows error will be set to this. + EXPECT_EQ(::GetLastError(), ERROR_BROKEN_PIPE); +#endif // __WINDOWS__ + + // Advance time until the internal reaper reaps the subprocess. + Clock::pause(); + while (s->status().isPending()) { + Clock::advance(MAX_REAP_INTERVAL()); + Clock::settle(); + } + Clock::resume(); + + // NOTE: Because we are specifically writing more data (two pages) + // than can be held by the OS-allocated buffer, (on Windows this is + // one page), we cannot reap the process before reading because it + // will not exit until it has written all its data. It can only + // successfully write all its data if we read it in the parent + // process, otherwise the buffer fills up, and the OS makes the + // process wait until the buffer is emptied. + + AWAIT_EXPECT_WEXITSTATUS_EQ(0, s->status()); +} + + TEST_F(SubprocessTest, PipeInput) { Try<Subprocess> s = subprocess(
