Better server start coordination (microsoft#4292)

## Change Improve this named event creation by having both sides try to create or open it. The current version will almost always have to sleep at least once while the server sets up and can completely fail if it takes too long. With this change, we should be able to properly wait for startup on the first attempt.
Madhusudhan-MSFT · Mar 22, 2024 · 834806b · 834806b
1 parent 88cd81e
commit 834806b
Show file tree

Hide file tree

Showing 12 changed files with 81 additions and 20 deletions.
diff --git a/.github/actions/spelling/allow.txt b/.github/actions/spelling/allow.txt
@@ -51,6 +51,7 @@ createnew
 createpintable
 createportabletable
 createtables
+CRTDECL
 CTLs
 curated
 CURSORPOSITON

diff --git a/azure-pipelines.yml b/azure-pipelines.yml
@@ -383,6 +383,7 @@ jobs:
           UndockedRegFreeWinRT\winrtact.dll
       TargetFolder: $(buildOutDirAnyCpu)\PowerShell\Microsoft.WinGet.Client\net6.0-windows10.0.22000.0\SharedDependencies\$(BuildPlatform)
       flattenFolders: true
+    condition: succeededOrFailed()
 
   - task: CopyFiles@2
     displayName: 'Copy native binaries for Microsoft.WinGet.Client'
@@ -395,6 +396,7 @@ jobs:
           UndockedRegFreeWinRT\winrtact.dll
       TargetFolder: $(buildOutDirAnyCpu)\PowerShell\Microsoft.WinGet.Client\net48\SharedDependencies\$(BuildPlatform)
       flattenFolders: true
+    condition: succeededOrFailed()
 
   - task: CopyFiles@2
     displayName: 'Copy native binaries for Microsoft.WinGet.Configuration'
@@ -404,6 +406,7 @@ jobs:
           Microsoft.Management.Configuration\Microsoft.Management.Configuration.dll
       TargetFolder: $(buildOutDirAnyCpu)\PowerShell\Microsoft.WinGet.Configuration\SharedDependencies\$(BuildPlatform)
       flattenFolders: true
+    condition: succeededOrFailed()
 
   - task: CopyFiles@2
     displayName: 'Copy managed binaries for Microsoft.WinGet.Configuration in arch specific'
@@ -413,6 +416,7 @@ jobs:
           Microsoft.Management.Configuration.Projection\net6.0-windows10.0.19041.0\Microsoft.Management.Configuration.Projection.dll
       TargetFolder: $(buildOutDirAnyCpu)\PowerShell\Microsoft.WinGet.Configuration\SharedDependencies\$(BuildPlatform)
       flattenFolders: true
+    condition: succeededOrFailed()
 
   - task: CopyFiles@2
     displayName: 'Copy PowerShell AnyCPU Module Files'

diff --git a/src/AppInstallerCLI.sln b/src/AppInstallerCLI.sln
@@ -180,6 +180,14 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "WinGetSourceCreator", "WinG
 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.WinGet.SharedLib", "PowerShell\Microsoft.WinGet.SharedLib\Microsoft.WinGet.SharedLib.csproj", "{272B2B0E-40D4-4F0F-B187-519A6EF89B10}"
 EndProject
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Tests", "Tests", "{5A52D9FC-0059-4A4A-8196-427A7AA0D1C5}"
+	ProjectSection(SolutionItems) = preProject
+		PowerShell\tests\Microsoft.WinGet.Client.Tests.ps1 = PowerShell\tests\Microsoft.WinGet.Client.Tests.ps1
+		PowerShell\tests\Microsoft.WinGet.Configuration.Tests.ps1 = PowerShell\tests\Microsoft.WinGet.Configuration.Tests.ps1
+		PowerShell\tests\Microsoft.WinGet.DSC.Tests.ps1 = PowerShell\tests\Microsoft.WinGet.DSC.Tests.ps1
+		PowerShell\tests\RunTests.ps1 = PowerShell\tests\RunTests.ps1
+	EndProjectSection
+EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Any CPU = Debug|Any CPU
@@ -1400,6 +1408,7 @@ Global
 		{167F634B-A3AD-494E-8E67-B888103E35FF} = {7C218A3E-9BC8-48FF-B91B-BCACD828C0C9}
 		{C54F80ED-B736-49B0-9BD3-662F57024D01} = {7C218A3E-9BC8-48FF-B91B-BCACD828C0C9}
 		{272B2B0E-40D4-4F0F-B187-519A6EF89B10} = {7C218A3E-9BC8-48FF-B91B-BCACD828C0C9}
+		{5A52D9FC-0059-4A4A-8196-427A7AA0D1C5} = {7C218A3E-9BC8-48FF-B91B-BCACD828C0C9}
 	EndGlobalSection
 	GlobalSection(ExtensibilityGlobals) = postSolution
 		SolutionGuid = {B6FDB70C-A751-422C-ACD1-E35419495857}

diff --git a/src/AppInstallerCLICore/Core.cpp b/src/AppInstallerCLICore/Core.cpp
@@ -45,11 +45,25 @@ namespace AppInstaller::CLI
 
         private:
             UINT m_previousCP = 0;
-        };
+        };
+
+        void __CRTDECL abort_signal_handler(int)
+        {
+#ifndef AICLI_DISABLE_TEST_HOOKS
+            if (Settings::User().Get<Settings::Setting::EnableSelfInitiatedMinidump>())
+            {
+                Debugging::WriteMinidump();
+            }
+#endif
+
+            std::_Exit(APPINSTALLER_CLI_ERROR_INTERNAL_ERROR);
+        }
     }
 
     int CoreMain(int argc, wchar_t const** argv) try
