From ac345f86af827c42216d3a6bd0fd6482afb8c7f0 Mon Sep 17 00:00:00 2001 From: davidmatson Date: Wed, 29 Jun 2022 17:42:15 -0700 Subject: [PATCH 1/5] 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 | 230 +++++++++++++----- src/catch2/internal/catch_output_redirect.hpp | 105 +++++--- 3 files changed, 248 insertions(+), 91 deletions(-) diff --git a/src/catch2/internal/catch_compiler_capabilities.hpp b/src/catch2/internal/catch_compiler_capabilities.hpp index 00280277af..32612a0cd7 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 ae06014127..83bcdcd2ee 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,85 +63,202 @@ 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() }; } -#else - TempFile::TempFile() { - m_file = std::tmpfile(); - if (!m_file) { - CATCH_RUNTIME_ERROR("Could not create a temp file."); - } +} + +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 + 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() }; } +} - std::string TempFile::getContents() { - std::stringstream sstr; - char buffer[100] = {}; - std::rewind(m_file); - while (std::fgets(buffer, sizeof(buffer), m_file)) { - sstr << buffer; - } - return sstr.str(); +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{} {} - 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); +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; } - 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); + close_or_throw(m_value); // std::terminate on failure (due to noexcept) +} - dup2(m_originalStdout, 1); - dup2(m_originalStderr, 2); +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) + } - m_stdoutDest += m_stdoutFile.getContents(); - m_stderrDest += m_stderrFile.getContents(); + m_value = other.m_value; + other.m_value = 0; } + 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); + } +} + +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); + + UniqueFileDescriptor readDescriptor{}; + UniqueFileDescriptor writeDescriptor{}; + create_pipe(readDescriptor, writeDescriptor); + + // 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 } // 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 d3463d992c..48e6fce6bf 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 From 713e31cd53914d60c3a10d88f5958cf289c269b9 Mon Sep 17 00:00:00 2001 From: davidmatson Date: Tue, 12 Jul 2022 12:41:51 -0700 Subject: [PATCH 2/5] Update per contributing docs. --- src/catch2/internal/catch_enforce.cpp | 6 ++++++ src/catch2/internal/catch_enforce.hpp | 5 +++++ src/catch2/internal/catch_output_redirect.cpp | 21 ++++++++++--------- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/catch2/internal/catch_enforce.cpp b/src/catch2/internal/catch_enforce.cpp index 4bc47ce3d3..67f2e41d92 100644 --- a/src/catch2/internal/catch_enforce.cpp +++ b/src/catch2/internal/catch_enforce.cpp @@ -9,6 +9,7 @@ #include #include +#include namespace Catch { @@ -36,6 +37,11 @@ namespace Catch { throw_exception(std::runtime_error(msg)); } + [[noreturn]] + void throw_system_error(int ev, const std::error_category& ecat) { + throw_exception(std::system_error(ev, ecat)); + } + } // namespace Catch; diff --git a/src/catch2/internal/catch_enforce.hpp b/src/catch2/internal/catch_enforce.hpp index db52a0e2d2..6ec810fc44 100644 --- a/src/catch2/internal/catch_enforce.hpp +++ b/src/catch2/internal/catch_enforce.hpp @@ -32,6 +32,8 @@ namespace Catch { void throw_domain_error(std::string const& msg); [[noreturn]] void throw_runtime_error(std::string const& msg); + [[noreturn]] + void throw_system_error(int ev, const std::error_category& ecat); } // namespace Catch; @@ -47,6 +49,9 @@ namespace Catch { #define CATCH_RUNTIME_ERROR(...) \ Catch::throw_runtime_error(CATCH_MAKE_MSG( __VA_ARGS__ )) +#define CATCH_SYSTEM_ERROR(ev, ecat) \ + Catch::throw_system_error((ev), (ecat)) + #define CATCH_ENFORCE( condition, ... ) \ do{ if( !(condition) ) CATCH_ERROR( __VA_ARGS__ ); } while(false) diff --git a/src/catch2/internal/catch_output_redirect.cpp b/src/catch2/internal/catch_output_redirect.cpp index 83bcdcd2ee..105e100227 100644 --- a/src/catch2/internal/catch_output_redirect.cpp +++ b/src/catch2/internal/catch_output_redirect.cpp @@ -14,6 +14,7 @@ #include #if defined(CATCH_CONFIG_NEW_CAPTURE) + #include #include #if defined(_MSC_VER) #include // _O_TEXT @@ -67,7 +68,7 @@ inline void close_or_throw(int descriptor) { if (close(descriptor)) { - throw std::system_error{ errno, std::generic_category() }; + CATCH_SYSTEM_ERROR(errno, std::generic_category()); } } @@ -77,7 +78,7 @@ inline int dup_or_throw(int descriptor) if (result == -1) { - throw std::system_error{ errno, std::generic_category() }; + CATCH_SYSTEM_ERROR(errno, std::generic_category()); } return result; @@ -89,7 +90,7 @@ inline int dup2_or_throw(int sourceDescriptor, int destinationDescriptor) if (result == -1) { - throw std::system_error{ errno, std::generic_category() }; + CATCH_SYSTEM_ERROR(errno, std::generic_category()); } return result; @@ -101,7 +102,7 @@ inline int fileno_or_throw(std::FILE* file) if (result == -1) { - throw std::system_error{ errno, std::generic_category() }; + CATCH_SYSTEM_ERROR(errno, std::generic_category()); } return result; @@ -119,7 +120,7 @@ inline void pipe_or_throw(int descriptors[2]) if (result) { - throw std::system_error{ errno, std::generic_category() }; + CATCH_SYSTEM_ERROR(errno, std::generic_category()); } } @@ -133,7 +134,7 @@ inline size_t read_or_throw(int descriptor, void* buffer, size_t size) if (result == -1) { - throw std::system_error{ errno, std::generic_category() }; + CATCH_SYSTEM_ERROR(errno, std::generic_category()); } return static_cast(result); @@ -143,14 +144,14 @@ inline void fflush_or_throw(std::FILE* file) { if (std::fflush(file)) { - throw std::system_error{ errno, std::generic_category() }; + CATCH_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)... } {} +jthread::jthread(F&& f, Args&&... args) : m_thread{ CATCH_FORWARD(f), CATCH_FORWARD(args)... } {} // Not exactly like std::jthread, but close enough for the code below. jthread::~jthread() noexcept @@ -237,8 +238,8 @@ OutputFileRedirector::OutputFileRedirector(FILE* file, std::string& result) : // 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); } }; + m_readThread = jthread{ [readDescriptor{ CATCH_MOVE(readDescriptor) }, &result] () mutable { + read_thread(CATCH_MOVE(readDescriptor), result); } }; dup2_or_throw(writeDescriptor.get(), m_fd); } From 10459a76f74417d2cf2c5d6e037a0dc4f3afd647 Mon Sep 17 00:00:00 2001 From: davidmatson Date: Tue, 13 Sep 2022 17:08:23 -0700 Subject: [PATCH 3/5] Incorporate PR feedback. Keep previous implementation when threads are not available. --- .../internal/catch_compiler_capabilities.hpp | 4 - src/catch2/internal/catch_output_redirect.cpp | 416 ++++++++++-------- src/catch2/internal/catch_output_redirect.hpp | 157 ++++--- 3 files changed, 332 insertions(+), 245 deletions(-) diff --git a/src/catch2/internal/catch_compiler_capabilities.hpp b/src/catch2/internal/catch_compiler_capabilities.hpp index 32612a0cd7..00280277af 100644 --- a/src/catch2/internal/catch_compiler_capabilities.hpp +++ b/src/catch2/internal/catch_compiler_capabilities.hpp @@ -37,10 +37,6 @@ # 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 105e100227..aa339b165b 100644 --- a/src/catch2/internal/catch_output_redirect.cpp +++ b/src/catch2/internal/catch_output_redirect.cpp @@ -5,37 +5,46 @@ // https://www.boost.org/LICENSE_1_0.txt) // SPDX-License-Identifier: BSL-1.0 -#include #include +#include +#include #include - #include #include #include -#if defined(CATCH_CONFIG_NEW_CAPTURE) - #include - #include - #if defined(_MSC_VER) - #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 // close, dup, dup2, fileno, pipe and read - #endif +#if defined( CATCH_CONFIG_NEW_CAPTURE ) +# if defined( CATCH_INTERNAL_CONFIG_USE_ASYNC ) +# include +# if defined( _MSC_VER ) +# 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 // close, dup, dup2, fileno, pipe and read +# endif +# else +# if defined( _MSC_VER ) +# include //_dup and _dup2 +# define dup _dup +# define dup2 _dup2 +# define fileno _fileno +# else +# include // dup and dup2 +# endif +# endif #endif - namespace Catch { - RedirectedStream::RedirectedStream( std::ostream& originalStream, std::ostream& redirectionStream ) - : m_originalStream( originalStream ), + RedirectedStream::RedirectedStream( std::ostream& originalStream, + std::ostream& redirectionStream ): + m_originalStream( originalStream ), m_redirectionStream( redirectionStream ), - m_prevBuf( m_originalStream.rdbuf() ) - { + m_prevBuf( m_originalStream.rdbuf() ) { m_originalStream.rdbuf( m_redirectionStream.rdbuf() ); } @@ -43,225 +52,282 @@ namespace Catch { m_originalStream.rdbuf( m_prevBuf ); } - RedirectedStdOut::RedirectedStdOut() : m_cout( Catch::cout(), m_rss.get() ) {} + RedirectedStdOut::RedirectedStdOut(): + m_cout( Catch::cout(), m_rss.get() ) {} auto RedirectedStdOut::str() const -> std::string { return m_rss.str(); } - RedirectedStdErr::RedirectedStdErr() - : m_cerr( Catch::cerr(), m_rss.get() ), - m_clog( Catch::clog(), m_rss.get() ) - {} + RedirectedStdErr::RedirectedStdErr(): + m_cerr( Catch::cerr(), m_rss.get() ), + m_clog( Catch::clog(), m_rss.get() ) {} auto RedirectedStdErr::str() const -> std::string { return m_rss.str(); } - RedirectedStreams::RedirectedStreams(std::string& redirectedCout, std::string& redirectedCerr) - : m_redirectedCout(redirectedCout), - m_redirectedCerr(redirectedCerr) - {} + RedirectedStreams::RedirectedStreams( std::string& redirectedCout, + std::string& redirectedCerr ): + m_redirectedCout( redirectedCout ), + m_redirectedCerr( redirectedCerr ) {} RedirectedStreams::~RedirectedStreams() { m_redirectedCout += m_redirectedStdOut.str(); m_redirectedCerr += m_redirectedStdErr.str(); } -#if defined(CATCH_CONFIG_NEW_CAPTURE) +#if defined( CATCH_CONFIG_NEW_CAPTURE ) + +# if defined( CATCH_INTERNAL_CONFIG_USE_ASYNC ) -inline void close_or_throw(int descriptor) -{ - if (close(descriptor)) - { - CATCH_SYSTEM_ERROR(errno, std::generic_category()); + static inline void close_or_throw( int descriptor ) { + if ( close( descriptor ) ) { + CATCH_SYSTEM_ERROR( errno, std::generic_category() ); + } } -} -inline int dup_or_throw(int descriptor) -{ - int result{ dup(descriptor) }; + static inline int dup_or_throw( int descriptor ) { + int result{ dup( descriptor ) }; + + if ( result == -1 ) { + CATCH_SYSTEM_ERROR( errno, std::generic_category() ); + } - if (result == -1) - { - CATCH_SYSTEM_ERROR(errno, std::generic_category()); + return result; } - return result; -} + static inline int dup2_or_throw( int sourceDescriptor, + int destinationDescriptor ) { + int result{ dup2( sourceDescriptor, destinationDescriptor ) }; -inline int dup2_or_throw(int sourceDescriptor, int destinationDescriptor) -{ - int result{ dup2(sourceDescriptor, destinationDescriptor) }; + if ( result == -1 ) { + CATCH_SYSTEM_ERROR( errno, std::generic_category() ); + } - if (result == -1) - { - CATCH_SYSTEM_ERROR(errno, std::generic_category()); + return result; } - return result; -} + static inline int fileno_or_throw( std::FILE* file ) { + int result{ fileno( file ) }; -inline int fileno_or_throw(std::FILE* file) -{ - int result{ fileno(file) }; + if ( result == -1 ) { + CATCH_SYSTEM_ERROR( errno, std::generic_category() ); + } - if (result == -1) - { - CATCH_SYSTEM_ERROR(errno, std::generic_category()); + return result; } - return result; -} + static inline void pipe_or_throw( int descriptors[2] ) { +# if defined( _MSC_VER ) + constexpr int defaultPipeSize{ 0 }; -inline void pipe_or_throw(int descriptors[2]) -{ -#if defined(_MSC_VER) - constexpr int defaultPipeSize{ 0 }; + int result{ _pipe( descriptors, defaultPipeSize, _O_TEXT ) }; +# else + int result{ pipe( descriptors ) }; +# endif - int result{ _pipe(descriptors, defaultPipeSize, _O_TEXT) }; -#else - int result{ pipe(descriptors) }; -#endif + if ( result ) { + CATCH_SYSTEM_ERROR( errno, std::generic_category() ); + } + } - if (result) - { - CATCH_SYSTEM_ERROR(errno, std::generic_category()); + static inline size_t + read_or_throw( int descriptor, void* buffer, size_t size ) { +# if defined( _MSC_VER ) + int result{ + _read( descriptor, buffer, static_cast( size ) ) }; +# else + ssize_t result{ read( descriptor, buffer, size ) }; +# endif + + if ( result == -1 ) { + CATCH_SYSTEM_ERROR( errno, std::generic_category() ); + } + + return static_cast( result ); } -} - -inline size_t read_or_throw(int descriptor, void* buffer, size_t size) -{ -#if defined(_MSC_VER) - int result{ _read(descriptor, buffer, static_cast(size)) }; -#else - ssize_t result{ read(descriptor, buffer, size) }; -#endif - if (result == -1) - { - CATCH_SYSTEM_ERROR(errno, std::generic_category()); + static inline void fflush_or_throw( std::FILE* file ) { + if ( std::fflush( file ) ) { + CATCH_SYSTEM_ERROR( errno, std::generic_category() ); + } } - return static_cast(result); -} + constexpr UniqueFileDescriptor::UniqueFileDescriptor() noexcept: + m_value{} {} + + UniqueFileDescriptor::UniqueFileDescriptor( int value ) noexcept: + m_value{ value } {} -inline void fflush_or_throw(std::FILE* file) -{ - if (std::fflush(file)) - { - CATCH_SYSTEM_ERROR(errno, std::generic_category()); + constexpr UniqueFileDescriptor::UniqueFileDescriptor( + UniqueFileDescriptor&& other ) noexcept: + m_value{ other.m_value } { + other.m_value = 0; } -} -jthread::jthread() noexcept : m_thread{} {} + UniqueFileDescriptor::~UniqueFileDescriptor() noexcept { + if ( m_value == 0 ) { + return; + } -template -jthread::jthread(F&& f, Args&&... args) : m_thread{ CATCH_FORWARD(f), CATCH_FORWARD(args)... } {} + close_or_throw( + m_value ); // std::terminate on failure (due to noexcept) + } -// Not exactly like std::jthread, but close enough for the code below. -jthread::~jthread() noexcept -{ - if (m_thread.joinable()) - { - m_thread.join(); + UniqueFileDescriptor& + UniqueFileDescriptor::operator=( UniqueFileDescriptor&& other ) noexcept { + std::swap( m_value, other.m_value ); + return *this; } -} -constexpr UniqueFileDescriptor::UniqueFileDescriptor() noexcept : m_value{} {} + constexpr int UniqueFileDescriptor::get() { return m_value; } -UniqueFileDescriptor::UniqueFileDescriptor(int value) noexcept : m_value{ value } {} + static inline void create_pipe( UniqueFileDescriptor& readDescriptor, + UniqueFileDescriptor& writeDescriptor ) { + readDescriptor = {}; + writeDescriptor = {}; -constexpr UniqueFileDescriptor::UniqueFileDescriptor(UniqueFileDescriptor&& other) noexcept : - m_value{ other.m_value } -{ - other.m_value = 0; -} + int descriptors[2]; + pipe_or_throw( descriptors ); -UniqueFileDescriptor::~UniqueFileDescriptor() noexcept -{ - if (m_value == 0) - { - return; + readDescriptor = UniqueFileDescriptor{ descriptors[0] }; + writeDescriptor = UniqueFileDescriptor{ descriptors[1] }; } - close_or_throw(m_value); // std::terminate on failure (due to noexcept) -} + static inline void read_thread( UniqueFileDescriptor&& file, + std::string& result ) { + std::string buffer{}; + constexpr size_t bufferSize{ 4096 }; + buffer.resize( bufferSize ); + size_t sizeRead{}; -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) + while ( ( sizeRead = read_or_throw( + file.get(), &buffer[0], bufferSize ) ) != 0 ) { + result.append( buffer.data(), sizeRead ); } + } - m_value = other.m_value; - other.m_value = 0; + 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 ); + + UniqueFileDescriptor readDescriptor{}; + UniqueFileDescriptor writeDescriptor{}; + create_pipe( readDescriptor, writeDescriptor ); + + // 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 = + std::thread{ [readDescriptor{ CATCH_MOVE( readDescriptor ) }, + &result]() mutable { + read_thread( CATCH_MOVE( readDescriptor ), result ); + } }; + + // Replace the stdout or stderr file descriptor with the write end of + // the pipe. + dup2_or_throw( writeDescriptor.get(), m_fd ); } - return *this; -} + OutputFileRedirector::~OutputFileRedirector() noexcept { + if ( m_readThread.joinable() ) { + m_readThread.join(); + } + + fflush_or_throw( + m_file ); // std::terminate on failure (due to noexcept) -constexpr int UniqueFileDescriptor::get() { return m_value; } + // Restore the original stdout or stderr file descriptor. + dup2_or_throw( m_previous.get(), + m_fd ); // std::terminate on failure (due to noexcept) + } -inline void create_pipe(UniqueFileDescriptor& readDescriptor, UniqueFileDescriptor& writeDescriptor) -{ - readDescriptor = {}; - writeDescriptor = {}; + OutputRedirect::OutputRedirect( std::string& output, std::string& error ): + m_output{ stdout, output }, m_error{ stderr, error } {} - int descriptors[2]; - pipe_or_throw(descriptors); +# else // !CATCH_INTERNAL_CONFIG_USE_ASYNC - readDescriptor = UniqueFileDescriptor{ descriptors[0] }; - writeDescriptor = UniqueFileDescriptor{ descriptors[1] }; -} +# 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 ); + } + } +# else + TempFile::TempFile() { + m_file = std::tmpfile(); + if ( !m_file ) { + CATCH_RUNTIME_ERROR( "Could not create a temp file." ); + } + } -inline void read_thread(UniqueFileDescriptor&& file, std::string& result) -{ - std::string buffer{}; - constexpr size_t bufferSize{ 4096 }; - buffer.resize(bufferSize); - size_t sizeRead{}; +# endif - while ((sizeRead = read_or_throw(file.get(), &buffer[0], bufferSize)) != 0) - { - result.append(buffer.data(), sizeRead); + 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 defined( _MSC_VER ) + std::remove( m_buffer ); +# endif } -} -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); + FILE* TempFile::getFile() { return m_file; } - UniqueFileDescriptor readDescriptor{}; - UniqueFileDescriptor writeDescriptor{}; - create_pipe(readDescriptor, writeDescriptor); + std::string TempFile::getContents() { + std::stringstream sstr; + char buffer[100] = {}; + std::rewind( m_file ); + while ( std::fgets( buffer, sizeof( buffer ), m_file ) ) { + sstr << buffer; + } + return sstr.str(); + } + + 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 ); + } - // 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{ CATCH_MOVE(readDescriptor) }, &result] () mutable { - read_thread(CATCH_MOVE(readDescriptor), result); } }; + 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 ); - dup2_or_throw(writeDescriptor.get(), m_fd); -} + dup2( m_originalStdout, 1 ); + dup2( m_originalStderr, 2 ); -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) -} + m_stdoutDest += m_stdoutFile.getContents(); + m_stderrDest += m_stderrFile.getContents(); + } -OutputRedirect::OutputRedirect(std::string& output, std::string& error) : - m_output{ stdout, output }, m_error{ stderr, error } {} +# endif // CATCH_INTERNAL_CONFIG_USE_ASYNC #endif // CATCH_CONFIG_NEW_CAPTURE } // namespace Catch -#if defined(CATCH_CONFIG_NEW_CAPTURE) - #if defined(_MSC_VER) - #undef close - #undef dup - #undef dup2 - #undef fileno - #endif +#if defined( CATCH_CONFIG_NEW_CAPTURE ) +# if defined( _MSC_VER ) +# undef close +# undef dup +# undef dup2 +# undef fileno +# endif #endif diff --git a/src/catch2/internal/catch_output_redirect.hpp b/src/catch2/internal/catch_output_redirect.hpp index 48e6fce6bf..599424f7a2 100644 --- a/src/catch2/internal/catch_output_redirect.hpp +++ b/src/catch2/internal/catch_output_redirect.hpp @@ -8,16 +8,17 @@ #ifndef CATCH_OUTPUT_REDIRECT_HPP_INCLUDED #define CATCH_OUTPUT_REDIRECT_HPP_INCLUDED +#include #include #include -#include - #include #include #include -#if defined(CATCH_CONFIG_NEW_CAPTURE) -#include +#if defined( CATCH_CONFIG_NEW_CAPTURE ) +# if defined( CATCH_INTERNAL_CONFIG_USE_ASYNC ) +# include +# endif #endif namespace Catch { @@ -28,13 +29,15 @@ namespace Catch { std::streambuf* m_prevBuf; public: - RedirectedStream( std::ostream& originalStream, std::ostream& redirectionStream ); + RedirectedStream( std::ostream& originalStream, + std::ostream& redirectionStream ); ~RedirectedStream(); }; class RedirectedStdOut { ReusableStringStream m_rss; RedirectedStream m_cout; + public: RedirectedStdOut(); auto str() const -> std::string; @@ -47,6 +50,7 @@ namespace Catch { ReusableStringStream m_rss; RedirectedStream m_cerr; RedirectedStream m_clog; + public: RedirectedStdErr(); auto str() const -> std::string; @@ -54,13 +58,15 @@ namespace Catch { class RedirectedStreams { public: - RedirectedStreams(RedirectedStreams const&) = delete; - RedirectedStreams& operator=(RedirectedStreams const&) = delete; - RedirectedStreams(RedirectedStreams&&) = delete; - RedirectedStreams& operator=(RedirectedStreams&&) = delete; + RedirectedStreams( RedirectedStreams const& ) = delete; + RedirectedStreams& operator=( RedirectedStreams const& ) = delete; + RedirectedStreams( RedirectedStreams&& ) = delete; + RedirectedStreams& operator=( RedirectedStreams&& ) = delete; - RedirectedStreams(std::string& redirectedCout, std::string& redirectedCerr); + RedirectedStreams( std::string& redirectedCout, + std::string& redirectedCerr ); ~RedirectedStreams(); + private: std::string& m_redirectedCout; std::string& m_redirectedCerr; @@ -68,83 +74,102 @@ namespace Catch { RedirectedStdErr m_redirectedStdErr; }; -#if defined(CATCH_CONFIG_NEW_CAPTURE) +#if defined( CATCH_CONFIG_NEW_CAPTURE ) -#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; +# if defined( CATCH_INTERNAL_CONFIG_USE_ASYNC ) - template - jthread(F&& f, Args&&... args); + struct UniqueFileDescriptor final { + constexpr UniqueFileDescriptor() noexcept; + explicit UniqueFileDescriptor( int value ) noexcept; - jthread(jthread const&) = delete; - jthread(jthread&&) noexcept = default; + UniqueFileDescriptor( UniqueFileDescriptor const& ) = delete; + constexpr UniqueFileDescriptor( UniqueFileDescriptor&& other ) noexcept; - jthread& operator=(jthread const&) noexcept = delete; + ~UniqueFileDescriptor() noexcept; - // Not exactly like std::jthread, but close enough for the code below. - jthread& operator=(jthread&&) noexcept = default; + UniqueFileDescriptor& operator=( UniqueFileDescriptor const& ) = delete; + UniqueFileDescriptor& + operator=( UniqueFileDescriptor&& other ) noexcept; - // Not exactly like std::jthread, but close enough for the code below. - ~jthread() noexcept; + constexpr int get(); -private: - std::thread m_thread; -}; -#endif + private: + int m_value; + }; + + struct OutputFileRedirector final { + explicit OutputFileRedirector( std::FILE* file, std::string& result ); + + OutputFileRedirector( OutputFileRedirector const& ) = delete; + OutputFileRedirector( OutputFileRedirector&& ) = delete; -struct UniqueFileDescriptor final -{ - constexpr UniqueFileDescriptor() noexcept; - explicit UniqueFileDescriptor(int value) noexcept; + ~OutputFileRedirector() noexcept; - UniqueFileDescriptor(UniqueFileDescriptor const&) = delete; - constexpr UniqueFileDescriptor(UniqueFileDescriptor&& other) noexcept; + OutputFileRedirector& operator=( OutputFileRedirector const& ) = delete; + OutputFileRedirector& operator=( OutputFileRedirector&& ) = delete; - ~UniqueFileDescriptor() noexcept; + private: + std::FILE* m_file; + int m_fd; + UniqueFileDescriptor m_previous; + std::thread m_readThread; + }; - UniqueFileDescriptor& operator=(UniqueFileDescriptor const&) = delete; - UniqueFileDescriptor& operator=(UniqueFileDescriptor&& other) noexcept; + struct OutputRedirect final { + OutputRedirect( std::string& output, std::string& error ); - constexpr int get(); + private: + OutputFileRedirector m_output; + OutputFileRedirector m_error; + }; -private: - int m_value; -}; +# else // !CATCH_INTERNAL_CONFIG_USE_ASYNC -struct OutputFileRedirector final -{ - explicit OutputFileRedirector(std::FILE* file, std::string& result); + // 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; - OutputFileRedirector(OutputFileRedirector const&) = delete; - OutputFileRedirector(OutputFileRedirector&&) = delete; + TempFile(); + ~TempFile(); - ~OutputFileRedirector() noexcept; + std::FILE* getFile(); + std::string getContents(); - OutputFileRedirector& operator=(OutputFileRedirector const&) = delete; - OutputFileRedirector& operator=(OutputFileRedirector&&) = delete; + private: + std::FILE* m_file = nullptr; +# if defined( _MSC_VER ) + char m_buffer[L_tmpnam] = { 0 }; +# endif + }; -private: - std::FILE* m_file; - int m_fd; - UniqueFileDescriptor m_previous; - jthread m_readThread; -}; + class OutputRedirect { + public: + OutputRedirect( OutputRedirect const& ) = delete; + OutputRedirect& operator=( OutputRedirect const& ) = delete; + OutputRedirect( OutputRedirect&& ) = delete; + OutputRedirect& operator=( OutputRedirect&& ) = delete; -struct OutputRedirect final -{ - OutputRedirect(std::string& output, std::string& error); + OutputRedirect( std::string& stdout_dest, std::string& stderr_dest ); + ~OutputRedirect(); -private: - OutputFileRedirector m_output; - OutputFileRedirector m_error; -}; + private: + int m_originalStdout = -1; + int m_originalStderr = -1; + TempFile m_stdoutFile; + TempFile m_stderrFile; + std::string& m_stdoutDest; + std::string& m_stderrDest; + }; -#endif +# endif // CATCH_INTERNAL_CONFIG_USE_ASYNC +#endif // CATCH_CONFIG_NEW_CAPTURE } // end namespace Catch From a560a9f827527d01e81d81547d2ced3f707cbb01 Mon Sep 17 00:00:00 2001 From: davidmatson Date: Tue, 20 Sep 2022 15:18:28 -0700 Subject: [PATCH 4/5] Fix OutputFileRedirector destructor ordering. --- src/catch2/internal/catch_output_redirect.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/catch2/internal/catch_output_redirect.cpp b/src/catch2/internal/catch_output_redirect.cpp index aa339b165b..3686636b77 100644 --- a/src/catch2/internal/catch_output_redirect.cpp +++ b/src/catch2/internal/catch_output_redirect.cpp @@ -228,16 +228,16 @@ namespace Catch { } OutputFileRedirector::~OutputFileRedirector() noexcept { - if ( m_readThread.joinable() ) { - m_readThread.join(); - } - fflush_or_throw( m_file ); // std::terminate on failure (due to noexcept) // Restore the original stdout or stderr file descriptor. dup2_or_throw( m_previous.get(), m_fd ); // std::terminate on failure (due to noexcept) + + if ( m_readThread.joinable() ) { + m_readThread.join(); + } } OutputRedirect::OutputRedirect( std::string& output, std::string& error ): From 3b474ce3cf55179996b73ad6b1c0b43a836a1d5b Mon Sep 17 00:00:00 2001 From: davidmatson Date: Wed, 26 Oct 2022 16:56:27 -0700 Subject: [PATCH 5/5] Add coverage for CATCH_SYSTEM_ERROR. --- tests/SelfTest/IntrospectiveTests/Details.tests.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/SelfTest/IntrospectiveTests/Details.tests.cpp b/tests/SelfTest/IntrospectiveTests/Details.tests.cpp index 987910a948..34b6ecf358 100644 --- a/tests/SelfTest/IntrospectiveTests/Details.tests.cpp +++ b/tests/SelfTest/IntrospectiveTests/Details.tests.cpp @@ -20,6 +20,7 @@ TEST_CASE("Check that our error handling macros throw the right exceptions", "[! REQUIRE_THROWS_AS(CATCH_INTERNAL_ERROR(""), std::logic_error); REQUIRE_THROWS_AS(CATCH_ERROR(""), std::domain_error); REQUIRE_THROWS_AS(CATCH_RUNTIME_ERROR(""), std::runtime_error); + REQUIRE_THROWS_AS(CATCH_SYSTEM_ERROR(0, std::generic_category()), std::system_error); REQUIRE_THROWS_AS([](){CATCH_ENFORCE(false, "");}(), std::domain_error); REQUIRE_NOTHROW([](){CATCH_ENFORCE(true, "");}()); }