Port window thread shutdown to 1.11 (#5782)

Ports #5781 to the 1.11 branch.

Author: JohnMcPMS

src/AppInstallerCLICore/Commands/TestCommand.cpp

@@ -11,6 +11,7 @@
 #include "Workflows/ConfigurationFlow.h"
 #include <winget/RepositorySource.h>
 #include <winrt/Microsoft.Management.Configuration.h>
+#include <winrt/Windows.ApplicationModel.Core.h>
 
 using namespace AppInstaller::CLI::Workflow;
 using namespace AppInstaller::Utility::literals;
@@ -72,7 +73,7 @@ namespace AppInstaller::CLI
                     NULL,
                     ENDSESSION_CLOSEAPP,
                     (SMTO_ABORTIFHUNG | SMTO_ERRORONEXIT),
-                    5000,
+                    10000,
                     NULL));
             }
 
@@ -275,6 +276,39 @@ namespace AppInstaller::CLI
                 return module ? module->GetObjectCount() : 0;
             }
         };
+
+        struct TestTerminateTerminationSignalHandler final : public Command
+        {
+            TestTerminateTerminationSignalHandler(std::string_view parent) : Command("term-signal-handler", {}, parent, Visibility::Hidden) {}
+
+            Resource::LocString ShortDescription() const override
+            {
+                return "Test TerminationSignalHandler thread"_lis;
+            }
+
+            Resource::LocString LongDescription() const override
+            {
+                return "Forces the TerminationSignalHandler static object to be destroyed so that the thread behavior can be observed."_lis;
+            }
+
+        protected:
+            void ExecuteInternal(Execution::Context& context) const override
+            {
+                // Destroy the one created by standard execution
+                // We join on the window thread, so if this never exits we have failed the test.
+                winrt::Windows::ApplicationModel::Core::CoreApplication::Properties().TryRemove(L"WindowsPackageManager.SignalTerminationHandler");
+
+                // Create a new instance
+                if (Execution::GetWindowHandle() == nullptr)
+                {
+                    LogAndReport(context, "Didn't get a window handle");
+                }
+                else
+                {
+                    LogAndReport(context, "Got a window handle");
+                }
+            }
+        };
     }
 
     std::vector<std::unique_ptr<Command>> TestCommand::GetCommands() const
@@ -285,6 +319,7 @@ namespace AppInstaller::CLI
                 std::make_unique<TestConfigurationExportCommand>(FullName()),
                 std::make_unique<TestConfigurationFindUnitProcessorsCommand>(FullName()),
                 std::make_unique<TestCanUnloadNowCommand>(FullName()),
+                std::make_unique<TestTerminateTerminationSignalHandler>(FullName()),
             });
     }
 

src/AppInstallerCLICore/ExecutionContext.cpp

@@ -13,6 +13,7 @@
 #include <winget/NetworkSettings.h>
 
 using namespace AppInstaller::Checkpoints;
+using namespace std::chrono_literals;
 
 namespace AppInstaller::CLI::Execution
 {
@@ -117,11 +118,16 @@ namespace AppInstaller::CLI::Execution
 
             ~SignalTerminationHandler()
             {
-                // At this point the thread is gone, but it will get angry
-                // if there's no call to join.
+                // std::thread requires that any managed thread (joinable) be joined or detached before destructing
                 if (m_windowThread.joinable())
                 {
-                    m_windowThread.detach();
+                    if (m_windowHandle)
+                    {
+                        // Inform the thread that it should stop.
+                        PostMessageW(m_windowHandle.get(), WM_DESTROY, 0, 0);
+                    }
+
+                    m_windowThread.join();
                 }
             }
 
@@ -219,6 +225,12 @@ namespace AppInstaller::CLI::Execution
                     return;
                 }
 
+                // Unregister the window class on exiting the thread
+                auto classUnregister = wil::scope_exit([&]()
+                    {
+                        UnregisterClassW(windowClass, hInstance);
+                    });
+
                 m_windowHandle = wil::unique_hwnd(CreateWindow(
                     windowClass,
                     L"WingetMessageOnlyWindow",
@@ -232,26 +244,38 @@ namespace AppInstaller::CLI::Execution
                     hInstance,
                     NULL)); /* lpParam */
 
-                if (m_windowHandle == nullptr)
+                HWND windowHandle = m_windowHandle.get();
+                if (windowHandle == nullptr)
                 {
                     LOG_LAST_ERROR_MSG("Failed creating window");
                     return;
                 }
 
-                ShowWindow(m_windowHandle.get(), SW_HIDE);
+                // We must destroy the window first so that the class unregister can succeed
+                auto destroyWindow = wil::scope_exit([&]()
+                    {
+                        DestroyWindow(windowHandle);
+                    });
+
+                ShowWindow(windowHandle, SW_HIDE);
 
                 // Force message queue to be created.
                 MSG msg;
                 PeekMessage(&msg, NULL, WM_USER, WM_USER, PM_NOREMOVE);
                 m_messageQueueReady.SetEvent();
 
-                // Message loop
+                // Message loop, we send WM_DESTROY to terminate it
                 BOOL getMessageResult;
-                while ((getMessageResult = GetMessage(&msg, m_windowHandle.get(), 0, 0)) != 0)
+                while ((getMessageResult = GetMessage(&msg, windowHandle, 0, 0)) != 0)
                 {
                     if (getMessageResult == -1)
                     {
                         LOG_LAST_ERROR();
+                        break;
+                    }
+                    else if (msg.message == WM_DESTROY)
+                    {
+                        break;
                     }
                     else
                     {

src/AppInstallerCLIE2ETests/AppShutdownTests.cs

@@ -1,4 +1,4 @@
-// -----------------------------------------------------------------------------
+// -----------------------------------------------------------------------------
 // <copyright file="AppShutdownTests.cs" company="Microsoft Corporation">
 //     Copyright (c) Microsoft Corporation. Licensed under the MIT License.
 // </copyright>
@@ -121,5 +121,15 @@ public void CanUnloadNowTest()
             Assert.True(lines[3].Contains("Internal objects: 0"));
             Assert.True(lines[4].Contains("External objects: 0"));
         }
+
+        /// <summary>
+        /// Runs winget test term-signal-handler to check for proper thread termination.
+        /// </summary>
+        [Test]
+        public void TermSignalHandler()
+        {
+            var result = TestCommon.RunAICLICommand("test", "term-signal-handler --verbose");
+            Assert.True(result.StdOut.Contains("Got a window handle"));
+        }
     }
-}
+}