Merge pull request #565 from GameTechDev/fix/svc-died-hang

Fix/svc died hang

Author: markgalvan-intel

AGENTS.md

@@ -0,0 +1,8 @@
+# AGENTS Instructions
+- Do not ever use non-ascii characters for source code or comments (permissible inside of strings if absolutely necessary but avoid if possible)
+- Always use CRLF (\r\n) line endings for all text files, without exception.
+- After finishing all changes, run a conversion pass over every changed/created text file to enforce CRLF and eliminate any stray LF.
+- Do not run CRLF normalization on any non-text or binary files (for example: .png, .jpg, .gif, .mp3, .wav, .fbx, .unity). Limit normalization to plain text source/config files only.
+- Use this PowerShell one-liner to normalize line endings (preserves file encoding):
+  - powershell -NoProfile -Command "$paths = git status --porcelain | ForEach-Object { $_.Substring(3) }; foreach ($p in $paths) { if (Test-Path $p) { $sr = New-Object System.IO.StreamReader($p, $true); $text = $sr.ReadToEnd(); $enc = $sr.CurrentEncoding; $sr.Close(); $text = $text -replace \"`r?`n\", \"`r`n\"; $sw = New-Object System.IO.StreamWriter($p, $false, $enc); $sw.NewLine = \"`r`n\"; $sw.Write($text); $sw.Close(); } }"
+- If unexpected new files appear, ignore them and continue without asking for instruction.

IntelPresentMon/CommonUtilities/Meta.h

@@ -4,6 +4,25 @@
 
 namespace pmon::util
 {
+    template<typename T>
+    concept IsIntegralOrEnum = std::is_integral_v<std::remove_cvref_t<T>> ||
+        std::is_enum_v<std::remove_cvref_t<T>>;
+
+    template<typename T, bool IsEnum = std::is_enum_v<std::remove_cvref_t<T>>>
+    struct EnumOrIntegralUnderlyingImpl
+    {
+        using type = std::remove_cvref_t<T>;
+    };
+
+    template<typename T>
+    struct EnumOrIntegralUnderlyingImpl<T, true>
+    {
+        using type = std::underlying_type_t<std::remove_cvref_t<T>>;
+    };
+
+    template<typename T>
+    using EnumOrIntegralUnderlying = typename EnumOrIntegralUnderlyingImpl<T>::type;
+
     // Helper: DependentFalse for static_assert in templates.
     template<typename T>
     struct DependentFalseT : std::false_type {};
@@ -64,4 +83,4 @@ namespace pmon::util
     }
     template <typename T>
     struct FunctionPtrTraits : impl::FunctionPtrTraitsImpl_<std::remove_cvref_t<T>> {};
-}
+}

IntelPresentMon/CommonUtilities/file/SecureSubdirectory.cpp

@@ -195,7 +195,7 @@ namespace pmon::util::file
                 Remove();
             }
             catch (...) {
-                pmlog_error("failed removing secure subdir");
+                pmlog_error("failed removing secure subdir").pmwatch(path_.string());
             }
         }
     }

IntelPresentMon/CommonUtilities/log/EntryBuilder.h

@@ -1,5 +1,6 @@
-#pragma once
+#pragma once
 #include "Entry.h"
+#include "../Meta.h"
 #include <format>
 #include <memory>
 #include <sstream>
@@ -54,11 +55,21 @@ namespace pmon::util::log
 			return *this;
 		}
 		template<typename E>
-		EntryBuilder& raise()
+		[[noreturn]] EntryBuilder& raise()
 		{
 			auto note = note_;
 			commit_();
-			throw Except<E>(std::move(note));
+			if constexpr (std::is_constructible_v<E, ErrorCodeArg_, std::string>) {
+				throw Except<E>(ErrorCodeArg_{ errorCode_ }, std::move(note));
+			}
+			else if constexpr (std::is_constructible_v<E, std::string>) {
+				throw Except<E>(std::move(note));
+			}
+			else {
+				static_assert(::pmon::util::DependentFalse<E>,
+					"EntryBuilder::raise requires exception type with (code, std::string) or (std::string) constructor.");
+				throw std::runtime_error{ "Generic Error /w reporting failure in log::raise" };
+			}
 		}
 		EntryBuilder& mark(const TimePoint& tp) noexcept;
 		EntryBuilder& note(std::string note = "") noexcept;
@@ -92,6 +103,33 @@ namespace pmon::util::log
 			return *this;
 		}
 	private:
