From 278e101698269c9bc8840aa94d72e7f24066a96d Mon Sep 17 00:00:00 2001 From: Dean Ellis Date: Wed, 23 Oct 2024 16:37:06 +0100 Subject: [PATCH] [Xamarin.Android.Build.Tasks] Add retry ability to RemoveDirFixed (#9409) Fixes: https://github.com/dotnet/android/issues/9133 Context: https://github.com/dotnet/android-tools/commit/60fae1924e2d71f31dc1ed05f7346fd7874d6636 Much to our chagrin, in .NET 6+ it is possible for Design-Time Builds (DTBs) to run concurrently with "normal" builds, as there is nothing within the [.NET Project System][0] or [Common Project System (CPS)][1] which would actively *prevent* such concurrency. Consequently, it is possible to encounter locked files and directories during the build process, and user may see errors such as: Error (active) XARDF7019 System.UnauthorizedAccessException: Access to the path 'GoogleGson.dll' is denied. at System.IO.Directory.DeleteHelper(String fullPath, String userPath, Boolean recursive, Boolean throwOnTopLevelDirectoryNotFound, WIN32_FIND_DATA& data) at System.IO.Directory.Delete(String fullPath, String userPath, Boolean recursive, Boolean checkHost) at Xamarin.Android.Tasks.RemoveDirFixed.RunTask() in /Users/runner/work/1/s/xamarin-android/src/Xamarin.Android.Build.Tasks/Tasks/RemoveDirFixed.cs:line 54 MauiApp2 (net9.0-android) C:\Program Files\dotnet\packs\Microsoft.Android.Sdk.Windows\34.99.0-preview.6.340\tools\Xamarin.Android.Common.targets 2503 dotnet/android-tools@60fae192 introduced the concept of a "Retry" in the cases of `UnauthorizedAccessException`s or `IOException`s when the code is `ACCESS_DENIED` or `ERROR_SHARING_VIOLATION`. Builds upon that work to use the API's added to add retry semantics to the `` task. This also simplifies the Task somewhat as it had quite complex exception handling. [0]: https://github.com/dotnet/project-system [1]: https://github.com/microsoft/VSProjectSystem --- .../Tasks/RemoveDirFixed.cs | 60 +++++++++++-------- .../Tasks/RemoveDirTests.cs | 41 +++++++++++++ 2 files changed, 76 insertions(+), 25 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/RemoveDirFixed.cs b/src/Xamarin.Android.Build.Tasks/Tasks/RemoveDirFixed.cs index c456b87eefd..da33725f0e6 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/RemoveDirFixed.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/RemoveDirFixed.cs @@ -28,6 +28,8 @@ using System; using System.Collections.Generic; using System.IO; +using System.Threading; +using System.Runtime.InteropServices; using Microsoft.Build.Framework; using Xamarin.Android.Tools; using Microsoft.Android.Build.Tasks; @@ -38,6 +40,9 @@ public class RemoveDirFixed : AndroidTask { public override string TaskPrefix => "RDF"; + const int ERROR_ACCESS_DENIED = -2147024891; + const int ERROR_SHARING_VIOLATION = -2147024864; + public override bool RunTask () { var temporaryRemovedDirectories = new List (Directories.Length); @@ -48,36 +53,41 @@ public override bool RunTask () Log.LogDebugMessage ($"Directory did not exist: {fullPath}"); continue; } + int retryCount = 0; + int attempts = Files.GetFileWriteRetryAttempts (); + int delay = Files.GetFileWriteRetryDelay (); try { - // try to do a normal "fast" delete of the directory. - Directory.Delete (fullPath, true); - temporaryRemovedDirectories.Add (directory); - } catch (UnauthorizedAccessException ex) { - // if that fails we probably have readonly files (or locked files) - // so try to make them writable and try again. - try { - Files.SetDirectoryWriteable (fullPath); - Directory.Delete (fullPath, true); - temporaryRemovedDirectories.Add (directory); - } catch (Exception inner) { - Log.LogUnhandledException (TaskPrefix, ex); - Log.LogUnhandledException (TaskPrefix, inner); - } - } catch (DirectoryNotFoundException ex) { - // This could be a file inside the directory over MAX_PATH. - // We can attempt using the \\?\ syntax. - if (OS.IsWindows) { + while (retryCount <= attempts) { try { - fullPath = Files.ToLongPath (fullPath); - Log.LogDebugMessage ("Trying long path: " + fullPath); + // try to do a normal "fast" delete of the directory. + // only do the set writable on the second attempt + if (retryCount == 1) + Files.SetDirectoryWriteable (fullPath); Directory.Delete (fullPath, true); temporaryRemovedDirectories.Add (directory); - } catch (Exception inner) { - Log.LogUnhandledException (TaskPrefix, ex); - Log.LogUnhandledException (TaskPrefix, inner); + break; + } catch (Exception e) { + switch (e) { + case DirectoryNotFoundException: + if (OS.IsWindows) { + fullPath = Files.ToLongPath (fullPath); + Log.LogDebugMessage ("Trying long path: " + fullPath); + break; + } + throw; + case UnauthorizedAccessException: + case IOException: + int code = Marshal.GetHRForException(e); + if ((code != ERROR_ACCESS_DENIED && code != ERROR_SHARING_VIOLATION) || retryCount >= attempts) { + throw; + }; + break; + default: + throw; + } + Thread.Sleep (delay); + retryCount++; } - } else { - Log.LogUnhandledException (TaskPrefix, ex); } } catch (Exception ex) { Log.LogUnhandledException (TaskPrefix, ex); diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/RemoveDirTests.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/RemoveDirTests.cs index bf93d07faca..20a828e8f4c 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/RemoveDirTests.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/RemoveDirTests.cs @@ -7,6 +7,8 @@ using Xamarin.Android.Tasks; using Xamarin.Android.Tools; using Microsoft.Android.Build.Tasks; +using TPL = System.Threading.Tasks; +using System.Threading; namespace Xamarin.Android.Build.Tests { @@ -83,5 +85,44 @@ public void LongPath () Assert.AreEqual (1, task.RemovedDirectories.Length, "Changes should have been made."); DirectoryAssert.DoesNotExist (tempDirectory); } + + [Test, Category ("SmokeTests")] + public void DirectoryInUse () + { + if (OS.IsMac) { + Assert.Ignore ("This is not an issue on macos."); + return; + } + var file = NewFile (); + var task = CreateTask (); + using (var f = File.OpenWrite (file)) { + Assert.IsFalse (task.Execute (), "task.Execute() should have failed."); + Assert.AreEqual (0, task.RemovedDirectories.Length, "Changes should not have been made."); + DirectoryAssert.Exists (tempDirectory); + } + } + + [Test, Category ("SmokeTests")] + public async TPL.Task DirectoryInUseWithRetry () + { + if (OS.IsMac) { + Assert.Ignore ("This is not an issue on macos."); + return; + } + var file = NewFile (); + var task = CreateTask (); + var ev = new ManualResetEvent (false); + var t = TPL.Task.Run (async () => { + using (var f = File.OpenWrite (file)) { + ev.Set (); + await TPL.Task.Delay (2500); + } + }); + ev.WaitOne (); + Assert.IsTrue (task.Execute (), "task.Execute() should have succeeded."); + Assert.AreEqual (1, task.RemovedDirectories.Length, "Changes should have been made."); + DirectoryAssert.DoesNotExist (tempDirectory); + await t; + } } }