[ROSAUTOTEST]

- Abstract unidirectional anonymous pipes into a CPipe class
- Abstract a process with redirected output into a CPipedProcess class
- Use these abstractions to avoid polling for output from test processes. Instead, use blocking read operations to yield the CPU while waiting for data.
ROSTESTS-144 #resolve

svn path=/trunk/; revision=66316

rostests/rosautotest/CMakeLists.txt

@@ -6,6 +6,8 @@ list(APPEND SOURCE
     CFatalException.cpp
     CInvalidParameterException.cpp
     CJournaledTestList.cpp
+    CPipe.cpp
+    CPipedProcess.cpp
     CProcess.cpp
     CSimpleException.cpp
     CTest.cpp

rostests/rosautotest/CPipe.cpp

@@ -0,0 +1,143 @@
+/*
+ * PROJECT:     ReactOS Automatic Testing Utility
+ * LICENSE:     GNU GPLv2 or any later version as published by the Free Software Foundation
+ * PURPOSE:     Class that managed an unidirectional anonymous byte stream pipe
+ * COPYRIGHT:   Copyright 2015 Thomas Faber <thomas.faber@reactos.org>
+ */
+
+#include "precomp.h"
+
+/**
+ * Constructs a CPipe object and initializes read and write handles.
+ */
+CPipe::CPipe()
+{
+    SECURITY_ATTRIBUTES SecurityAttributes;
+
+    SecurityAttributes.nLength = sizeof(SecurityAttributes);
+    SecurityAttributes.bInheritHandle = TRUE;
+    SecurityAttributes.lpSecurityDescriptor = NULL;
+
+    if(!CreatePipe(&m_hReadPipe, &m_hWritePipe, &SecurityAttributes, 0))
+        FATAL("CreatePipe failed\n");
+}
+
+/**
+ * Destructs a CPipe object and closes all open handles.
+ */
+CPipe::~CPipe()
+{
+    if (m_hReadPipe)
+        CloseHandle(m_hReadPipe);
+    if (m_hWritePipe)
+        CloseHandle(m_hWritePipe);
+}
+
+/**
+ * Closes a CPipe's read pipe, leaving the write pipe unchanged.
+ */
+void
+CPipe::CloseReadPipe()
+{
+    if (!m_hReadPipe)
+        FATAL("Trying to close already closed read pipe");
+    CloseHandle(m_hReadPipe);
+}
+
+/**
+ * Closes a CPipe's write pipe, leaving the read pipe unchanged.
+ */
+void
+CPipe::CloseWritePipe()
+{
+    if (!m_hWritePipe)
+        FATAL("Trying to close already closed write pipe");
+    CloseHandle(m_hWritePipe);
+}
+
+/**
+ * Reads data from a pipe without advancing the read offset and/or retrieves information about available data.
+ *
+ * This function must not be called after CloseReadPipe.
+ *
+ * @param Buffer
+ * An optional buffer to read pipe data into.
+ *
+ * @param BufferSize
+ * The size of the buffer specified in Buffer, or 0 if no read should be performed.
+ *
+ * @param BytesRead
+ * On return, the number of bytes actually read from the pipe into Buffer.
+ *
+ * @param TotalBytesAvailable
+ * On return, the total number of bytes available to read from the pipe.
+ *
+ * @return
+ * True on success, false on failure; call GetLastError for error information.
+ *
+ * @see PeekNamedPipe
+ */
+bool
+CPipe::Peek(PVOID Buffer, DWORD BufferSize, PDWORD BytesRead, PDWORD TotalBytesAvailable)
+{
+    if (!m_hReadPipe)
+        FATAL("Trying to peek from a closed read pipe");
+
+    return PeekNamedPipe(m_hReadPipe, Buffer, BufferSize, BytesRead, TotalBytesAvailable, NULL);
+}
+
+/**
+ * Reads data from the read pipe, advancing the read offset accordingly.
+ *
+ * This function must not be called after CloseReadPipe.
+ *
+ * @param Buffer
+ * Buffer to read pipe data into.
+ *
+ * @param NumberOfBytesToRead
+ * The number of bytes to read into Buffer.
+ *
+ * @param NumberOfBytesRead
+ * On return, the number of bytes actually read from the pipe into Buffer.
+ *
+ * @return
+ * True on success, false on failure; call GetLastError for error information.
+ *
+ * @see ReadFile
+ */
+bool
+CPipe::Read(PVOID Buffer, DWORD NumberOfBytesToRead, PDWORD NumberOfBytesRead)
+{
+    if (!m_hReadPipe)
+        FATAL("Trying to read from a closed read pipe");
+
+    return ReadFile(m_hReadPipe, Buffer, NumberOfBytesToRead, NumberOfBytesRead, NULL);
+}
+
+/**
+ * Writes data to the write pipe.
+ *
+ * This function must not be called after CloseWritePipe.
+ *
+ * @param Buffer
+ * Buffer containing the data to write.
+ *
+ * @param NumberOfBytesToWrite
+ * The number of bytes to write to the pipe from Buffer.
+ *
+ * @param NumberOfBytesWritten
+ * On return, the number of bytes actually written to the pipe.
+ *
+ * @return
+ * True on success, false on failure; call GetLastError for error information.
+ *
+ * @see WriteFile
+ */
+bool
+CPipe::Write(LPCVOID Buffer, DWORD NumberOfBytesToWrite, PDWORD NumberOfBytesWritten)
+{
+    if (!m_hWritePipe)
+        FATAL("Trying to write to a closed write pipe");
+
+    return WriteFile(m_hWritePipe, Buffer, NumberOfBytesToWrite, NumberOfBytesWritten, NULL);
+}

rostests/rosautotest/CPipe.h

@@ -0,0 +1,26 @@
+/*
+ * PROJECT:     ReactOS Automatic Testing Utility
+ * LICENSE:     GNU GPLv2 or any later version as published by the Free Software Foundation
+ * PURPOSE:     Class that managed an unidirectional anonymous byte stream pipe
+ * COPYRIGHT:   Copyright 2015 Thomas Faber <thomas.faber@reactos.org>
+ */
+
+class CPipe
+{
+private:
+    HANDLE m_hReadPipe;
+    HANDLE m_hWritePipe;
+
+public:
+    CPipe();
+    ~CPipe();
+
+    void CloseReadPipe();
+    void CloseWritePipe();
+
+    bool Peek(PVOID Buffer, DWORD BufferSize, PDWORD BytesRead, PDWORD TotalBytesAvailable);
+    bool Read(PVOID Buffer, DWORD NumberOfBytesToRead, PDWORD NumberOfBytesRead);
+    bool Write(LPCVOID Buffer, DWORD NumberOfBytesToWrite, PDWORD NumberOfBytesWritten);
+
+    friend class CPipedProcess;
+};

rostests/rosautotest/CPipedProcess.cpp

@@ -0,0 +1,42 @@
+/*
+ * PROJECT:     ReactOS Automatic Testing Utility
+ * LICENSE:     GNU GPLv2 or any later version as published by the Free Software Foundation
+ * PURPOSE:     Class that creates a process and redirects its output to a pipe
+ * COPYRIGHT:   Copyright 2015 Thomas Faber <thomas.faber@reactos.org>
+ */
+
+#include "precomp.h"
+
+/**
+ * Constructs a CPipedProcess object and starts the process with redirected output.
+ *
+ * @param CommandLine
+ * A std::wstring containing the command line to run.
+ *
+ * @param Pipe
+ * The CPipe instance to redirect the process's output to.
+ * Note that only the read pipe is usable after the pipe was passed to this object.
+ */
+CPipedProcess::CPipedProcess(const wstring& CommandLine, CPipe& Pipe)
+    : CProcess(CommandLine, InitStartupInfo(Pipe))
+{
+    Pipe.CloseWritePipe();
+}
+
+/**
+ * Initializes the STARTUPINFO structure for use in CreateProcessW.
+ *
+ * @param Pipe
+ * The CPipe instance to redirect the process's output to.
+ */
+LPSTARTUPINFOW
+CPipedProcess::InitStartupInfo(CPipe& Pipe)
+{
+    ZeroMemory(&m_StartupInfo, sizeof(m_StartupInfo));
+    m_StartupInfo.cb = sizeof(m_StartupInfo);
+    m_StartupInfo.dwFlags = STARTF_USESTDHANDLES;
+    m_StartupInfo.hStdInput  = GetStdHandle(STD_INPUT_HANDLE);
+    m_StartupInfo.hStdOutput = Pipe.m_hWritePipe;
+    m_StartupInfo.hStdError  = Pipe.m_hWritePipe;
+    return &m_StartupInfo;
+}

rostests/rosautotest/CPipedProcess.h

@@ -0,0 +1,17 @@
+/*
+ * PROJECT:     ReactOS Automatic Testing Utility
+ * LICENSE:     GNU GPLv2 or any later version as published by the Free Software Foundation
+ * PURPOSE:     Class that creates a process and redirects its output to a pipe
+ * COPYRIGHT:   Copyright 2015 Thomas Faber <thomas.faber@reactos.org>
+ */
+
+class CPipedProcess : public CProcess
+{
+private:
+    STARTUPINFOW m_StartupInfo;
+
+    LPSTARTUPINFOW InitStartupInfo(CPipe& Pipe);
+
+public:
+    CPipedProcess(const wstring& CommandLine, CPipe& Pipe);
+};

rostests/rosautotest/CWineTest.cpp

@@ -18,10 +18,7 @@ CWineTest::CWineTest()
 
     /* Zero-initialize variables */
     m_hFind = NULL;
-    m_hReadPipe = NULL;
-    m_hWritePipe = NULL;
     m_ListBuffer = NULL;
-    memset(&m_StartupInfo, 0, sizeof(m_StartupInfo));
 
     /* Set up m_TestPath */
     if(!GetWindowsDirectoryW(WindowsDirectory, MAX_PATH))
@@ -39,12 +36,6 @@ CWineTest::~CWineTest()
     if(m_hFind)
         FindClose(m_hFind);
 
-    if(m_hReadPipe)
-        CloseHandle(m_hReadPipe);
-
-    if(m_hWritePipe)
-        CloseHandle(m_hWritePipe);
-
     if(m_ListBuffer)
         delete m_ListBuffer;
 }
