Skip to content

Commit

Permalink
Make rename retry more frequently for longer, then try making a hardl…
Browse files Browse the repository at this point in the history
…ink (#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.
  • Loading branch information
JohnMcPMS authored Sep 23, 2021
1 parent 44dba6a commit e630cf7
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 17 deletions.
80 changes: 63 additions & 17 deletions src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions src/AppInstallerCommonCore/Public/AppInstallerRuntime.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
8 changes: 8 additions & 0 deletions src/AppInstallerCommonCore/Runtime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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)
{
Expand Down

0 comments on commit e630cf7

Please sign in to comment.