commit e630cf7c64291c3009e7b2e03ab73e314e501c76
parent 44dba6a63b663a77a546b584e61e72aec927c16f
Author: JohnMcPMS <johnmcp@microsoft.com>
Date: Wed, 22 Sep 2021 17:48:28 -0700
Make rename retry more frequently for longer, then try making a hardlink (#1497)
We already had a single retry after a 250ms wait for our `rename` of the installer to make it shell actionable with the correct extension. This change makes it try every 100ms for 500ms, then use `copy_file` with a hard link option as a fall back.
The hard link is better than a true copy, as it won't take up more space. But it won't get cleaned up on a successful install either.
Long term we should put in some background cleanup to remove old installers that were abandoned due to failures.
Diffstat:
3 files changed, 74 insertions(+), 17 deletions(-)
diff --git a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp
@@ -181,6 +181,68 @@ namespace AppInstaller::CLI::Workflow
return args;
}
+
+ // Complicated rename algorithm due to somewhat arbitrary failures.
+ // 1. First, try to rename.
+ // 2. Then, create an empty file for the target, and attempt to rename.
+ // 3. Then, try repeatedly for 500ms in case it is a timing thing.
+ // 4. Attempt to use a hard link if available.
+ // 5. Copy the file if nothing else has worked so far.
+ void RenameFile(const std::filesystem::path& from, const std::filesystem::path& to)
+ {
+ // 1. First, try to rename.
+ try
+ {
+ // std::filesystem::rename() handles motw correctly if applicable.
+ std::filesystem::rename(from, to);
+ return;
+ }
+ CATCH_LOG();
+
+ // 2. Then, create an empty file for the target, and attempt to rename.
+ // This seems to fix things in certain cases, so we do it.
+ try
+ {
+ {
+ std::ofstream targetFile{ to };
+ }
+ std::filesystem::rename(from, to);
+ return;
+ }
+ CATCH_LOG();
+
+ // 3. Then, try repeatedly for 500ms in case it is a timing thing.
+ for (int i = 0; i < 5; ++i)
+ {
+ try
+ {
+ std::this_thread::sleep_for(100ms);
+ std::filesystem::rename(from, to);
+ return;
+ }
+ CATCH_LOG();
+ }
+
+ // 4. Attempt to use a hard link if available.
+ if (Runtime::SupportsHardLinks(from))
+ {
+ try
+ {
+ // Create a hard link to the file; the installer will be left in the temp directory afterward
+ // but it is better to succeed the operation and leave a file around than to fail.
+ // First we have to remove the target file as the function will not overwrite.
+ std::filesystem::remove(to);
+ std::filesystem::create_hard_link(from, to);
+ return;
+ }
+ CATCH_LOG();
+ }
+
+ // 5. Copy the file if nothing else has worked so far.
+ // Create a copy of the file; the installer will be left in the temp directory afterward
+ // but it is better to succeed the operation and leave a file around than to fail.
+ std::filesystem::copy_file(from, to, std::filesystem::copy_options::overwrite_existing);
+ }
}
void ShellExecuteInstallImpl(Execution::Context& context)
@@ -242,23 +304,7 @@ namespace AppInstaller::CLI::Workflow
break;
}
- // If rename fails, don't give up quite yet.
- bool retry = true;
-
- try
- {
- // std::filesystem::rename() handles motw correctly if applicable.
- std::filesystem::rename(installerPath, renamedDownloadedInstaller);
- retry = false;
- }
- CATCH_LOG();
-
- if (retry)
- {
- std::this_thread::sleep_for(250ms);
- // If it fails again, let that one throw
- std::filesystem::rename(installerPath, renamedDownloadedInstaller);
- }
+ RenameFile(installerPath, renamedDownloadedInstaller);
installerPath.assign(renamedDownloadedInstaller);
AICLI_LOG(CLI, Info, << "Successfully renamed downloaded installer. Path: " << installerPath);
diff --git a/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h b/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h
@@ -60,4 +60,7 @@ namespace AppInstaller::Runtime
// Checks if the file system is NTFS
bool IsNTFS(const std::filesystem::path& filePath);
+
+ // Checks if the file system at path supports hard links
+ bool SupportsHardLinks(const std::filesystem::path& path);
}
diff --git a/src/AppInstallerCommonCore/Runtime.cpp b/src/AppInstallerCommonCore/Runtime.cpp
@@ -409,6 +409,9 @@ namespace AppInstaller::Runtime
return wil::test_token_membership(nullptr, SECURITY_NT_AUTHORITY, SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_ADMINS);
}
+ // TODO: Replace this function with proper checks for supported functionality rather
+ // than simply relying on "is it NTFS?", even if those functions delegate to
+ // this one for the answer.
bool IsNTFS(const std::filesystem::path& filePath)
{
wil::unique_hfile fileHandle{ CreateFileW(
@@ -436,6 +439,11 @@ namespace AppInstaller::Runtime
return _wcsicmp(fileSystemName, L"NTFS") == 0;
}
+ bool SupportsHardLinks(const std::filesystem::path& path)
+ {
+ return IsNTFS(path);
+ }
+
#ifndef AICLI_DISABLE_TEST_HOOKS
void TestHook_SetPathOverride(PathName target, const std::filesystem::path& path)
{