@@ -111,6 +102,7 @@ CWineTest::DoListCommand()
     DWORD BytesAvailable;
     DWORD Temp;
     wstring CommandLine;
+    CPipe Pipe;
 
     /* Build the command line */
     CommandLine = m_TestPath;
@@ -119,7 +111,7 @@ CWineTest::DoListCommand()
 
     {
         /* Start the process for getting all available tests */
-        CProcess Process(CommandLine, &m_StartupInfo);
+        CPipedProcess Process(CommandLine, Pipe);
 
         /* Wait till this process ended */
         if(WaitForSingleObject(Process.GetProcessHandle(), ListTimeout) == WAIT_FAILED)
@@ -127,8 +119,8 @@ CWineTest::DoListCommand()
     }
 
     /* Read the output data into a buffer */
-    if(!PeekNamedPipe(m_hReadPipe, NULL, 0, NULL, &BytesAvailable, NULL))
+    if(!Pipe.Peek(NULL, 0, NULL, &BytesAvailable))
-        FATAL("PeekNamedPipe failed for the test list\n");
+        FATAL("CPipe::Peek failed for the test list\n");
 
     /* Check if we got any */
     if(!BytesAvailable)
@@ -142,8 +134,8 @@ CWineTest::DoListCommand()
     /* Read the data */
     m_ListBuffer = new char[BytesAvailable];
 
