Skip to content

Commit

Permalink
[Xamarin.Android.Build.Tasks] random error in <Aapt2Link/>
Browse files Browse the repository at this point in the history
Context: http://build.devdiv.io/2547670
Context: dotnet#2911 (comment)

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<T>`, but it
seemed more appropriate to use a lock because of the `foreach` +
`Clear()` calls.
  • Loading branch information
jonathanpeppers committed Apr 1, 2019
1 parent 0355558 commit d1bf323
Showing 1 changed file with 7 additions and 4 deletions.
11 changes: 7 additions & 4 deletions src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Link.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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 ();
}
}

Expand Down Expand Up @@ -272,7 +274,8 @@ void ProcessManifest (ITaskItem manifestFile)
string GetTempFile ()
{
var temp = Path.GetTempFileName ();
tempFiles.Add (temp);
lock (tempFiles)
tempFiles.Add (temp);
return temp;
}
}
Expand Down

0 comments on commit d1bf323

Please sign in to comment.