diff --git a/src/cascadia/Remoting/Peasant.cpp b/src/cascadia/Remoting/Peasant.cpp index 659a2b293f8..699adf976b2 100644 --- a/src/cascadia/Remoting/Peasant.cpp +++ b/src/cascadia/Remoting/Peasant.cpp @@ -69,12 +69,15 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation return _initialArgs; } - void Peasant::ActivateWindow(const winrt::Microsoft::Terminal::Remoting::WindowActivatedArgs& args) + void Peasant::ActivateWindow(const Remoting::WindowActivatedArgs& args) { // TODO: projects/5 - somehow, pass an identifier for the current // desktop into this method. The Peasant shouldn't need to be able to // figure it out, but it will need to report it to the monarch. + // Store these new args as our last activated state. If a new monarch + // comes looking, we can use this info to tell them when we were last + // activated. _lastActivatedArgs = args; bool successfullyNotified = false; @@ -91,7 +94,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation } catch (...) { - // TODO:MG Tracelogging LOG_CAUGHT_EXCEPTION(); } @@ -100,12 +102,17 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation TraceLoggingUInt64(GetID(), "peasantID", "Our ID"), TraceLoggingBoolean(successfullyNotified, "successfullyNotified", "true if we successfully notified the monarch"), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); - // TODO:MG Open three windows, close the first (the monarch). The focus - // should automatically move to the third, from the windows shell. In - // that window, `wt -w 0` does not work right. } - winrt::Microsoft::Terminal::Remoting::WindowActivatedArgs Peasant::GetLastActivatedArgs() + // Method Description: + // - Retrieve the WindowActivatedArgs describing the last activation of this + // peasant. New monarchs can use this state to determine when we were last + // activated. + // Arguments: + // - + // Return Value: + // - a WindowActivatedArgs with info about when and where we were last activated. + Remoting::WindowActivatedArgs Peasant::GetLastActivatedArgs() { return _lastActivatedArgs; } diff --git a/src/cascadia/Remoting/WindowManager.cpp b/src/cascadia/Remoting/WindowManager.cpp index 08234b2ba95..df7006b3ca4 100644 --- a/src/cascadia/Remoting/WindowManager.cpp +++ b/src/cascadia/Remoting/WindowManager.cpp @@ -199,7 +199,14 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation return _peasant; } - bool WindowManager::_electionNight2020() + // Method Description: + // - Attempt to connect to the monarch process. This might be us! + // - For the new monarch, add us to their list of peasants. + // Arguments: + // - + // Return Value: + // - true iff we're the new monarch process. + bool WindowManager::_performElection() { _createMonarchAndCallbacks(); @@ -208,10 +215,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation if (_areWeTheKing()) { - // This is only called when a _new_ monarch is elected. So don't do - // anything here that needs to be done for all monarch windows. This - // should only be for work that's done when a window _becomes_ a - // monarch, after the death of the previous monarch. + // This method is only called when a _new_ monarch is elected. So + // don't do anything here that needs to be done for all monarch + // windows. This should only be for work that's done when a window + // _becomes_ a monarch, after the death of the previous monarch. return true; } return false; @@ -233,63 +240,126 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation { HANDLE waits[2]; waits[1] = _monarchWaitInterrupt.get(); + const auto peasantID = _peasant.GetID(); - bool exitRequested = false; - while (!exitRequested) + bool exitThreadRequested = false; + while (!exitThreadRequested) { - 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) + // At any point in all this, the current monarch might die. If it + // does, we'll go straight to a new election, in the "jail" + // try/catch below. Worst case, eventually, we'll become the new + // monarch. + try { - case WAIT_OBJECT_0 + 0: // waits[0] was signaled - - TraceLoggingWrite(g_hRemotingProvider, - "WindowManager_MonarchDied", - TraceLoggingUInt64(_peasant.GetID(), "peasantID", "Our peasant ID"), - TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); - // 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 + // This might fail to even ask the monarch for it's PID. + wil::unique_handle hMonarch{ OpenProcess(PROCESS_ALL_ACCESS, + FALSE, + static_cast(_monarch.GetPID())) }; + + // 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(); + TraceLoggingWrite(g_hRemotingProvider, + "WindowManager_FailedToOpenMonarch", + TraceLoggingUInt64(peasantID, "peasantID", "Our peasant ID"), + TraceLoggingUInt64(gle, "lastError", "The result of GetLastError"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + + exitThreadRequested = _performElection(); + continue; + } + + waits[0] = hMonarch.get(); + auto waitResult = WaitForMultipleObjects(2, waits, FALSE, INFINITE); + + switch (waitResult) + { + case WAIT_OBJECT_0 + 0: // waits[0] was signaled, the handle to the monarch process + + TraceLoggingWrite(g_hRemotingProvider, + "WindowManager_MonarchDied", + TraceLoggingUInt64(peasantID, "peasantID", "Our peasant ID"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + // Connect to the new monarch, which might be us! + // If we become the monarch, then we'll return true and exit this thread. + exitThreadRequested = _performElection(); + break; + + case WAIT_OBJECT_0 + 1: // waits[1] was signaled, our manual interrupt + + TraceLoggingWrite(g_hRemotingProvider, + "WindowManager_MonarchWaitInterrupted", + TraceLoggingUInt64(peasantID, "peasantID", "Our peasant ID"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + exitThreadRequested = true; + break; + + case WAIT_TIMEOUT: + // This should be impossible. + TraceLoggingWrite(g_hRemotingProvider, + "WindowManager_MonarchWaitTimeout", + TraceLoggingUInt64(peasantID, "peasantID", "Our peasant ID"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + exitThreadRequested = true; + break; + + default: + { + // Returning any other value is invalid. Just die. + const auto gle = GetLastError(); + TraceLoggingWrite(g_hRemotingProvider, + "WindowManager_WaitFailed", + TraceLoggingUInt64(peasantID, "peasantID", "Our peasant ID"), + TraceLoggingUInt64(gle, "lastError", "The result of GetLastError"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + ExitProcess(0); + } + } + } + catch (...) + { + // Theoretically, if window[1] dies when we're trying to get + // it's PID we'll get here. If we just try to do the election + // once here, it's possible we might elect window[2], but have + // it die before we add ourselves as a peasant. That + // _performElection call will throw, and we wouldn't catch it + // here, and we'd die. + + // Instead, we're going to have a resilent election process. + // We're going to keep trying an election, until one _doesn't_ + // throw an exception. That might mean burning through all the + // other dying monarchs until we find us as the monarch. But if + // this process is alive, then there's _someone_ in the line of + // succession. TraceLoggingWrite(g_hRemotingProvider, - "WindowManager_MonarchWaitInterrupted", - TraceLoggingUInt64(_peasant.GetID(), "peasantID", "Our peasant ID"), + "WindowManager_ExceptionInWaitThread", + TraceLoggingUInt64(peasantID, "peasantID", "Our peasant ID"), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); - - 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); - } + bool foundNewMonarch = false; + while (!foundNewMonarch) + { + try + { + exitThreadRequested = _performElection(); + // It doesn't matter if we're the monarch, or someone + // else is, but if we complete the election, then we've + // registered with a new one. We can escape this jail + // and re-enter society. + foundNewMonarch = true; + } + catch (...) + { + // If we fail to acknowledge the results of the election, + // stay in this jail until we do. + TraceLoggingWrite(g_hRemotingProvider, + "WindowManager_ExceptionInNestedWaitThread", + TraceLoggingUInt64(peasantID, "peasantID", "Our peasant ID"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); + } + } } } } diff --git a/src/cascadia/Remoting/WindowManager.h b/src/cascadia/Remoting/WindowManager.h index 5e52210cb33..a577ab5e502 100644 --- a/src/cascadia/Remoting/WindowManager.h +++ b/src/cascadia/Remoting/WindowManager.h @@ -38,7 +38,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation bool _areWeTheKing(); winrt::Microsoft::Terminal::Remoting::IPeasant _createOurPeasant(std::optional givenID); - bool _electionNight2020(); + bool _performElection(); void _createPeasantThread(); void _waitOnMonarchThread(); void _raiseFindTargetWindowRequested(const winrt::Windows::Foundation::IInspectable& sender, diff --git a/src/cascadia/UnitTests_Remoting/RemotingTests.cpp b/src/cascadia/UnitTests_Remoting/RemotingTests.cpp index dd635eecd1f..247061fcc72 100644 --- a/src/cascadia/UnitTests_Remoting/RemotingTests.cpp +++ b/src/cascadia/UnitTests_Remoting/RemotingTests.cpp @@ -543,6 +543,60 @@ namespace RemotingUnitTests void RemotingTests::ProposeCommandlineDeadWindow() { Log::Comment(L"Test proposing a commandline for a peasant that previously died"); - VERIFY_ARE_EQUAL(true, false, L"TODO: Finish this test"); + + const auto monarch0PID = 12345u; + com_ptr m0; + m0.attach(new Remoting::implementation::Monarch(monarch0PID)); + VERIFY_IS_NOT_NULL(m0); + m0->FindTargetWindowRequested(&RemotingTests::_findTargetWindowHelper); + + Log::Comment(L"Add a peasant"); + const auto peasant1PID = 23456u; + com_ptr p1; + p1.attach(new Remoting::implementation::Peasant(peasant1PID)); + VERIFY_IS_NOT_NULL(p1); + m0->AddPeasant(*p1); + p1->ExecuteCommandlineRequested([&](auto&&, const Remoting::CommandlineArgs& /*cmdlineArgs*/) { + Log::Comment(L"Commandline dispatched to p1"); + VERIFY_IS_TRUE(false, L"This should not happen, this peasant should be dead."); + }); + + Log::Comment(L"Add a second peasant"); + const auto peasant2PID = 34567u; + com_ptr p2; + p2.attach(new Remoting::implementation::Peasant(peasant2PID)); + VERIFY_IS_NOT_NULL(p2); + m0->AddPeasant(*p2); + p2->ExecuteCommandlineRequested([&](auto&&, const Remoting::CommandlineArgs& cmdlineArgs) { + Log::Comment(L"Commandline dispatched to p2"); + VERIFY_IS_GREATER_THAN(cmdlineArgs.Args().size(), 1u); + VERIFY_ARE_EQUAL(L"this is for p2", cmdlineArgs.Args().at(1)); + }); + + std::vector p1Args{ L"1", L"arg[1]" }; + std::vector p2Args{ L"2", L"this is for p2" }; + + Log::Comment(L"Kill peasant 1"); + + _killPeasant(m0, 1); + + { + Log::Comment(L"Send a commandline to p2, who is still alive. We won't create a new window."); + + Remoting::CommandlineArgs eventArgs{ { p2Args }, { L"" } }; + + auto result = m0->ProposeCommandline(eventArgs); + VERIFY_ARE_EQUAL(false, result.ShouldCreateWindow()); + VERIFY_ARE_EQUAL(false, (bool)result.Id()); + } + { + Log::Comment(L"Send a commandline to p1, who is dead. We will create a new window."); + Remoting::CommandlineArgs eventArgs{ { p1Args }, { L"" } }; + + auto result = m0->ProposeCommandline(eventArgs); + VERIFY_ARE_EQUAL(true, result.ShouldCreateWindow()); + VERIFY_ARE_EQUAL(true, (bool)result.Id()); + VERIFY_ARE_EQUAL(1u, result.Id().Value()); + } } }