From ac345f86af827c42216d3a6bd0fd6482afb8c7f0 Mon Sep 17 00:00:00 2001 From: davidmatson Date: Wed, 29 Jun 2022 17:42:15 -0700 Subject: [PATCH] Use anonymous pipes for stdout/stderr redirection. Closes #2444. Avoid the overhead of creating and deleting temporary files. Anonymous pipes have a limited buffer and require an active reader to ensure the writer does not become blocked. Use a separate thread to ensure the buffer does not get stuck full. --- .../internal/catch_compiler_capabilities.hpp | 4 + src/catch2/internal/catch_output_redirect.cpp | 232 +++++++++++++----- src/catch2/internal/catch_output_redirect.hpp | 105 +++++--- 3 files changed, 249 insertions(+), 92 deletions(-) diff --git a/src/catch2/internal/catch_compiler_capabilities.hpp b/src/catch2/internal/catch_compiler_capabilities.hpp index 00280277..32612a0c 100644 --- a/src/catch2/internal/catch_compiler_capabilities.hpp +++ b/src/catch2/internal/catch_compiler_capabilities.hpp @@ -37,6 +37,10 @@ # define CATCH_CPP17_OR_GREATER # endif +# if (__cplusplus >= 202002L) || (defined(_MSVC_LANG) && _MSVC_LANG >= 202002L) +# define CATCH_CPP20_OR_GREATER +# endif + #endif // Only GCC compiler should be used in this block, so other compilers trying to diff --git a/src/catch2/internal/catch_output_redirect.cpp b/src/catch2/internal/catch_output_redirect.cpp index ae060141..83bcdcd2 100644 --- a/src/catch2/internal/catch_output_redirect.cpp +++ b/src/catch2/internal/catch_output_redirect.cpp @@ -14,13 +14,16 @@ #include #if defined(CATCH_CONFIG_NEW_CAPTURE) + #include #if defined(_MSC_VER) - #include //_dup and _dup2 + #include // _O_TEXT + #include // _close, _dup, _dup2, _fileno, _pipe and _read + #define close _close #define dup _dup #define dup2 _dup2 #define fileno _fileno #else - #include // dup and dup2 + #include // close, dup, dup2, fileno, pipe and read #endif #endif @@ -60,78 +63,194 @@ namespace Catch { #if defined(CATCH_CONFIG_NEW_CAPTURE) -#if defined(_MSC_VER) - TempFile::TempFile() { - if (tmpnam_s(m_buffer)) { - CATCH_RUNTIME_ERROR("Could not get a temp filename"); - } - if (fopen_s(&m_file, m_buffer, "w+")) { - char buffer[100]; - if (strerror_s(buffer, errno)) { - CATCH_RUNTIME_ERROR("Could not translate errno to a string"); - } - CATCH_RUNTIME_ERROR("Could not open the temp file: '" << m_buffer << "' because: " << buffer); - } +inline void close_or_throw(int descriptor) +{ + if (close(descriptor)) + { + throw std::system_error{ errno, std::generic_category() }; } +} + +inline int dup_or_throw(int descriptor) +{ + int result{ dup(descriptor) }; + + if (result == -1) + { + throw std::system_error{ errno, std::generic_category() }; + } + + return result; +} + +inline int dup2_or_throw(int sourceDescriptor, int destinationDescriptor) +{ + int result{ dup2(sourceDescriptor, destinationDescriptor) }; + + if (result == -1) + { + throw std::system_error{ errno, std::generic_category() }; + } + + return result; +} + +inline int fileno_or_throw(std::FILE* file) +{ + int result{ fileno(file) }; + + if (result == -1) + { + throw std::system_error{ errno, std::generic_category() }; + } + + return result; +} + +inline void pipe_or_throw(int descriptors[2]) +{ +#if defined(_MSC_VER) + constexpr int defaultPipeSize{ 0 }; + + int result{ _pipe(descriptors, defaultPipeSize, _O_TEXT) }; #else - TempFile::TempFile() { - m_file = std::tmpfile(); - if (!m_file) { - CATCH_RUNTIME_ERROR("Could not create a temp file."); - } - } - + int result{ pipe(descriptors) }; #endif - TempFile::~TempFile() { - // TBD: What to do about errors here? - std::fclose(m_file); - // We manually create the file on Windows only, on Linux - // it will be autodeleted + if (result) + { + throw std::system_error{ errno, std::generic_category() }; + } +} + +inline size_t read_or_throw(int descriptor, void* buffer, size_t size) +{ #if defined(_MSC_VER) - std::remove(m_buffer); + int result{ _read(descriptor, buffer, static_cast(size)) }; +#else + ssize_t result{ read(descriptor, buffer, size) }; #endif + + if (result == -1) + { + throw std::system_error{ errno, std::generic_category() }; } + return static_cast(result); +} - FILE* TempFile::getFile() { - return m_file; +inline void fflush_or_throw(std::FILE* file) +{ + if (std::fflush(file)) + { + throw std::system_error{ errno, std::generic_category() }; + } +} + +jthread::jthread() noexcept : m_thread{} {} + +template +jthread::jthread(F&& f, Args&&... args) : m_thread{ std::forward(f), std::forward(args)... } {} + +// Not exactly like std::jthread, but close enough for the code below. +jthread::~jthread() noexcept +{ + if (m_thread.joinable()) + { + m_thread.join(); + } +} + +constexpr UniqueFileDescriptor::UniqueFileDescriptor() noexcept : m_value{} {} + +UniqueFileDescriptor::UniqueFileDescriptor(int value) noexcept : m_value{ value } {} + +constexpr UniqueFileDescriptor::UniqueFileDescriptor(UniqueFileDescriptor&& other) noexcept : + m_value{ other.m_value } +{ + other.m_value = 0; +} + +UniqueFileDescriptor::~UniqueFileDescriptor() noexcept +{ + if (m_value == 0) + { + return; } - std::string TempFile::getContents() { - std::stringstream sstr; - char buffer[100] = {}; - std::rewind(m_file); - while (std::fgets(buffer, sizeof(buffer), m_file)) { - sstr << buffer; + close_or_throw(m_value); // std::terminate on failure (due to noexcept) +} + +UniqueFileDescriptor& UniqueFileDescriptor::operator=(UniqueFileDescriptor&& other) noexcept +{ + if (this != &other) + { + if (m_value != 0) + { + close_or_throw(m_value); // std::terminate on failure (due to noexcept) } - return sstr.str(); + + m_value = other.m_value; + other.m_value = 0; } - OutputRedirect::OutputRedirect(std::string& stdout_dest, std::string& stderr_dest) : - m_originalStdout(dup(1)), - m_originalStderr(dup(2)), - m_stdoutDest(stdout_dest), - m_stderrDest(stderr_dest) { - dup2(fileno(m_stdoutFile.getFile()), 1); - dup2(fileno(m_stderrFile.getFile()), 2); + return *this; +} + +constexpr int UniqueFileDescriptor::get() { return m_value; } + +inline void create_pipe(UniqueFileDescriptor& readDescriptor, UniqueFileDescriptor& writeDescriptor) +{ + readDescriptor = {}; + writeDescriptor = {}; + + int descriptors[2]; + pipe_or_throw(descriptors); + + readDescriptor = UniqueFileDescriptor{ descriptors[0] }; + writeDescriptor = UniqueFileDescriptor{ descriptors[1] }; +} + +inline void read_thread(UniqueFileDescriptor&& file, std::string& result) +{ + std::string buffer{}; + constexpr size_t bufferSize{ 4096 }; + buffer.resize(bufferSize); + size_t sizeRead{}; + + while ((sizeRead = read_or_throw(file.get(), &buffer[0], bufferSize)) != 0) + { + result.append(buffer.data(), sizeRead); } +} - OutputRedirect::~OutputRedirect() { - Catch::cout() << std::flush; - fflush(stdout); - // Since we support overriding these streams, we flush cerr - // even though std::cerr is unbuffered - Catch::cerr() << std::flush; - Catch::clog() << std::flush; - fflush(stderr); +OutputFileRedirector::OutputFileRedirector(FILE* file, std::string& result) : + m_file{ file }, + m_fd{ fileno_or_throw(m_file) }, + m_previous{ dup_or_throw(m_fd) } +{ + fflush_or_throw(m_file); - dup2(m_originalStdout, 1); - dup2(m_originalStderr, 2); + UniqueFileDescriptor readDescriptor{}; + UniqueFileDescriptor writeDescriptor{}; + create_pipe(readDescriptor, writeDescriptor); - m_stdoutDest += m_stdoutFile.getContents(); - m_stderrDest += m_stderrFile.getContents(); - } + // Anonymous pipes have a limited buffer and require an active reader to ensure the writer does not become blocked. + // Use a separate thread to ensure the buffer does not get stuck full. + m_readThread = jthread{ [readDescriptor{ std::move(readDescriptor) }, &result] () mutable { + read_thread(std::move(readDescriptor), result); } }; + + dup2_or_throw(writeDescriptor.get(), m_fd); +} + +OutputFileRedirector::~OutputFileRedirector() noexcept +{ + fflush_or_throw(m_file); // std::terminate on failure (due to noexcept) + dup2_or_throw(m_previous.get(), m_fd); // std::terminate on failure (due to noexcept) +} + +OutputRedirect::OutputRedirect(std::string& output, std::string& error) : + m_output{ stdout, output }, m_error{ stderr, error } {} #endif // CATCH_CONFIG_NEW_CAPTURE @@ -139,6 +258,7 @@ namespace Catch { #if defined(CATCH_CONFIG_NEW_CAPTURE) #if defined(_MSC_VER) + #undef close #undef dup #undef dup2 #undef fileno diff --git a/src/catch2/internal/catch_output_redirect.hpp b/src/catch2/internal/catch_output_redirect.hpp index d3463d99..48e6fce6 100644 --- a/src/catch2/internal/catch_output_redirect.hpp +++ b/src/catch2/internal/catch_output_redirect.hpp @@ -16,6 +16,10 @@ #include #include +#if defined(CATCH_CONFIG_NEW_CAPTURE) +#include +#endif + namespace Catch { class RedirectedStream { @@ -66,50 +70,79 @@ namespace Catch { #if defined(CATCH_CONFIG_NEW_CAPTURE) - // Windows's implementation of std::tmpfile is terrible (it tries - // to create a file inside system folder, thus requiring elevated - // privileges for the binary), so we have to use tmpnam(_s) and - // create the file ourselves there. - class TempFile { - public: - TempFile(TempFile const&) = delete; - TempFile& operator=(TempFile const&) = delete; - TempFile(TempFile&&) = delete; - TempFile& operator=(TempFile&&) = delete; +#if defined(CATCH_CPP20_OR_GREATER) +using jthread = std::jthread; +#else +// Just enough of std::jthread from C++20 for the code below. +struct jthread final +{ + jthread() noexcept; - TempFile(); - ~TempFile(); + template + jthread(F&& f, Args&&... args); - std::FILE* getFile(); - std::string getContents(); + jthread(jthread const&) = delete; + jthread(jthread&&) noexcept = default; - private: - std::FILE* m_file = nullptr; - #if defined(_MSC_VER) - char m_buffer[L_tmpnam] = { 0 }; - #endif - }; + jthread& operator=(jthread const&) noexcept = delete; + // Not exactly like std::jthread, but close enough for the code below. + jthread& operator=(jthread&&) noexcept = default; - class OutputRedirect { - public: - OutputRedirect(OutputRedirect const&) = delete; - OutputRedirect& operator=(OutputRedirect const&) = delete; - OutputRedirect(OutputRedirect&&) = delete; - OutputRedirect& operator=(OutputRedirect&&) = delete; + // Not exactly like std::jthread, but close enough for the code below. + ~jthread() noexcept; +private: + std::thread m_thread; +}; +#endif - OutputRedirect(std::string& stdout_dest, std::string& stderr_dest); - ~OutputRedirect(); +struct UniqueFileDescriptor final +{ + constexpr UniqueFileDescriptor() noexcept; + explicit UniqueFileDescriptor(int value) noexcept; - private: - int m_originalStdout = -1; - int m_originalStderr = -1; - TempFile m_stdoutFile; - TempFile m_stderrFile; - std::string& m_stdoutDest; - std::string& m_stderrDest; - }; + UniqueFileDescriptor(UniqueFileDescriptor const&) = delete; + constexpr UniqueFileDescriptor(UniqueFileDescriptor&& other) noexcept; + + ~UniqueFileDescriptor() noexcept; + + UniqueFileDescriptor& operator=(UniqueFileDescriptor const&) = delete; + UniqueFileDescriptor& operator=(UniqueFileDescriptor&& other) noexcept; + + constexpr int get(); + +private: + int m_value; +}; + +struct OutputFileRedirector final +{ + explicit OutputFileRedirector(std::FILE* file, std::string& result); + + OutputFileRedirector(OutputFileRedirector const&) = delete; + OutputFileRedirector(OutputFileRedirector&&) = delete; + + ~OutputFileRedirector() noexcept; + + OutputFileRedirector& operator=(OutputFileRedirector const&) = delete; + OutputFileRedirector& operator=(OutputFileRedirector&&) = delete; + +private: + std::FILE* m_file; + int m_fd; + UniqueFileDescriptor m_previous; + jthread m_readThread; +}; + +struct OutputRedirect final +{ + OutputRedirect(std::string& output, std::string& error); + +private: + OutputFileRedirector m_output; + OutputFileRedirector m_error; +}; #endif