[ros-diffs] [reactos] 01/02: [ROSAUTOTEST] Implement a process activity timeout of 2 minutes. If there is no log output within 2 minutes, the test process is killed, and we continue with the next test.

[Colin Finck]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=7dd4d2256b97b1f8a3a39…

commit 7dd4d2256b97b1f8a3a392060592a3d0f715fbd7
Author:     Colin Finck <colin(a)reactos.org>
AuthorDate: Tue Apr 23 09:17:05 2019 +0200
Commit:     Timo Kreuzer <timo.kreuzer(a)reactos.org>
CommitDate: Fri Apr 26 08:47:15 2019 +0200

    [ROSAUTOTEST] Implement a process activity timeout of 2 minutes. If there is no log
output within 2 minutes, the test process is killed, and we continue with the next test.
    
    This is a rather graceful approach compared to sysreg2's 3 minute timeout before
killing and restarting the entire VM.
    Since we added autochk for FAT filesystems, the filesystem is often "fixed"
after a reset with the consequence that ReactOS doesn't boot up anymore.
    The sysreg2 restart code still remains for handling tests causing BSODs.
---
 modules/rostests/rosautotest/CPipe.cpp     | 101 +++++++++++++++++++++++++++--
 modules/rostests/rosautotest/CPipe.h       |   6 +-
 modules/rostests/rosautotest/CProcess.cpp  |   5 +-
 modules/rostests/rosautotest/CWineTest.cpp |  38 ++++++++---
 4 files changed, 131 insertions(+), 19 deletions(-)

diff --git a/modules/rostests/rosautotest/CPipe.cpp
b/modules/rostests/rosautotest/CPipe.cpp
index 8fd3cf03e1..5af7163971 100644
--- a/modules/rostests/rosautotest/CPipe.cpp
+++ b/modules/rostests/rosautotest/CPipe.cpp
@@ -3,10 +3,14 @@
  * LICENSE:     GPL-2.0+ (https://spdx.org/licenses/GPL-2.0+)
  * PURPOSE:     Class that manages an unidirectional anonymous byte stream pipe
  * COPYRIGHT:   Copyright 2015 Thomas Faber (thomas.faber(a)reactos.org)
+ *              Copyright 2019 Colin Finck (colin(a)reactos.org)
  */
 
 #include "precomp.h"
 
+LONG CPipe::m_lPipeCount = 0;
+
+
 /**
  * Constructs a CPipe object and initializes read and write handles.
  */
@@ -18,8 +22,50 @@ CPipe::CPipe()
     SecurityAttributes.bInheritHandle = TRUE;
     SecurityAttributes.lpSecurityDescriptor = NULL;
 
-    if(!CreatePipe(&m_hReadPipe, &m_hWritePipe, &SecurityAttributes, 0))
-        FATAL("CreatePipe failed\n");
+    // Construct a unique pipe name.
+    WCHAR wszPipeName[MAX_PATH];
+    InterlockedIncrement(&m_lPipeCount);
+    swprintf(wszPipeName, L"\\\\.\\pipe\\TestmanPipe%ld", m_lPipeCount);
+
+    // Create a named pipe with the default settings, but overlapped (asynchronous)
operations.
+    // Latter feature is why we can't simply use CreatePipe.
+    const DWORD dwDefaultBufferSize = 4096;
+    const DWORD dwDefaultTimeoutMilliseconds = 120000;
+
+    m_hReadPipe = CreateNamedPipeW(wszPipeName,
+        PIPE_ACCESS_INBOUND | FILE_FLAG_OVERLAPPED,
+        PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT,
+        1,
+        dwDefaultBufferSize,
+        dwDefaultBufferSize,
+        dwDefaultTimeoutMilliseconds,
+        &SecurityAttributes);
+    if (m_hReadPipe == INVALID_HANDLE_VALUE)
+    {
+        FATAL("CreateNamedPipe failed\n");
+    }
+
+    // Use CreateFileW to get the write handle to the pipe.
+    // Writing is done synchronously, so no FILE_FLAG_OVERLAPPED here!
+    m_hWritePipe = CreateFileW(wszPipeName,
+        GENERIC_WRITE,
+        0,
+        &SecurityAttributes,
+        OPEN_EXISTING,
+        FILE_ATTRIBUTE_NORMAL,
+        NULL);
+    if (m_hWritePipe == INVALID_HANDLE_VALUE)
+    {
+        FATAL("CreateFileW failed\n");
+    }
+
+    // Prepare the OVERLAPPED structure for reading.
+    ZeroMemory(&m_ReadOverlapped, sizeof(m_ReadOverlapped));
+    m_ReadOverlapped.hEvent = CreateEventW(NULL, TRUE, TRUE, NULL);
+    if (!m_ReadOverlapped.hEvent)
+    {
+        FATAL("CreateEvent failed\n");
+    }
 }
 
 /**
@@ -103,17 +149,60 @@ CPipe::Peek(PVOID Buffer, DWORD BufferSize, PDWORD BytesRead, PDWORD
TotalBytesA
  * 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.
+ * Returns a Win32 error code. Expected error codes include:
+ *   - ERROR_SUCCESS:     The read has completed successfully.
+ *   - WAIT_TIMEOUT:      The given timeout has elapsed before any data was read.
+ *   - ERROR_BROKEN_PIPE: The other end of the pipe has been closed.
  *
  * @see ReadFile
  */
