From d32650bb9ee087815e047c2100fcf61fc7114f99 Mon Sep 17 00:00:00 2001 From: Jamie Madill Date: Thu, 21 Nov 2019 12:52:34 -0500 Subject: [PATCH] Posix: Fix async LaunchProcess. The prior method was only checking for a PID's existance. We should also check for zombie process states. This allows our child process launcher to correctly determine when a process is done on Linux and other Posix systems. Also starts the test timer on Posix LaunchProcess implementations. Also removes the need to end command line arg lists with nullptr. Also removes some Fuchsia-specific preprocessor checks. Also adds a regression test. Bug: angleproject:3162 Change-Id: I5725a8a0e8c5151f2d3e06df0a36dd64c0e0ee69 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/1928873 Commit-Queue: Jamie Madill Reviewed-by: Shahbaz Youssefi --- util/posix/test_utils_posix.cpp | 186 +++++++++++++++++++++----------- util/test_utils_unittest.cpp | 67 +++++++++--- 2 files changed, 174 insertions(+), 79 deletions(-) diff --git a/util/posix/test_utils_posix.cpp b/util/posix/test_utils_posix.cpp index 82c5a7079..b1ec61ce9 100644 --- a/util/posix/test_utils_posix.cpp +++ b/util/posix/test_utils_posix.cpp @@ -8,23 +8,25 @@ #include "util/test_utils.h" +#include #include +#include #include #include +#include +#include +#include #include #include #include #include +#include #include "common/debug.h" #include "common/platform.h" #if !defined(ANGLE_PLATFORM_FUCHSIA) -# include # include -# include -# include -# include #endif namespace angle @@ -56,28 +58,33 @@ struct ScopedPipe }; }; +bool ReadFromFile(int fd, std::string *out) +{ + char buffer[256]; + ssize_t bytesRead = read(fd, buffer, sizeof(buffer)); + + // If interrupted, retry. + if (bytesRead < 0 && errno == EINTR) + { + return true; + } + + // If failed, or nothing to read, we are done. + if (bytesRead <= 0) + { + return false; + } + + out->append(buffer, bytesRead); + return true; +} + void ReadEntireFile(int fd, std::string *out) { - out->clear(); - while (true) { - char buffer[256]; - ssize_t bytesRead = read(fd, buffer, sizeof(buffer)); - - // If interrupted, retry. - if (bytesRead < 0 && errno == EINTR) - { - continue; - } - - // If failed, or nothing to read, we are done. - if (bytesRead <= 0) - { + if (!ReadFromFile(fd, out)) break; - } - - out->append(buffer, bytesRead); } } @@ -88,24 +95,37 @@ class PosixProcess : public Process bool captureStdOut, bool captureStdErr) { -#if defined(ANGLE_PLATFORM_FUCHSIA) - ANGLE_UNUSED_VARIABLE(ReadEntireFile); - ANGLE_UNUSED_VARIABLE(mExitCode); - ANGLE_UNUSED_VARIABLE(mPID); -#else - if (commandLineArgs.empty() || commandLineArgs.back() != nullptr) + if (commandLineArgs.empty()) { return; } // Create pipes for stdout and stderr. - if (captureStdOut && pipe(mStdoutPipe.fds) != 0) + if (captureStdOut) { - return; + if (pipe(mStdoutPipe.fds) != 0) + { + std::cerr << "Error calling pipe: " << errno << "\n"; + return; + } + if (fcntl(mStdoutPipe.fds[0], F_SETFL, O_NONBLOCK) == -1) + { + std::cerr << "Error calling fcntl: " << errno << "\n"; + return; + } } - if (captureStdErr && pipe(mStderrPipe.fds) != 0) + if (captureStdErr) { - return; + if (pipe(mStderrPipe.fds) != 0) + { + std::cerr << "Error calling pipe: " << errno << "\n"; + return; + } + if (fcntl(mStderrPipe.fds[0], F_SETFL, O_NONBLOCK) == -1) + { + std::cerr << "Error calling fcntl: " << errno << "\n"; + return; + } } mPID = fork(); @@ -115,6 +135,7 @@ class PosixProcess : public Process } mStarted = true; + mTimer.start(); if (mPID == 0) { @@ -127,6 +148,7 @@ class PosixProcess : public Process { _exit(errno); } + mStdoutPipe.closeEndPoint(1); } if (captureStdErr) { @@ -134,6 +156,7 @@ class PosixProcess : public Process { _exit(errno); } + mStderrPipe.closeEndPoint(1); } // Execute the application, which doesn't return unless failed. Note: execv takes argv @@ -149,11 +172,20 @@ class PosixProcess : public Process // functions do not modify either the array of pointers or the characters to which the // function points, but this would disallow existing correct code. Instead, only the // array of pointers is noted as constant. - execv(commandLineArgs[0], const_cast(commandLineArgs.data())); + std::vector args; + for (const char *arg : commandLineArgs) + { + args.push_back(const_cast(arg)); + } + args.push_back(nullptr); + + execv(commandLineArgs[0], args.data()); + std::cerr << "Error calling evecv: " << errno; _exit(errno); } // Parent continues execution. -#endif // defined(ANGLE_PLATFORM_FUCHSIA) + mStdoutPipe.closeEndPoint(1); + mStderrPipe.closeEndPoint(1); } ~PosixProcess() override {} @@ -167,41 +199,17 @@ class PosixProcess : public Process return false; } -#if defined(ANGLE_PLATFORM_FUCHSIA) - return false; -#else - // Close the write end of the pipes, so EOF can be generated when child exits. - // Then read back the output of the child. - if (mStdoutPipe.valid()) + if (mFinished) { - mStdoutPipe.closeEndPoint(1); - ReadEntireFile(mStdoutPipe.fds[0], &mStdout); - } - if (mStderrPipe.valid()) - { - mStderrPipe.closeEndPoint(1); - ReadEntireFile(mStderrPipe.fds[0], &mStderr); + return true; } - // Cleanup the child. - int status = 0; - do + while (!finished()) { - pid_t changedPid = waitpid(mPID, &status, 0); - if (changedPid < 0 && errno == EINTR) - { - continue; - } - if (changedPid < 0) - { - return false; - } - } while (!WIFEXITED(status) && !WIFSIGNALED(status)); + angle::Sleep(1); + } - // Retrieve the error code. - mExitCode = WEXITSTATUS(status); return true; -#endif // defined(ANGLE_PLATFORM_FUCHSIA) } bool finished() override @@ -211,10 +219,43 @@ class PosixProcess : public Process return false; } - return (::kill(mPID, 0) != 0); + if (mFinished) + { + return true; + } + + int status = 0; + pid_t returnedPID = ::waitpid(mPID, &status, WNOHANG); + + if (returnedPID == -1 && errno != ECHILD) + { + std::cerr << "Error calling waitpid: " << ::strerror(errno) << "\n"; + return true; + } + + if (returnedPID == mPID) + { + mFinished = true; + mTimer.stop(); + readPipes(); + mExitCode = WEXITSTATUS(status); + return true; + } + + if (mStdoutPipe.valid()) + { + ReadFromFile(mStdoutPipe.fds[0], &mStdout); + } + + if (mStderrPipe.valid()) + { + ReadFromFile(mStderrPipe.fds[0], &mStderr); + } + + return false; } - int getExitCode() override { return 0; } + int getExitCode() override { return mExitCode; } bool kill() override { @@ -232,7 +273,22 @@ class PosixProcess : public Process } private: - bool mStarted = false; + void readPipes() + { + // Close the write end of the pipes, so EOF can be generated when child exits. + // Then read back the output of the child. + if (mStdoutPipe.valid()) + { + ReadEntireFile(mStdoutPipe.fds[0], &mStdout); + } + if (mStderrPipe.valid()) + { + ReadEntireFile(mStderrPipe.fds[0], &mStderr); + } + } + + bool mStarted = false; + bool mFinished = false; ScopedPipe mStdoutPipe; ScopedPipe mStderrPipe; int mExitCode = 0; diff --git a/util/test_utils_unittest.cpp b/util/test_utils_unittest.cpp index 380d43fc3..9b02a5acc 100644 --- a/util/test_utils_unittest.cpp +++ b/util/test_utils_unittest.cpp @@ -126,26 +126,28 @@ TEST(TestUtils, MAYBE_CreateAndDeleteFileInTempDir) EXPECT_TRUE(angle::DeleteFile(path)); } -// Test running an external application and receiving its output -TEST(TestUtils, RunApp) -{ +// TODO: android support. http://anglebug.com/3125 #if defined(ANGLE_PLATFORM_ANDROID) - // TODO: android support. http://anglebug.com/3125 - return; -#endif - -#if defined(ANGLE_PLATFORM_FUCHSIA) - // TODO: fuchsia support. http://anglebug.com/3161 - return; -#endif +# define MAYBE_RunApp DISABLED_RunApp +# define MAYBE_RunAppAsync DISABLED_RunAppAsync +// TODO: fuchsia support. http://anglebug.com/3161 +#elif defined(ANGLE_PLATFORM_FUCHSIA) +# define MAYBE_RunApp DISABLED_RunApp +# define MAYBE_RunAppAsync DISABLED_RunAppAsync +#else +# define MAYBE_RunApp RunApp +# define MAYBE_RunAppAsync RunAppAsync +#endif // defined(ANGLE_PLATFORM_ANDROID) +// Test running an external application and receiving its output +TEST(TestUtils, MAYBE_RunApp) +{ std::string executablePath = GetExecutableDirectory(); EXPECT_NE(executablePath, ""); executablePath += "/"; executablePath += kRunAppHelperExecutable; - std::vector args = {executablePath.c_str(), kRunAppTestArg1, kRunAppTestArg2, - nullptr}; + std::vector args = {executablePath.c_str(), kRunAppTestArg1, kRunAppTestArg2}; // Test that the application can be executed. { @@ -154,12 +156,13 @@ TEST(TestUtils, RunApp) EXPECT_TRUE(process->finish()); EXPECT_TRUE(process->finished()); + EXPECT_GT(process->getElapsedTimeSeconds(), 0.0); EXPECT_EQ(kRunAppTestStdout, NormalizeNewLines(process->getStdout())); EXPECT_EQ(kRunAppTestStderr, NormalizeNewLines(process->getStderr())); EXPECT_EQ(EXIT_SUCCESS, process->getExitCode()); } - // Test that environment variables reach the cild. + // Test that environment variables reach the child. { bool setEnvDone = SetEnvironmentVar(kRunAppTestEnvVarName, kRunAppTestEnvVarValue); EXPECT_TRUE(setEnvDone); @@ -168,9 +171,45 @@ TEST(TestUtils, RunApp) EXPECT_TRUE(process->started()); EXPECT_TRUE(process->finish()); + EXPECT_GT(process->getElapsedTimeSeconds(), 0.0); EXPECT_EQ("", process->getStdout()); EXPECT_EQ(kRunAppTestEnvVarValue, NormalizeNewLines(process->getStderr())); EXPECT_EQ(EXIT_SUCCESS, process->getExitCode()); + + // Unset environment var. + SetEnvironmentVar(kRunAppTestEnvVarName, ""); + } +} + +// Test running an external application and receiving its output asynchronously. +TEST(TestUtils, MAYBE_RunAppAsync) +{ + std::string executablePath = GetExecutableDirectory(); + EXPECT_NE(executablePath, ""); + executablePath += "/"; + executablePath += kRunAppHelperExecutable; + + std::vector args = {executablePath.c_str(), kRunAppTestArg1, kRunAppTestArg2}; + + // Test that the application can be executed. + { + ProcessHandle process(args, true, true); + EXPECT_TRUE(process->started()); + + constexpr double kTimeout = 3.0; + + Timer timer; + timer.start(); + while (!process->finished() && timer.getElapsedTime() < kTimeout) + { + angle::Sleep(1); + } + + EXPECT_TRUE(process->finished()); + EXPECT_GT(process->getElapsedTimeSeconds(), 0.0); + EXPECT_EQ(kRunAppTestStdout, NormalizeNewLines(process->getStdout())); + EXPECT_EQ(kRunAppTestStderr, NormalizeNewLines(process->getStderr())); + EXPECT_EQ(EXIT_SUCCESS, process->getExitCode()); } }