-    {
+    {
+        std::signal(SIGABRT, abort_signal_handler);
+
         init_apartment();
 
 #ifndef AICLI_DISABLE_TEST_HOOKS

diff --git a/src/AppInstallerCLICore/pch.h b/src/AppInstallerCLICore/pch.h
@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation.
+// Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 #pragma once
 
@@ -18,6 +18,7 @@
 
 #include <array>
 #include <atomic>
+#include <csignal>
 #include <iostream>
 #include <fstream>
 #include <future>

diff --git a/src/AppInstallerCommonCore/Debugging.cpp b/src/AppInstallerCommonCore/Debugging.cpp
@@ -57,6 +57,14 @@ namespace AppInstaller::Debugging
                 return EXCEPTION_CONTINUE_SEARCH;
             }
 
+            void WriteMinidump()
+            {
+                std::thread([&]() {
+                    MiniDumpWriteDump(GetCurrentProcess(), GetCurrentProcessId(), Instance().m_file.get(), MiniDumpNormal, nullptr, nullptr, nullptr);
+                    Instance().m_keepFile = true;
+                    }).join();
+            }
+
         private:
             std::filesystem::path m_filePath;
             wil::unique_handle m_file;
@@ -69,4 +77,9 @@ namespace AppInstaller::Debugging
         // Force object creation and thus enabling of the crash detection.
         SelfInitiatedMinidumpHelper::Instance();
     }
+
+    void WriteMinidump()
+    {
+        SelfInitiatedMinidumpHelper::Instance().WriteMinidump();
+    }
 }
diff --git a/src/AppInstallerCommonCore/Public/winget/Debugging.h b/src/AppInstallerCommonCore/Public/winget/Debugging.h
@@ -6,4 +6,7 @@ namespace AppInstaller::Debugging
 {
     // Enables a self initiated minidump on certain process level failures.
     void EnableSelfInitiatedMinidump();
+
+    // Forces the minidump to be written.
+    void WriteMinidump();
 }
