From c08889585bf3fdb05093b2713ef00402835fb51d Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 18 Dec 2020 15:21:53 -0600 Subject: [PATCH] last commit before the holidays --- src/cascadia/Remoting/Monarch.cpp | 9 +- src/cascadia/Remoting/WindowManager.cpp | 124 ++++++++---------- src/cascadia/Remoting/WindowManager.h | 6 +- .../UnitTests_Remoting/RemotingTests.cpp | 2 +- 4 files changed, 61 insertions(+), 80 deletions(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 9792f5f1ec6..591b5d177c4 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -63,13 +63,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // Add an event listener to the peasant's WindowActivated event. peasant.WindowActivated({ this, &Monarch::_peasantWindowActivated }); - // TODO:projects/5 Wait on the peasant's PID, and remove them from the - // map if they die. This won't work great in tests though, with fake - // PIDs. - // - // We should trigger a callback. The manager will use this callback as - // an opportunity to start waiting on the new peasant. - return newPeasantsId; } @@ -115,7 +108,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation catch (...) { LOG_CAUGHT_EXCEPTION(); - // TODO: Remove the peasant from the list of peasants + // Remove the peasant from the list of peasants _peasants.erase(peasantID); return nullptr; } diff --git a/src/cascadia/Remoting/WindowManager.cpp b/src/cascadia/Remoting/WindowManager.cpp index 10bf4831465..cb8c124b52a 100644 --- a/src/cascadia/Remoting/WindowManager.cpp +++ b/src/cascadia/Remoting/WindowManager.cpp @@ -16,11 +16,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation WindowManager::WindowManager() { _monarchWaitInterrupt.create(); - // _peasantListenerInterrupt.create(); - - // wil::unique_event peasantListenerInterrupt; - // peasantListenerInterrupt.create(); - // _peasantHandles.emplace_back(std::move(peasantListenerInterrupt)); // Register with COM as a server for the Monarch class _registerAsMonarch(); @@ -37,15 +32,11 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation CoRevokeClassObject(_registrationHostClass); _registrationHostClass = 0; _monarchWaitInterrupt.SetEvent(); - // _peasantListenerInterrupt.SetEvent(); + if (_electionThread.joinable()) { _electionThread.join(); } - // if (_peasantListenerThread.joinable()) - // { - // _peasantListenerThread.join(); - // } } void WindowManager::ProposeCommandline(const Remoting::CommandlineArgs& args) @@ -113,20 +104,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation } // Here, we're the king! - // TODO:MG Add an even handler to the monarch's PeasantAdded event. - // We'll use that callback as a chance to start waiting on the peasant's - // PID. If they die, we'll tell the monarch to remove them from the - // list. - // _peasantHandles.emplace_back(_peasantListenerInterrupt.get()); - - // _peasantListenerThread = std::thread([this]() { - - // bool exitRequested = false; - // while (!exitRequested) - // { - // } - // }); - // Wait, don't. Let's just have the monarch try/catch any accesses to // peasants. If the peasant dies, then it can't get the peasant's // anything. In that case, _remove it_. @@ -151,6 +128,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation bool WindowManager::_electionNight2020() { _createMonarchAndCallbacks(); + + // Tell the new monarch who we are. We might be that monarch! + _monarch.AddPeasant(_peasant); + if (_areWeTheKing()) { return true; @@ -166,51 +147,62 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // caused the exception in the first place... _electionThread = std::thread([this] { - HANDLE waits[2]; - waits[1] = _monarchWaitInterrupt.get(); + _waitOnMonarchThread(); + }); + } + + void WindowManager::_waitOnMonarchThread() + { + HANDLE waits[2]; + waits[1] = _monarchWaitInterrupt.get(); - bool exitRequested = false; - while (!exitRequested) + bool exitRequested = false; + while (!exitRequested) + { + wil::unique_handle hMonarch{ OpenProcess(PROCESS_ALL_ACCESS, + FALSE, + static_cast(_monarch.GetPID())) }; + // TODO:MG If we fail to open the monarch, then they don't exist + // anymore! Go straight to an election. + // + // TODO:MG At any point in all this, the current monarch might die. + // We go straight to a new election, right? Worst case, eventually, + // we'll become the new monarch. + // + // if (hMonarch.get() == nullptr) + // { + // const auto gle = GetLastError(); + // return false; + // } + waits[0] = hMonarch.get(); + auto waitResult = WaitForMultipleObjects(2, waits, FALSE, INFINITE); + + switch (waitResult) { - wil::unique_handle hMonarch{ OpenProcess(PROCESS_ALL_ACCESS, FALSE, static_cast(_monarch.GetPID())) }; - // TODO:MG If we fail to open the monarch, then they don't exist - // anymore! Go straight to an election. - // - // if (hMonarch.get() == nullptr) - // { - // const auto gle = GetLastError(); - // return false; - // } - waits[0] = hMonarch.get(); - auto waitResult = WaitForMultipleObjects(2, waits, FALSE, INFINITE); - - switch (waitResult) - { - case WAIT_OBJECT_0 + 0: // waits[0] was signaled - // Connect to the new monarch, which might be us! - // If we become the monarch, then we'll return true and exit this thread. - exitRequested = _electionNight2020(); - break; - case WAIT_OBJECT_0 + 1: // waits[1] was signaled - exitRequested = true; - break; - - case WAIT_TIMEOUT: - printf("Wait timed out. This should be impossible.\n"); - exitRequested = true; - break; - - // Return value is invalid. - default: - { - auto gle = GetLastError(); - printf("WaitForMultipleObjects returned: %d\n", waitResult); - printf("Wait error: %d\n", gle); - ExitProcess(0); - } - } + case WAIT_OBJECT_0 + 0: // waits[0] was signaled + // Connect to the new monarch, which might be us! + // If we become the monarch, then we'll return true and exit this thread. + exitRequested = _electionNight2020(); + break; + case WAIT_OBJECT_0 + 1: // waits[1] was signaled + exitRequested = true; + break; + + case WAIT_TIMEOUT: + printf("Wait timed out. This should be impossible.\n"); + exitRequested = true; + break; + + // Return value is invalid. + default: + { + auto gle = GetLastError(); + printf("WaitForMultipleObjects returned: %d\n", waitResult); + printf("Wait error: %d\n", gle); + ExitProcess(0); } - }); + } + } } Remoting::Peasant WindowManager::CurrentWindow() diff --git a/src/cascadia/Remoting/WindowManager.h b/src/cascadia/Remoting/WindowManager.h index acbca463588..69c26175902 100644 --- a/src/cascadia/Remoting/WindowManager.h +++ b/src/cascadia/Remoting/WindowManager.h @@ -24,13 +24,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation winrt::Microsoft::Terminal::Remoting::Peasant _peasant{ nullptr }; wil::unique_event _monarchWaitInterrupt; - // wil::unique_event _peasantListenerInterrupt; std::thread _electionThread; - // std::thread _peasantListenerThread; - - // // NON-OWNING HANDLES - // std::vector _peasantHandles{}; void _registerAsMonarch(); void _createMonarch(); @@ -40,6 +35,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation bool _electionNight2020(); void _createPeasantThread(); + void _waitOnMonarchThread(); }; } diff --git a/src/cascadia/UnitTests_Remoting/RemotingTests.cpp b/src/cascadia/UnitTests_Remoting/RemotingTests.cpp index c23e2219555..dd3b5ad123d 100644 --- a/src/cascadia/UnitTests_Remoting/RemotingTests.cpp +++ b/src/cascadia/UnitTests_Remoting/RemotingTests.cpp @@ -279,6 +279,7 @@ namespace RemotingUnitTests VERIFY_ARE_EQUAL(2u, m0->_peasants.size()); + Log::Comment(L"Kill peasant 1. Make sure that it gets removed from the monarch."); RemotingTests::_killPeasant(m0, p1->GetID()); auto maybeP2 = m0->_getPeasant(2); @@ -286,7 +287,6 @@ namespace RemotingUnitTests VERIFY_ARE_EQUAL(peasant2PID, maybeP2.GetPID()); auto maybeP1 = m0->_getPeasant(1); - // VERIFY_ARE_EQUAL(peasant1PID, maybeP1.GetPID()); VERIFY_IS_NULL(maybeP1); VERIFY_ARE_EQUAL(1u, m0->_peasants.size());