Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to hardlink for retry if rename does not succeed #1496

Closed
wants to merge 18 commits into from

Conversation

sreadingMSFT
Copy link
Contributor

@sreadingMSFT sreadingMSFT commented Sep 22, 2021


User reports of rename failing due to unknown issue (thread here: microsoft/PowerToys#13271 ). I had seen this previously and before the repro went away, switching it to a hardlink copy was successful. Using that solution here.

Microsoft Reviewers: Open in CodeFlow

@@ -256,8 +256,9 @@ namespace AppInstaller::CLI::Workflow
if (retry)
{
std::this_thread::sleep_for(250ms);
// If it fails again, let that one throw
std::filesystem::rename(installerPath, renamedDownloadedInstaller);
// If retry fails, try copying with hardlink. The file at installerPath will not be cleaned up after installation,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

retry

if rename fails?

Copy link
Contributor

@yao-msft yao-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@sreadingMSFT
Copy link
Contributor Author

Please don't merge for me. Still trying to do some more testing.

std::filesystem::rename(installerPath, renamedDownloadedInstaller);
// If retry fails, try copying with hardlink. The file at installerPath will not be cleaned up after installation,
// but it is in a temp folder.
std::filesystem::copy(installerPath, renamedDownloadedInstaller, std::filesystem::copy_options::create_hard_links);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking to make it retry every 100ms for 500ms, and only then copy. Since the file won't get cleaned up we would really prefer the rename.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

@JohnMcPMS
Copy link
Member

Sorry, I drank your milkshake 🥛

@JohnMcPMS JohnMcPMS closed this Sep 23, 2021
Trenly added a commit to Trenly/winget-cli that referenced this pull request Mar 22, 2024
411a10915 mint 2.10.19
096a9a1bb Merge pull request microsoft#1782 from microsoft/fix_uri_parsing
006271f6a make Uri.is_host_loopback() only return true for localhost and 127.0.0.1 exactly
9c654889e Remove email list from the readme
31e7feacc Merge pull request microsoft#1718 from dashanji/Fix-typo
e1b6a8e61 Merge pull request microsoft#1711 from Fighter19/pr-fix-safeint3
c5dcbb5bb Merge pull request microsoft#1717 from microsoft/users/GitHubPolicyService/f2ee14d6-8d6b-4313-b754-3880e015f7c3
98ee36dac Fix typo
a57f45918 Microsoft mandatory file
3308d9728 Fix likely typo in SafeInt3.hpp, that results in error with clang 15
07cf58910 Merge pull request microsoft#1429 from NN---/fix/value_int_ctor
3eac925ad Update Release/include/cpprest/json.h
06363bc78 Update Release/include/cpprest/json.h
bfe348779 Merge pull request #1577 from JvdGlind/hidden_visibility_support_macos
0ddc61829 Clarify cpprestsdk level of support
804448058 export http_exception for non Windows builds
d9d7f5ed4 Merge pull request microsoft#1496 from icherniukh/oauth2_client_credentials
8ae5da616 Update oauth2.h
708a5df2b Add support for oauth2 using only client credentials
5408f1dc9 Add constructor from all integer types.

git-subtree-dir: src/cpprestsdk/cpprestsdk
git-subtree-split: 411a109150b270f23c8c97fa4ec9a0a4a98cdecf
Trenly added a commit to Trenly/winget-cli that referenced this pull request Mar 22, 2024
411a10915 mint 2.10.19
096a9a1bb Merge pull request microsoft#1782 from microsoft/fix_uri_parsing
006271f6a make Uri.is_host_loopback() only return true for localhost and 127.0.0.1 exactly
9c654889e Remove email list from the readme
31e7feacc Merge pull request microsoft#1718 from dashanji/Fix-typo
e1b6a8e61 Merge pull request microsoft#1711 from Fighter19/pr-fix-safeint3
c5dcbb5bb Merge pull request microsoft#1717 from microsoft/users/GitHubPolicyService/f2ee14d6-8d6b-4313-b754-3880e015f7c3
98ee36dac Fix typo
a57f45918 Microsoft mandatory file
3308d9728 Fix likely typo in SafeInt3.hpp, that results in error with clang 15
07cf58910 Merge pull request microsoft#1429 from NN---/fix/value_int_ctor
3eac925ad Update Release/include/cpprest/json.h
06363bc78 Update Release/include/cpprest/json.h
bfe348779 Merge pull request #1577 from JvdGlind/hidden_visibility_support_macos
0ddc61829 Clarify cpprestsdk level of support
804448058 export http_exception for non Windows builds
d9d7f5ed4 Merge pull request microsoft#1496 from icherniukh/oauth2_client_credentials
8ae5da616 Update oauth2.h
708a5df2b Add support for oauth2 using only client credentials
5408f1dc9 Add constructor from all integer types.

git-subtree-dir: src/cpprestsdk/cpprestsdk
git-subtree-split: 411a109150b270f23c8c97fa4ec9a0a4a98cdecf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants