From a02c81ecc6e102729e14ccac038f971eb8cbc2cb Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Tue, 2 Apr 2019 04:32:50 -0500 Subject: [PATCH] [Xamarin.Android.Build.Tasks] random error in (#2914) Context: http://build.devdiv.io/2547670 Context: https://github.com/xamarin/xamarin-android/pull/2911#issuecomment-478739173 We have been occasionally hitting this on CI: Xamarin.Android.Aapt2.targets(146,3): Value cannot be null. Parameter name: path at System.IO.File.Delete(String path) at Xamarin.Android.Tasks.Aapt2Link.DoExecute() at System.Threading.Tasks.Task.InnerInvoke() at System.Threading.Tasks.Task.Execute() [E:\A\_work\1214\s\bin\TestRelease\temp\CompileBeforeUpgradingNuGet\UnnamedProject.csproj] What I think is happening: * We call `tempFiles.Add()` during a `Parallel.ForEach`. Bad... * Instead of an exception happening during `Add()`, a `null` is somehow added to the underlying array--which is corrupted. Instead I think we should add locking around all usage of this `tempFiles` member variable. I looked at `ConcurrentBag`, but it seemed more appropriate to use a lock because of the `foreach` + `Clear()` calls. --- src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Link.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Link.cs b/src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Link.cs index edb299c4734..ba5f28fea4b 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Link.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Link.cs @@ -101,10 +101,12 @@ void DoExecute () this.ParallelForEach (ManifestFiles, ProcessManifest); } finally { - foreach (var temp in tempFiles) { - File.Delete (temp); + lock (tempFiles) { + foreach (var temp in tempFiles) { + File.Delete (temp); + } + tempFiles.Clear (); } - tempFiles.Clear (); } } @@ -272,7 +274,8 @@ void ProcessManifest (ITaskItem manifestFile) string GetTempFile () { var temp = Path.GetTempFileName (); - tempFiles.Add (temp); + lock (tempFiles) + tempFiles.Add (temp); return temp; } }