More tests, more redundancy

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:
+    // - <none>
+    // Return Value:
+    // - a WindowActivatedArgs with info about when and where we were last activated.
+    Remoting::WindowActivatedArgs Peasant::GetLastActivatedArgs()
     {
         return _lastActivatedArgs;
     }

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:
+    // - <none>
+    // 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<DWORD>(_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<DWORD>(_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));
+                    }
+                }
             }
         }
     }

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<uint64_t> givenID);
 
-        bool _electionNight2020();
+        bool _performElection();
         void _createPeasantThread();
         void _waitOnMonarchThread();
         void _raiseFindTargetWindowRequested(const winrt::Windows::Foundation::IInspectable& sender,

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<Remoting::implementation::Monarch> 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<Remoting::implementation::Peasant> 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<Remoting::implementation::Peasant> 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<winrt::hstring> p1Args{ L"1", L"arg[1]" };
+        std::vector<winrt::hstring> 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());
+        }
     }
 }