diff --git a/src/Microsoft.Management.Configuration.OutOfProc/Prepare-ConfigurationOOPTests.ps1 b/src/Microsoft.Management.Configuration.OutOfProc/Prepare-ConfigurationOOPTests.ps1
@@ -24,7 +24,7 @@ if (-not [System.String]::IsNullOrEmpty($PackageLayoutPath))
 {
     $Local:packageManifestPath = Join-Path $PackageLayoutPath "AppxManifest.xml"
 
-    Add-AppxPackage -Register $Local:packageManifestPath
+    Add-AppxPackage -ForceApplicationShutdown -Register $Local:packageManifestPath
 
     # Configure crash dump and log file settings
     $Local:settingsExport = ConvertFrom-Json (wingetdev.exe settings export)

diff --git a/src/WinGetServer/Utils.cpp b/src/WinGetServer/Utils.cpp
@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation.
+// Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 #include "Utils.h"
 #pragma warning( push )
@@ -32,4 +32,21 @@ std::string GetUserSID()
     LPSTR pszSID = NULL;
     THROW_LAST_ERROR_IF(!ConvertSidToStringSidA(pTokenUser->User.Sid, &pszSID));
     return std::string{ pszSID };
-}
+}
+
+wil::unique_event CreateOrOpenServerStartEvent()
+{
+    wil::unique_event result;
+
+    for (int i = 0; !result && i < 2; ++i)
+    {
+        if (!result.try_create(wil::EventOptions::ManualReset, L"WinGetServerStartEvent"))
+        {
+            result.try_open(L"WinGetServerStartEvent");
+        }
+    }
+
+    THROW_LAST_ERROR_IF(!result);
+
+    return result;
+}
diff --git a/src/WinGetServer/Utils.h b/src/WinGetServer/Utils.h
@@ -1,8 +1,14 @@
-// Copyright (c) Microsoft Corporation.
+// Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 #pragma once
+#pragma warning( push )
+#pragma warning ( disable : 6001 6388 6553)
+#include <wil/resource.h>
+#pragma warning( pop )
 #include <string>
 
 unsigned char* GetUCharString(const std::string& str);
 
-std::string GetUserSID();
+std::string GetUserSID();
+
+wil::unique_event CreateOrOpenServerStartEvent();
diff --git a/src/WinGetServer/WinGetServerManualActivation_Client.cpp b/src/WinGetServer/WinGetServerManualActivation_Client.cpp
@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation.
+// Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 #include "WinGetServer.h"
 #include "Utils.h"
@@ -76,11 +76,8 @@ HRESULT LaunchWinGetServerWithManualActivation()
     RETURN_LAST_ERROR_IF(!CreateProcessW(NULL, &commandLineInput[0], NULL, NULL, FALSE, 0, NULL, NULL, &info, &process));
 
     // Wait for manual reset event from server before proceeding with COM activation.
-    wil::unique_event manualResetEvent;
-    if (manualResetEvent.try_open(L"WinGetServerStartEvent"))
-    {
-        manualResetEvent.wait();
-    }
+    wil::unique_event manualResetEvent = CreateOrOpenServerStartEvent();
+    manualResetEvent.wait(10000);
 
     return S_OK;
 }

diff --git a/src/WinGetServer/WinMain.cpp b/src/WinGetServer/WinMain.cpp
@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation.
+// Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 #define NOMINMAX
 #pragma warning( push )
@@ -152,11 +152,7 @@ int __stdcall wWinMain(_In_ HINSTANCE, _In_opt_ HINSTANCE, _In_ LPWSTR cmdLine,
 
             RETURN_IF_FAILED(WindowsPackageManagerServerInitializeRPCServer());
 
-            if (!manualResetEvent.try_create(wil::EventOptions::ManualReset, L"WinGetServerStartEvent"))
-            {
-                manualResetEvent.open(L"WinGetServerStartEvent");
-            }
-
+            manualResetEvent = CreateOrOpenServerStartEvent();
             manualResetEvent.SetEvent();
         }