-    if(!ReadFile(m_hReadPipe, m_ListBuffer, BytesAvailable, &Temp, NULL))
+    if(!Pipe.Read(m_ListBuffer, BytesAvailable, &Temp))
-        FATAL("ReadPipe failed\n");
+        FATAL("CPipe::Read failed\n");
 
     return BytesAvailable;
 }
@@ -260,13 +252,13 @@ CWineTest::GetNextTestInfo()
 void
 CWineTest::RunTest(CTestInfo* TestInfo)
 {
-    bool BreakLoop = false;
     DWORD BytesAvailable;
-    DWORD Temp;
     stringstream ss, ssFinish;
     DWORD StartTime = GetTickCount();
     float TotalTime;
     string tailString;
+    CPipe Pipe;
+    char Buffer[1024];
 
     ss << "Running Wine Test, Module: " << TestInfo->Module << ", Test: " << TestInfo->Test << endl;
     StringOut(ss.str());
@@ -275,40 +267,20 @@ CWineTest::RunTest(CTestInfo* TestInfo)
 
     {
         /* Execute the test */
-        CProcess Process(TestInfo->CommandLine, &m_StartupInfo);
+        CPipedProcess Process(TestInfo->CommandLine, Pipe);
 
         /* Receive all the data from the pipe */
-        do
+        while(Pipe.Read(Buffer, sizeof(Buffer) - 1, &BytesAvailable) && BytesAvailable)
         {
-            /* When the application finished, make sure that we peek the pipe one more time, so that we get all data.
+            /* Output text through StringOut, even while the test is still running */
-               If the following condition would be the while() condition, we might hit a race condition:
+            Buffer[BytesAvailable] = 0;
-                  - We check for data with PeekNamedPipe -> no data available
+            tailString = StringOut(tailString.append(string(Buffer)), false);
-                  - The application outputs its data and finishes
-                  - WaitForSingleObject reports that the application has finished and we break the loop without receiving any data
-            */
-            if(WaitForSingleObject(Process.GetProcessHandle(), 0) != WAIT_TIMEOUT)
-                BreakLoop = true;
 
-            if(!PeekNamedPipe(m_hReadPipe, NULL, 0, NULL, &BytesAvailable, NULL))
+            if(Configuration.DoSubmit())
-                FATAL("PeekNamedPipe failed for the test run\n");
+                TestInfo->Log += Buffer;
-
-            if(BytesAvailable)
-            {
-                /* There is data, so get it and output it */
-                auto_array_ptr<char> Buffer(new char[BytesAvailable + 1]);
-
-                if(!ReadFile(m_hReadPipe, Buffer, BytesAvailable, &Temp, NULL))
-                    FATAL("ReadFile failed for the test run\n");
-
-                /* Output text through StringOut, even while the test is still running */
-                Buffer[BytesAvailable] = 0;
-                tailString = StringOut(tailString.append(string(Buffer)), false);
-
-                if(Configuration.DoSubmit())
-                    TestInfo->Log += Buffer;
-            }
         }
-        while(!BreakLoop);
+        if(GetLastError() != ERROR_BROKEN_PIPE)
+            FATAL("CPipe::Read failed for the test run\n");
     }
 
     /* Print what's left */
@@ -330,21 +302,6 @@ CWineTest::Run()
     auto_ptr<CTestList> TestList;
     auto_ptr<CWebService> WebService;
     CTestInfo* TestInfo;
-    SECURITY_ATTRIBUTES SecurityAttributes;
-
-    /* Create a pipe for getting the output of the tests */
-    SecurityAttributes.nLength = sizeof(SecurityAttributes);
-    SecurityAttributes.bInheritHandle = TRUE;
-    SecurityAttributes.lpSecurityDescriptor = NULL;
-
-    if(!CreatePipe(&m_hReadPipe, &m_hWritePipe, &SecurityAttributes, 0))
-        FATAL("CreatePipe failed\n");
-
-    m_StartupInfo.cb = sizeof(m_StartupInfo);
-    m_StartupInfo.dwFlags = STARTF_USESTDHANDLES;
-    m_StartupInfo.hStdInput  = GetStdHandle(STD_INPUT_HANDLE);
-    m_StartupInfo.hStdOutput = m_hWritePipe;
-    m_StartupInfo.hStdError  = m_hWritePipe;
 
     /* The virtual test list is of course faster, so it should be preferred over
        the journaled one.

rostests/rosautotest/CWineTest.h

@@ -9,10 +9,7 @@ class CWineTest : public CTest
 {
 private:
     HANDLE m_hFind;
-    HANDLE m_hReadPipe;
-    HANDLE m_hWritePipe;
     PCHAR m_ListBuffer;
-    STARTUPINFOW m_StartupInfo;
     string m_CurrentTest;
     wstring m_CurrentFile;
     wstring m_CurrentListCommand;

rostests/rosautotest/precomp.h

@@ -30,7 +30,9 @@ using namespace std;
 #include "CConfiguration.h"
 #include "CFatalException.h"
 #include "CInvalidParameterException.h"
+#include "CPipe.h"
 #include "CProcess.h"
+#include "CPipedProcess.h"
 #include "CSimpleException.h"
 #include "CTestInfo.h"
 #include "CTest.h"