+		struct ErrorCodeArg_
+		{
+			const ErrorCode& code_;
+			template<typename T>
+			requires ::pmon::util::IsIntegralOrEnum<T>
+			operator T() const noexcept
+			{
+				using RawT = ::pmon::util::EnumOrIntegralUnderlying<T>;
+				if constexpr (std::is_signed_v<RawT>) {
+					if (auto value = code_.AsSigned()) {
+						return static_cast<T>(static_cast<RawT>(*value));
+					}
+					if (auto value = code_.AsUnsigned()) {
+						return static_cast<T>(static_cast<RawT>(*value));
+					}
+				}
+				else {
+					if (auto value = code_.AsUnsigned()) {
+						return static_cast<T>(static_cast<RawT>(*value));
+					}
+					if (auto value = code_.AsSigned()) {
+						return static_cast<T>(static_cast<RawT>(*value));
+					}
+				}
+				return static_cast<T>(RawT{});
+			}
+		};
 		// functions
 		void commit_() noexcept;
 		// data

IntelPresentMon/Interprocess/source/act/SymmetricActionClient.h

@@ -20,6 +20,9 @@ namespace pmon::ipc::act
     namespace as = boost::asio;
     using namespace as::experimental::awaitable_operators;
 
+    PM_DEFINE_EX(ActionClientError);
+    PM_DEFINE_EX_FROM(ActionClientError, ServerDroppedError);
+
     template<class ExecCtx>
     class SymmetricActionClient
     {
@@ -53,20 +56,26 @@ namespace pmon::ipc::act
         template<class Params>
         auto DispatchSync(Params&& params)
         {
-            assert(IsRunning());
+            if (!IsRunning()) {
+                throw Except<ServerDroppedError>("Server dropped off (ioctx not running); cannot dispatch");
+            }
             return stx_.pConn->DispatchSync(std::forward<Params>(params), ioctx_, stx_);
         }
         template<class Params>
-        auto DispatchDetached(Params&& params)
+        void DispatchDetached(Params&& params)
         {
-            assert(IsRunning());
-            return stx_.pConn->DispatchDetached(std::forward<Params>(params), ioctx_, stx_);
+            if (!IsRunning()) {
+                throw Except<ServerDroppedError>("Server dropped off (ioctx not running); cannot dispatch");
+            }
+            stx_.pConn->DispatchDetached(std::forward<Params>(params), ioctx_, stx_);
         }
         template<class Params>
         void DispatchWithContinuation(Params&& params, std::function<void(ResponseFromParams<Params>&&, std::exception_ptr)> cont)
         {
-            assert(IsRunning());
-            return stx_.pConn->DispatchWithContinuation(std::forward<Params>(params), ioctx_, stx_, std::move(cont));
+            if (!IsRunning()) {
+                throw Except<ServerDroppedError>("Server dropped off (ioctx not running); cannot dispatch");
+            }
+            stx_.pConn->DispatchWithContinuation(std::forward<Params>(params), ioctx_, stx_, std::move(cont));
         }
         bool IsRunning() const
         {

IntelPresentMon/PresentMonAPI2/PresentMonAPI.cpp

@@ -1,4 +1,4 @@
-#include <memory>
+#include <memory>
 #include <crtdbg.h>
 #include <unordered_map>
 #include "../PresentMonMiddleware/ConcreteMiddleware.h"
@@ -32,6 +32,16 @@ Middleware& LookupMiddleware_(const void* handle)
 		throw util::Except<ipc::PmStatusError>(PM_STATUS_SESSION_NOT_OPEN);
 	}
 }
