Skip to content
This repository has been archived by the owner on Apr 20, 2023. It is now read-only.

Add retry when Directory.Move #9313

Merged
merged 2 commits into from
May 22, 2018
Merged

Conversation

wli3
Copy link

@wli3 wli3 commented May 18, 2018

Link to GitHub issue for open source changes

https://github.com/dotnet/cli/issues/9289

Link to GitHub dotnet:release/[version] pull request (see Git Prepping a PR from the Release Branch)

#9313

ProjectK TFS bug and changeset for closed source changes (security fix, package authoring, etc)

N/A

Describe the issue and how it manifests to the customer.

When running uninstall and then install on Windows. Due to global tools use Directory.Move underneath, there is a chance the indexer or the antivirus collided (had a handle still open on the prior name). And the installation/uninstalling will fail with access denied error.

What is the impact on the customer?

This impact the producers most. During producer’s test, it is likely they will uninstall and install their tools several times. And the frequency of seeing this error varies machine by machine.

Are there changes that cannot be made in GitHub? (security fixes, etc)

No

Characterize potential risks introduced by the fix.

Low risk. Try action on failure. And Directory.Move, File.Move is atomic. Once it fails it can be retired.

Does the change alter existing behaviors? (return values or types, error messages or exceptions, etc. In
other words, is this a build-to-build breaking change?)

No

What changed?
What will teams need to do to respond?
Once check-in is approved send breaking change mail to netcorebreak

Code reviewers:

What testing has been completed?

manual test

Which platforms are impacted? (Linux | Mac | Windows)

Windows

Effected binaries and packages (exact name)

dotnet.dll, Microsoft.DotNet.Cli.Utils.dll


Why check 0x80070005 on IOException
This error is the exact error thrown. It is just an IOException. The only way to detect that is by HResult.

Why don’t use the function above -- RetryOnFileAccessFailure?
RetryOnFileAccessFailure is async. And also in this case, it is awaiting a void. There is some gotcha around it. Since it is so close to release, I choose the safer way. And RetryOnFileAccessFailure ‘s exception is not specific. UnauthorizedAccessException is likely a real error for user to handle for –-tool-path.

Tested with 100 times tight loop. No error

@wli3 wli3 requested review from nguerrera, livarcocc, peterhuene, JeremyKuhne and a team May 18, 2018 01:28
@wli3 wli3 force-pushed the retry-directory.move2 branch from dfafcd6 to 751ec0d Compare May 18, 2018 01:32
@wli3
Copy link
Author

wli3 commented May 18, 2018

@wli3
Copy link
Author

wli3 commented May 18, 2018

The following can repo the root cause

using System;
using System.IO;
using System.Threading;
 
namespace console1
{
    class Program
    {
        static void Main(string[] args)
        {
            for (int i = 1; i < 1000000; i++)
            {
                Console.WriteLine(i);
                Directory.Move(@"C:\work\test1\folder", @"C:\work\test2\folder");
                Directory.Move(@"C:\work\test2\folder", @"C:\work\test1\folder");
            }
        }
    }
}
 

result

 …..
….
 
5873
5874
5875
5876
dotnet : 
At line:1 char:1
+ dotnet run >> result.txt 2>&1
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:String) [], RemoteException
    + FullyQualifiedErrorId : NativeCommandError
 
Unhandled Exception:
 
System.IO.IOException: Access to the path 'C:\work\test1\folder' is denied.
   at System.IO.FileSystem.MoveDirectory(String sourceFullPath, String destFullPath)
   at System.IO.Directory.Move(String sourceDirName, String destDirName)
   at console1.Program.Main(String[] args) in C:\Users\wul\Documents\console1\Program.cs:line 14
 

/// </summary>
internal static void RetryOnMoveAccessFailure(Action action)
{
const int ERROR_HRESULT_ACCESS_DENIED = unchecked((int)0x80070005);

This comment was marked as spam.

/// <summary>
/// Run Directory.Move and File.Move in Windows has a chance to get IOException with HResult 0x80070005. But this error is transient.
/// </summary>
internal static void RetryOnMoveAccessFailure(Action action)

This comment was marked as spam.

This comment was marked as spam.

@@ -47,5 +48,34 @@ public static class FileAccessRetrier
}
}
}

/// <summary>
/// Run Directory.Move and File.Move in Windows has a chance to get IOException with HResult 0x80070005. But this error is transient.

This comment was marked as spam.

