From 834806b5392c2f21f677135846e23eb3348812d9 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Fri, 22 Mar 2024 09:38:20 -0700 Subject: [PATCH] Better server start coordination (#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. --- .github/actions/spelling/allow.txt | 1 + azure-pipelines.yml | 4 ++++ src/AppInstallerCLI.sln | 9 ++++++++ src/AppInstallerCLICore/Core.cpp | 18 ++++++++++++++-- src/AppInstallerCLICore/pch.h | 3 ++- src/AppInstallerCommonCore/Debugging.cpp | 13 ++++++++++++ .../Public/winget/Debugging.h | 3 +++ .../Prepare-ConfigurationOOPTests.ps1 | 2 +- src/WinGetServer/Utils.cpp | 21 +++++++++++++++++-- src/WinGetServer/Utils.h | 10 +++++++-- .../WinGetServerManualActivation_Client.cpp | 9 +++----- src/WinGetServer/WinMain.cpp | 8 ++----- 12 files changed, 81 insertions(+), 20 deletions(-) diff --git a/.github/actions/spelling/allow.txt b/.github/actions/spelling/allow.txt index 9714eaf8a8..2840f7c2b0 100644 --- 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 index 0f020f4fdb..d1ee25f861 100644 --- 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 index 4be91723a3..b0517698c8 100644 --- 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 index 23be91523e..418c326016 100644 --- 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()) + { + 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 index 50a37a2cc0..e18952c69d 100644 --- 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 #include +#include #include #include #include diff --git a/src/AppInstallerCommonCore/Debugging.cpp b/src/AppInstallerCommonCore/Debugging.cpp index 6bfbf795d2..e9bc6c6e53 100644 --- 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 index 319c04a313..e7c5476f24 100644 --- 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 index 3b3554e37a..c057449965 100644 --- 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 index 57af9414d8..31069713eb 100644 --- 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 }; -} \ No newline at end of file +} + +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 index 80b73dfcfb..0e1cc8745f 100644 --- 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 +#pragma warning( pop ) #include unsigned char* GetUCharString(const std::string& str); -std::string GetUserSID(); \ No newline at end of file +std::string GetUserSID(); + +wil::unique_event CreateOrOpenServerStartEvent(); diff --git a/src/WinGetServer/WinGetServerManualActivation_Client.cpp b/src/WinGetServer/WinGetServerManualActivation_Client.cpp index 33880833a4..0000dd157c 100644 --- 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 index 761a33aeb3..541a13deb4 100644 --- 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(); }