Refactor pipe-based redirect to not always create new thread and pipe

src/catch2/internal/catch_output_redirect.cpp

@@ -19,6 +19,7 @@
 
 #if defined(CATCH_CONFIG_USE_ASYNC)
 #include <thread>
+#include <mutex>
 #endif
 
 #if defined( CATCH_CONFIG_NEW_CAPTURE ) || defined(CATCH_CONFIG_USE_ASYNC)
@@ -253,68 +254,37 @@ namespace Catch {
 
 #if defined( CATCH_CONFIG_USE_ASYNC )
 
-        struct UniqueFileDescriptor final {
+        static inline void pipe_or_throw( int descriptors[2] ) {
-            constexpr UniqueFileDescriptor() noexcept;
+#    if defined( _MSC_VER )
-            explicit UniqueFileDescriptor( int value ) noexcept;
+            constexpr int defaultPipeSize{ 0 };
 
-            UniqueFileDescriptor( UniqueFileDescriptor const& ) = delete;
+            int result{ _pipe( descriptors, defaultPipeSize, _O_BINARY ) };
-            constexpr UniqueFileDescriptor(
+#    else
-                UniqueFileDescriptor&& other ) noexcept;
+            int result{ pipe( descriptors ) };
+#    endif
 
-            ~UniqueFileDescriptor() noexcept;
+            if ( result ) {
-
+                CATCH_INTERNAL_ERROR( "pipe-or-throw" );
-            UniqueFileDescriptor&
+                // CATCH_SYSTEM_ERROR( errno, std::generic_category() );
-            operator=( UniqueFileDescriptor const& ) = delete;
+            }
-            UniqueFileDescriptor&
+        }
-            operator=( UniqueFileDescriptor&& other ) noexcept;
+
-
+        static inline size_t
-            constexpr int get();
+        read_or_throw( int descriptor, void* buffer, size_t size ) {
-
+#    if defined( _MSC_VER )
-        private:
+            int result{
-            int m_value;
+                _read( descriptor, buffer, static_cast<unsigned>( size ) ) };
-        };
+#    else
-
+            ssize_t result{ read( descriptor, buffer, size ) };
-        struct OutputFileRedirector final {
+#    endif
-            explicit OutputFileRedirector( std::FILE* file,
+
-                                           std::string& result );
+            if ( result == -1 ) {
-
+                CATCH_INTERNAL_ERROR( "read-or-throw" );
-            OutputFileRedirector( OutputFileRedirector const& ) = delete;
+                // CATCH_SYSTEM_ERROR( errno, std::generic_category() );
-            OutputFileRedirector( OutputFileRedirector&& ) = delete;
+            }
-
+
-            ~OutputFileRedirector() noexcept;
+            return static_cast<size_t>( result );
-
-            OutputFileRedirector&
-            operator=( OutputFileRedirector const& ) = delete;
-            OutputFileRedirector& operator=( OutputFileRedirector&& ) = delete;
-
-        private:
-            std::FILE* m_file;
-            int m_fd;
-            UniqueFileDescriptor m_previous;
-            std::thread m_readThread;
-        };
-
-        struct PipeRedirect final {
-            PipeRedirect( std::string& output, std::string& error ):
-                m_output{ stdout, output }, m_error{ stderr, error } {}
-
-        private:
-            OutputFileRedirector m_output;
-            OutputFileRedirector m_error;
-        };
-
-        class PipeRedirectWrapper : public OutputRedirect {
-            void activateImpl() override { m_redirect = Detail::make_unique<PipeRedirect>( m_stdout, m_stderr ); }
-            void deactivateImpl() override { m_redirect.reset(); }
-            std::string getStdout() override { return m_stdout; }
-            std::string getStderr() override { return m_stderr; }
-            void clearBuffers() override {
-                m_stdout.clear();
-                m_stderr.clear();
         }
-            Detail::unique_ptr<PipeRedirect> m_redirect;
-            std::string m_stdout, m_stderr;
-        };
 
         static inline void close_or_throw( int descriptor ) {
             if ( close( descriptor ) ) {
@@ -357,38 +327,6 @@ namespace Catch {
             return result;
         }
 
-        static inline void pipe_or_throw( int descriptors[2] ) {
-#    if defined( _MSC_VER )
-            constexpr int defaultPipeSize{ 0 };
-
-            int result{ _pipe( descriptors, defaultPipeSize, _O_BINARY ) };
-#    else
-            int result{ pipe( descriptors ) };
-#    endif
-
-            if ( result ) {
-                CATCH_INTERNAL_ERROR( "pipe-or-throw" );
-                // 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<unsigned>( size ) ) };
-#    else
-            ssize_t result{ read( descriptor, buffer, size ) };
-#    endif
-
-            if ( result == -1 ) {
-                CATCH_INTERNAL_ERROR( "read-or-throw" );
-                //CATCH_SYSTEM_ERROR( errno, std::generic_category() );
-            }
-
-            return static_cast<size_t>( result );
-        }
-
         static inline void fflush_or_throw( std::FILE* file ) {
             if ( std::fflush( file ) ) {
                 CATCH_INTERNAL_ERROR( "fflush-or-throw" );
@@ -396,96 +334,95 @@ namespace Catch {
             }
         }
 
-        constexpr UniqueFileDescriptor::UniqueFileDescriptor() noexcept:
+        class StreamPipeHandler {
-            m_value{} {}
+            int m_originalFd = -1;
+            int m_pipeReadEnd = -1;
+            int m_pipeWriteEnd = -1;
+            FILE* m_targetStream;
+            std::mutex m_mutex;
+            std::string m_captured;
+            std::thread m_readThread;
 
-        UniqueFileDescriptor::UniqueFileDescriptor( int value ) noexcept:
+        public:
-            m_value{ value } {}
+            StreamPipeHandler( FILE* original ):
-
+                m_originalFd( dup_or_throw( fileno( original ) ) ),
-        constexpr UniqueFileDescriptor::UniqueFileDescriptor(
+                m_targetStream(original)
-            UniqueFileDescriptor&& other ) noexcept:
+            {
-            m_value{ other.m_value } {
+                CATCH_ENFORCE( m_originalFd >= 0, "Could not dup stream" );
-            other.m_value = 0;
+                int pipe_fds[2];
-        }
+                pipe_or_throw( pipe_fds );
-
+                m_pipeReadEnd = pipe_fds[0];
-        UniqueFileDescriptor::~UniqueFileDescriptor() noexcept {
+                m_pipeWriteEnd = pipe_fds[1];
-            if ( m_value == 0 ) { return; }
-
-            close_or_throw(
-                m_value ); // std::terminate on failure (due to noexcept)
-        }
-
-        UniqueFileDescriptor& UniqueFileDescriptor::operator=(
-            UniqueFileDescriptor&& other ) noexcept {
-            std::swap( m_value, other.m_value );
-            return *this;
-        }
-
-        constexpr int UniqueFileDescriptor::get() { return m_value; }
-
-        static 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] };
-        }
-
-        static inline void read_thread( UniqueFileDescriptor&& file,
-                                        std::string& result ) {
-            std::string buffer{};
-            constexpr size_t bufferSize{ 4096 };
-            buffer.resize( bufferSize );
-            size_t sizeRead{};
 
+                m_readThread = std::thread([this]() {
+                    constexpr size_t bufferSize = 4096;
+                    char buffer[bufferSize];
+                    size_t sizeRead;
                     while ( ( sizeRead = read_or_throw(
-                          file.get(), &buffer[0], bufferSize ) ) != 0 ) {
+                                  m_pipeReadEnd, buffer, bufferSize ) ) != 0 ) {
-                result.append( buffer.data(), sizeRead );
+                        std::unique_lock<std::mutex> _( m_mutex );
+                        m_captured.append( buffer, sizeRead );
                     }
+                });
             }
 
-        OutputFileRedirector::OutputFileRedirector( FILE* file,
+            ~StreamPipeHandler() {
-                                                    std::string& result ):
+                close_or_throw( m_pipeWriteEnd );
-            m_file{ file },
+                m_readThread.join();
-            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 );
             }
 
-        OutputFileRedirector::~OutputFileRedirector() noexcept {
+            std::string getCapturedOutput() {
-            fflush_or_throw(
+                std::unique_lock<std::mutex> _( m_mutex );
-                m_file ); // std::terminate on failure (due to noexcept)
+                return m_captured;
-
-            // 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(); }
             }
 
+            void clearCapturedOutput() {
+                std::unique_lock<std::mutex> _( m_mutex );
+                m_captured.clear();
+            }
+
+            void startCapture() {
+                fflush_or_throw( m_targetStream );
+                int ret = dup2_or_throw( m_pipeWriteEnd, fileno( m_targetStream ) );
+                CATCH_ENFORCE( ret >= 0,
+                               "dup2 pipe-write -> original stream failed " << errno );
+            }
+            void stopCapture() {
+                fflush_or_throw( m_targetStream );
+                int ret = dup2_or_throw( m_originalFd, fileno( m_targetStream ) );
+                CATCH_ENFORCE( ret >= 0,
+                               "dup2 of original fd -> original stream failed " << errno );
+            }
+        };
+
+        class PipeRedirect : public OutputRedirect {
+        private:
+            StreamPipeHandler m_stdout;
+            StreamPipeHandler m_stderr;
+        public:
+            PipeRedirect():
+                m_stdout(stdout),
+                m_stderr(stderr){}
+
+            void activateImpl() override {
+                m_stdout.startCapture();
+                m_stderr.startCapture();
+            }
+            void deactivateImpl() override {
+                m_stdout.stopCapture();
+                m_stderr.stopCapture();
+            }
+            std::string getStdout() override {
+                return m_stdout.getCapturedOutput();
+            }
+            std::string getStderr() override {
+                return m_stderr.getCapturedOutput();
+            }
+            void clearBuffers() override {
+                m_stdout.clearCapturedOutput();
+                m_stderr.clearCapturedOutput();
+            }
+        };
 
 #endif // CATCH_CONFIG_USE_ASYNC
 
@@ -518,7 +455,7 @@ namespace Catch {
             return Detail::make_unique<FileRedirect>();
 #else
             //return Detail::make_unique<StreamRedirect>();
-            return Detail::make_unique<PipeRedirectWrapper>();
+            return Detail::make_unique<PipeRedirect>();
 #endif
         } else {
             return Detail::make_unique<NoopRedirect>();