-bool
-CPipe::Read(PVOID Buffer, DWORD NumberOfBytesToRead, PDWORD NumberOfBytesRead)
+DWORD
+CPipe::Read(PVOID Buffer, DWORD NumberOfBytesToRead, PDWORD NumberOfBytesRead, DWORD
TimeoutMilliseconds)
 {
     if (!m_hReadPipe)
+    {
         FATAL("Trying to read from a closed read pipe");
+    }
 
-    return ReadFile(m_hReadPipe, Buffer, NumberOfBytesToRead, NumberOfBytesRead, NULL);
+    if (ReadFile(m_hReadPipe, Buffer, NumberOfBytesToRead, NumberOfBytesRead,
&m_ReadOverlapped))
+    {
+        // The asynchronous read request could be satisfied immediately.
+        return ERROR_SUCCESS;
+    }
+    else if (GetLastError() == ERROR_IO_PENDING)
+    {
+        // The asynchronous read request could not be satisfied immediately, so wait for
it with the given timeout.
+        DWORD dwWaitResult = WaitForSingleObject(m_ReadOverlapped.hEvent,
TimeoutMilliseconds);
+        if (dwWaitResult == WAIT_OBJECT_0)
+        {
+            // Fill NumberOfBytesRead.
+            if (GetOverlappedResult(m_hReadPipe, &m_ReadOverlapped,
NumberOfBytesRead, FALSE))
+            {
+                // We successfully read NumberOfBytesRead bytes.
+                return ERROR_SUCCESS;
+            }
+            else if (GetLastError() == ERROR_BROKEN_PIPE)
+            {
+                // The other end of the pipe has been closed.
+                return ERROR_BROKEN_PIPE;
+            }
+            else
+            {
+                // An unexpected error.
+                FATAL("GetOverlappedResult failed\n");
+            }
+        }
+        else
+        {
+            // This may be WAIT_TIMEOUT or an unexpected error.
+            return dwWaitResult;
+        }
+    }
+    else
+    {
+        // This may be ERROR_BROKEN_PIPE or an unexpected error.
+        return GetLastError();
+    }
 }
 
 /**
diff --git a/modules/rostests/rosautotest/CPipe.h b/modules/rostests/rosautotest/CPipe.h
index 6efe670373..84bb01897f 100644
--- a/modules/rostests/rosautotest/CPipe.h
+++ b/modules/rostests/rosautotest/CPipe.h
@@ -3,11 +3,15 @@
  * LICENSE:     GPL-2.0+ (https://spdx.org/licenses/GPL-2.0+)
  * PURPOSE:     Class that manages an unidirectional anonymous byte stream pipe
  * COPYRIGHT:   Copyright 2015 Thomas Faber (thomas.faber(a)reactos.org)
+ *              Copyright 2019 Colin Finck (colin(a)reactos.org)
  */
 
 class CPipe
 {
 private:
+    static LONG m_lPipeCount;
+
+    OVERLAPPED m_ReadOverlapped;
     HANDLE m_hReadPipe;
     HANDLE m_hWritePipe;
 
@@ -19,7 +23,7 @@ public:
     void CloseWritePipe();
 
     bool Peek(PVOID Buffer, DWORD BufferSize, PDWORD BytesRead, PDWORD
TotalBytesAvailable);
-    bool Read(PVOID Buffer, DWORD NumberOfBytesToRead, PDWORD NumberOfBytesRead);
+    DWORD Read(PVOID Buffer, DWORD NumberOfBytesToRead, PDWORD NumberOfBytesRead, DWORD
TimeoutMilliseconds);
     bool Write(LPCVOID Buffer, DWORD NumberOfBytesToWrite, PDWORD NumberOfBytesWritten);
 
     friend class CPipedProcess;
diff --git a/modules/rostests/rosautotest/CProcess.cpp
b/modules/rostests/rosautotest/CProcess.cpp
index 65445cb379..e462456564 100644
--- a/modules/rostests/rosautotest/CProcess.cpp
+++ b/modules/rostests/rosautotest/CProcess.cpp
@@ -2,7 +2,7 @@
  * PROJECT:     ReactOS Automatic Testing Utility
  * LICENSE:     GPL-2.0+ (https://spdx.org/licenses/GPL-2.0+)
  * PURPOSE:     Class able to create a new process and closing its handles on destruction
(exception-safe)
- * COPYRIGHT:   Copyright 2009 Colin Finck (colin(a)reactos.org)
+ * COPYRIGHT:   Copyright 2009-2019 Colin Finck (colin(a)reactos.org)
  */
 
 #include "precomp.h"
@@ -27,10 +27,11 @@ CProcess::CProcess(const wstring& CommandLine, LPSTARTUPINFOW
StartupInfo)
 }
 
 /**
- * Destructs a CProcess object and closes all handles belonging to the process.
+ * Destructs a CProcess object, terminates the process if running, and closes all handles
belonging to the process.
  */
 CProcess::~CProcess()
 {
+    TerminateProcess(m_ProcessInfo.hProcess, 255);
     CloseHandle(m_ProcessInfo.hThread);
     CloseHandle(m_ProcessInfo.hProcess);
 }
diff --git a/modules/rostests/rosautotest/CWineTest.cpp
b/modules/rostests/rosautotest/CWineTest.cpp
index baf68a6b8a..6e163979e0 100644
--- a/modules/rostests/rosautotest/CWineTest.cpp
+++ b/modules/rostests/rosautotest/CWineTest.cpp
@@ -2,12 +2,13 @@
  * PROJECT:     ReactOS Automatic Testing Utility
  * LICENSE:     GPL-2.0+ (https://spdx.org/licenses/GPL-2.0+)
  * PURPOSE:     Class implementing functions for handling Wine tests
- * COPYRIGHT:   Copyright 2009-2015 Colin Finck (colin(a)reactos.org)
+ * COPYRIGHT:   Copyright 2009-2019 Colin Finck (colin(a)reactos.org)
  */
 
 #include "precomp.h"
 
 static const DWORD ListTimeout = 10000;
+static const DWORD ProcessActivityTimeout = 120000;
 
 /**
  * Constructs a CWineTest object.
@@ -140,7 +141,7 @@ CWineTest::DoListCommand()
     /* Read the data */
     m_ListBuffer = new char[BytesAvailable];
 
-    if(!Pipe.Read(m_ListBuffer, BytesAvailable, &Temp))
+    if(Pipe.Read(m_ListBuffer, BytesAvailable, &Temp, INFINITE) != ERROR_SUCCESS)
         TESTEXCEPTION("CPipe::Read failed\n");
 
     return BytesAvailable;
@@ -291,17 +292,34 @@ CWineTest::RunTest(CTestInfo* TestInfo)
         CPipedProcess Process(TestInfo->CommandLine, Pipe);
 
         /* Receive all the data from the pipe */
-        while(Pipe.Read(Buffer, sizeof(Buffer) - 1, &BytesAvailable) &&
BytesAvailable)
+        for (;;)
         {
-            /* Output text through StringOut, even while the test is still running */
-            Buffer[BytesAvailable] = 0;
-            tailString = StringOut(tailString.append(string(Buffer)), false);
+            DWORD dwReadResult = Pipe.Read(Buffer, sizeof(Buffer) - 1,
&BytesAvailable, ProcessActivityTimeout);
+            if (dwReadResult == ERROR_SUCCESS)
+            {
+                /* 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;
+                if (Configuration.DoSubmit())
+                    TestInfo->Log += Buffer;
+            }
+            else if (dwReadResult == ERROR_BROKEN_PIPE)
+            {
+                // The process finished and has been terminated.
+                break;
+            }
+            else if (dwReadResult == WAIT_TIMEOUT)
+            {
+                // The process activity timeout above has elapsed without any new data.
+                TESTEXCEPTION("Timeout while waiting for the test process\n");
+            }
+            else
+            {
+                // An unexpected error.
+                TESTEXCEPTION("CPipe::Read failed for the test run\n");
+            }
         }
-        if(GetLastError() != ERROR_BROKEN_PIPE)
-            TESTEXCEPTION("CPipe::Read failed for the test run\n");
     }
     catch(CTestException& e)
     {