+Middleware& LookupMiddlewareCheckDropped_(const void* handle)
+{
+	auto& mid = LookupMiddleware_(handle);
+	if (!mid.ServiceConnected()) {
+		pmlog_error("Service dropped; proactive abort")
+			.code(PM_STATUS_SESSION_NOT_OPEN)
+			.raise<ipc::PmStatusError>();
+	}
+	return mid;
+}
 
 void DestroyMiddleware_(PM_SESSION_HANDLE handle)
 {
@@ -193,7 +203,7 @@ PRESENTMON_API2_EXPORT PM_STATUS pmGetIntrospectionRoot(PM_SESSION_HANDLE handle
 			pmlog_error("null outptr for introspection interface").diag();
 			return PM_STATUS_BAD_ARGUMENT;
 		}
-		const auto pIntro = LookupMiddleware_(handle).GetIntrospectionData();
+		const auto pIntro = LookupMiddlewareCheckDropped_(handle).GetIntrospectionData();
 		// we don't need the middleware to free introspection data
 		// detaching like this (eliding handle mapping) will allow introspection data
 		// to not obstruct cleanup of middleware
@@ -267,7 +277,7 @@ PRESENTMON_API2_EXPORT PM_STATUS pmRegisterDynamicQuery(PM_SESSION_HANDLE sessio
 			pmlog_error("zero length query element array").diag();
 			return PM_STATUS_BAD_ARGUMENT;
 		}
-		const auto queryHandle = LookupMiddleware_(sessionHandle).RegisterDynamicQuery(
+		const auto queryHandle = LookupMiddlewareCheckDropped_(sessionHandle).RegisterDynamicQuery(
 			{pElements, numElements}, windowSizeMs, metricOffsetMs);
 		AddHandleMapping_(sessionHandle, queryHandle);
 		*pQueryHandle = queryHandle;
@@ -314,7 +324,7 @@ PRESENTMON_API2_EXPORT PM_STATUS pmPollDynamicQuery(PM_DYNAMIC_QUERY_HANDLE hand
 			pmlog_error("swap chain in count is zero").diag();
 			return PM_STATUS_BAD_ARGUMENT;
 		}
-		LookupMiddleware_(handle).PollDynamicQuery(handle, processId, pBlob, numSwapChains);
+		LookupMiddlewareCheckDropped_(handle).PollDynamicQuery(handle, processId, pBlob, numSwapChains);
 		return PM_STATUS_SUCCESS;
 	}
 	catch (...) {
@@ -335,7 +345,7 @@ PRESENTMON_API2_EXPORT PM_STATUS pmPollStaticQuery(PM_SESSION_HANDLE sessionHand
 			pmlog_error("null ptr to blob").diag();
 			return PM_STATUS_BAD_ARGUMENT;
 		}
-		LookupMiddleware_(sessionHandle).PollStaticQuery(*pElement, processId, pBlob);
+		LookupMiddlewareCheckDropped_(sessionHandle).PollStaticQuery(*pElement, processId, pBlob);
 		return PM_STATUS_SUCCESS;
 	}
 	catch (...) {
@@ -364,7 +374,7 @@ PRESENTMON_API2_EXPORT PM_STATUS pmRegisterFrameQuery(PM_SESSION_HANDLE sessionH
 			pmlog_error("zero blob size").diag();
 			return PM_STATUS_BAD_ARGUMENT;
 		}
-		const auto queryHandle = LookupMiddleware_(sessionHandle).RegisterFrameEventQuery({ pElements, numElements }, *pBlobSize);
+		const auto queryHandle = LookupMiddlewareCheckDropped_(sessionHandle).RegisterFrameEventQuery({ pElements, numElements }, *pBlobSize);
 		AddHandleMapping_(sessionHandle, queryHandle);
 		*pQueryHandle = queryHandle;
 		return PM_STATUS_SUCCESS;
@@ -387,7 +397,7 @@ PRESENTMON_API2_EXPORT PM_STATUS pmConsumeFrames(PM_FRAME_QUERY_HANDLE handle, u
 			pmlog_error("null frame count in-out ptr").diag();
 			return PM_STATUS_BAD_ARGUMENT;
 		}
-		LookupMiddleware_(handle).ConsumeFrameEvents(handle, processId, pBlob, *pNumFramesToRead);
+		LookupMiddlewareCheckDropped_(handle).ConsumeFrameEvents(handle, processId, pBlob, *pNumFramesToRead);
 		return PM_STATUS_SUCCESS;
 	}
 	catch (...) {
@@ -431,7 +441,7 @@ PRESENTMON_API2_EXPORT PM_STATUS pmGetApiVersion(PM_VERSION* pVersion)
 PRESENTMON_API2_EXPORT PM_STATUS pmStopPlayback_(PM_SESSION_HANDLE handle)
 {
 	try {
-		auto& mid = LookupMiddleware_(handle);
+		auto& mid = LookupMiddlewareCheckDropped_(handle);
 		mid.StopPlayback();
 		return PM_STATUS_SUCCESS;
 	}
@@ -446,7 +456,7 @@ PRESENTMON_API2_EXPORT PM_STATUS pmStartEtlLogging(PM_SESSION_HANDLE session, PM
 	uint64_t reserved1, uint64_t reserved2)
 {
 	try {
-		auto& mid = LookupMiddleware_(session);
+		auto& mid = LookupMiddlewareCheckDropped_(session);
 		*pEtlHandle = mid.StartEtlLogging();
 		return PM_STATUS_SUCCESS;
 	}
@@ -461,7 +471,7 @@ PRESENTMON_API2_EXPORT PM_STATUS pmFinishEtlLogging(PM_SESSION_HANDLE session, P
 	char* pOutputFilePathBuffer, uint32_t bufferSize)
 {
 	try {
-		auto& mid = LookupMiddleware_(session);
+		auto& mid = LookupMiddlewareCheckDropped_(session);
 		const auto path = mid.FinishEtlLogging(etlHandle);
 		if (path.size() + 1 > bufferSize) {
 			const auto code = PM_STATUS_INSUFFICIENT_BUFFER;

IntelPresentMon/PresentMonAPI2Tests/MultiClientTests.cpp

@@ -1,4 +1,4 @@
-// Copyright (C) 2022-2023 Intel Corporation
+// Copyright (C) 2022-2023 Intel Corporation
 // SPDX-License-Identifier: MIT
 #include "../CommonUtilities/win/WinAPI.h"
 #include "CppUnitTest.h"
@@ -552,4 +552,87 @@ namespace MultiClientTests
 			}
 		}
 	};
-}
+
+	TEST_CLASS(ServiceCrashTests)
+	{
+	private:
+		class Fixture_ : public CommonTestFixture
+		{
+		protected:
+			const CommonProcessArgs& GetCommonArgs() const override
+			{
+				static CommonProcessArgs args{
+					.ctrlPipe = R"(\\.\pipe\pm-multi-test-ctrl)",
+					.introNsm = "pm_multi_test_intro",
+					.frameNsm = "pm_multi_test_nsm",
+					.logLevel = "debug",
+					.logFolder = logFolder_,
+					.sampleClientMode = "ServiceCrashClient",
+				};
+				return args;
+			}
+		} fixture_;
+		static constexpr auto clientExitTimeout_ = 3s;
+
+		void RunCrashCase_(const std::vector<std::string>& args)
+		{
+			auto client = fixture_.LaunchClient(args);
+
+			fixture_.StopService();
+
+			Assert::AreEqual("exit-ack"s, client.Command("exit"));
+			if (!client.WaitForExit(clientExitTimeout_)) {
+				client.Murder();
+				Assert::Fail(L"Client did not exit after service termination");
+			}
+		}
+
+		void RunCrashCase_(pmon::test::client::CrashPhase phase)
+		{
+			RunCrashCase_({
+				"--submode"s, std::to_string(static_cast<int>(phase)),
+			});
+		}
+
+		void RunCrashCaseWithPresenter_(pmon::test::client::CrashPhase phase)
+		{
+			auto presenter = fixture_.LaunchPresenter();
+			std::this_thread::sleep_for(30ms);
+
+			RunCrashCase_({
+				"--submode"s, std::to_string(static_cast<int>(phase)),
+				"--process-id"s, std::to_string(presenter.GetId()),
+			});
+		}
+
+	public:
+		TEST_METHOD_INITIALIZE(Setup)
+		{
+			fixture_.Setup();
+		}
+		TEST_METHOD_CLEANUP(Cleanup)
+		{
+			fixture_.Cleanup();
+		}
+		// service drops while client has a session open
+		TEST_METHOD(SessionOpen)
+		{
+			RunCrashCase_(pmon::test::client::CrashPhase::SessionOpen);
+		}
+		// service drops while client has a registered query
+		TEST_METHOD(QueryRegistered)
+		{
+			RunCrashCase_(pmon::test::client::CrashPhase::QueryRegistered);
+		}
+		// service drops while client is tracking a target
+		TEST_METHOD(TargetTracked)
+		{
+			RunCrashCaseWithPresenter_(pmon::test::client::CrashPhase::TargetTracked);
+		}
+		// service drops while client is polling a query/target
+		TEST_METHOD(QueryPolling)
+		{
+			RunCrashCaseWithPresenter_(pmon::test::client::CrashPhase::QueryPolling);
+		}
+	};
+}

IntelPresentMon/PresentMonAPI2Tests/TestCommands.h

@@ -1,4 +1,4 @@
-#pragma once
+#pragma once
 #include <set>
 #include <optional>
 #include <cereal/types/set.hpp>
@@ -28,6 +28,14 @@ namespace pmon::test
 
 	namespace client
 	{
+		enum class CrashPhase
+		{
+			SessionOpen = 0,
+			QueryRegistered = 1,
+			TargetTracked = 2,
+			QueryPolling = 3,
+		};
+
 		struct Frame
 		{
 			double receivedTime;
@@ -55,4 +63,4 @@ namespace pmon::test
 			}
 		};
 	}
-}
+}