catch (IOException e) when (e.HResult == ERROR_HRESULT_ACCESS_DENIED)
{
Thread.Sleep(nextWaitTime);
nextWaitTime *= 10;

This comment was marked as spam.

This comment was marked as spam.

internal static void RetryOnMoveAccessFailure(Action action)
{
const int ERROR_HRESULT_ACCESS_DENIED = unchecked((int)0x80070005);
int nextWaitTime = 1000;

This comment was marked as spam.

catch (IOException e) when (e.HResult == ERROR_HRESULT_ACCESS_DENIED)
{
Thread.Sleep(nextWaitTime);
nextWaitTime *= 2;

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@@ -47,5 +48,35 @@ public static class FileAccessRetrier
}
}
}

/// <summary>
/// Run Directory.Move and File.Move in Windows has a chance to get IOException with

This comment was marked as spam.

This comment was marked as spam.

@wli3
Copy link
Author

wli3 commented May 18, 2018

We decide to not fix in 2.1.300 and set it to known issue. Re targeting to 2.1.4xx

@wli3 wli3 changed the base branch from release/2.1.3xx to release/2.1.4xx May 18, 2018 22:27
@wli3
Copy link
Author

wli3 commented May 18, 2018

@livarcocc need your approval for check in 2.1.4xx

@wli3 wli3 modified the milestones: 2.1.3xx, 2.1.4xx May 18, 2018
@wli3 wli3 merged commit ca8a109 into dotnet:release/2.1.4xx May 22, 2018
@wli3 wli3 deleted the retry-directory.move2 branch May 22, 2018 16:55
wli3 pushed a commit to wli3/cli that referenced this pull request Jun 1, 2018
livarcocc added a commit to livarcocc/cli-1 that referenced this pull request Jun 8, 2018
* master: (31 commits)
  Updating signing project to use new intermediate directory (int).
  Update runtimeconfig.json doc for 2.1 (dotnet#9382)
  Shortening the path to the intermediate folder by renaming it to int.
  fix typo (dotnet#9364)
  Updating asp.net to 2.2.0 as well.
  Updating the build and tests to work with the 2.2.0 runtime.
  Simplified combining dictionaries in Telemetry
  Fixing 'Channel' and 'BranchName': "release/2.1.4xx" to "master" (dotnet#9362)
  Fix extraction of folders (dotnet#9335)
  Update Sha256Hasher.cs
  Fix relative path tool path (dotnet#9330)
  Insert updated SDK from 2.1.4xx branch
  MSBuild 15.8.60
  Fix crash when user home directory cannot be determined.
  Make `CliFolderPathCalculator` a static class.
  Don't add the ReleaseSuffix to the branding on the CLI when DropSuffix is set to true.
  Add retry when Directory.Move (dotnet#9313)
  Override new SdkResult public properties
  Add reference to Microsoft.Build.NuGetSdkResolver
  Disable crossgen for MSBuild inline-task refs
  ...

	README.md
	build/BranchInfo.props
	build/BundledTemplates.props
	build/DependencyVersions.props
	build/Version.props
	src/redist/redist.csproj
	test/EndToEnd/GivenAspNetAppsResolveImplicitVersions.cs
	tools/CrossGen.Dependencies/CrossGen.Dependencies.csproj
dotnet-maestro-bot pushed a commit to dotnet-maestro-bot/cli that referenced this pull request Aug 7, 2018
…e_master_cli

* 'master' of /Users/livarcocc/Documents/git/cli: (1063 commits)
  Updating signing project to use new intermediate directory (int).
  Update runtimeconfig.json doc for 2.1 (dotnet#9382)
  Shortening the path to the intermediate folder by renaming it to int.
  fix typo (dotnet#9364)
  Updating asp.net to 2.2.0 as well.
  Updating the build and tests to work with the 2.2.0 runtime.
  Simplified combining dictionaries in Telemetry
  Fixing 'Channel' and 'BranchName': "release/2.1.4xx" to "master" (dotnet#9362)
  Fix extraction of folders (dotnet#9335)
  Update Sha256Hasher.cs
  Fix relative path tool path (dotnet#9330)
  Insert updated SDK from 2.1.4xx branch
  MSBuild 15.8.60
  Fix crash when user home directory cannot be determined.
  Make `CliFolderPathCalculator` a static class.
  Don't add the ReleaseSuffix to the branding on the CLI when DropSuffix is set to true.
  Add retry when Directory.Move (dotnet#9313)
  Override new SdkResult public properties
  Add reference to Microsoft.Build.NuGetSdkResolver
  Disable crossgen for MSBuild inline-task refs
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants