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

[Xamarin.Android.Build.Tasks] helpers for AsyncTask + TPL #2881

Merged
merged 1 commit into from
Mar 29, 2019

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Mar 26, 2019

Unhandled exceptions from a Parallel.ForEach log an exception in
MSBuild such as:

E:\A\_work\1214\s\bin\Release\lib\xamarin.android\xbuild\Xamarin\Android\Xamarin.Android.Aapt2.targets(84,3): One or more errors occurred.
    at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
    at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
    at System.Threading.Tasks.Task.Wait()
    at System.Threading.Tasks.Parallel.ForWorker[TLocal](Int32 fromInclusive, Int32 toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
    at System.Threading.Tasks.Parallel.ForEachWorker[TSource,TLocal](TSource[] array, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
    at System.Threading.Tasks.Parallel.ForEachWorker[TSource,TLocal](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
    at System.Threading.Tasks.Parallel.ForEach[TSource](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body)
    at Xamarin.Android.Tasks.Aapt2Compile.DoExecute()
    at Xamarin.Android.Tasks.Aapt2Compile.<Execute>b__15_0()
    at System.Threading.Tasks.Task.InnerInvoke()
    at System.Threading.Tasks.Task.Execute() [E:\A\_work\1214\s\bin\TestRelease\temp\BuildAMassiveApp\App1\App1.csproj]

One or more errors occurred is not helpful, and it looks like this
could happen in several places throughout our MSBuild tasks. The only
way to get a better error is to add a try-catch and manually log the
exception.

A couple extension methods can simplify where we use the PCL, and fix
this problem at the same time:

  • Parallel.ForEach -> this.ParallelForEach():
    • Can setup ParallelOptions & CancellationToken
    • Adds a try-catch block that reports XA0000 on unhandled
      exceptions
    • Calls AsyncTask.Cancel() if there is an exception
  • this.ParallelForEachWithLock passes an object lockObject to the
    inner method for any needed locking.
  • Task.Run -> this.RunTask()
    • Sets up the CancellationToken

With these extension methods we can also drop this using in several
files:

using ThreadingTasks = System.Threading.Tasks;

It is a bit annoying to have to disambiguate an MSBuild task from a
threading task.

@jonathanpeppers
Copy link
Member Author

Aha! the error is:

Unhandled exception: System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at System.Collections.Generic.List`1.Add(T item)
   at Xamarin.Android.Tasks.Aapt2Compile.ProcessDirectory(ITaskItem resourceDirectory)

I think this should also throw; to abort the Parallel.ForEach otherwise I got this many errors:

image

Or maybe there is some usage of CancellationToken to cleanly exit.

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Mar 27, 2019
Context: dotnet#2881
Context: http://build.devdiv.io/2531303

I have noticed a random failure in our MSBuild tests such as:

    E:\A\_work\1214\s\bin\Release\lib\xamarin.android\xbuild\Xamarin\Android\Xamarin.Android.Aapt2.targets(84,3): One or more errors occurred.
        at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
        at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
        at System.Threading.Tasks.Task.Wait()
        at System.Threading.Tasks.Parallel.ForWorker[TLocal](Int32 fromInclusive, Int32 toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
        at System.Threading.Tasks.Parallel.ForEachWorker[TSource,TLocal](TSource[] array, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
        at System.Threading.Tasks.Parallel.ForEachWorker[TSource,TLocal](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
        at System.Threading.Tasks.Parallel.ForEach[TSource](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body)
        at Xamarin.Android.Tasks.Aapt2Compile.DoExecute()
        at Xamarin.Android.Tasks.Aapt2Compile.<Execute>b__15_0()
        at System.Threading.Tasks.Task.InnerInvoke()
        at System.Threading.Tasks.Task.Execute() [E:\A\_work\1214\s\bin\TestRelease\temp\BuildAMassiveApp\App1\App1.csproj]

So an exception during `Parallel.ForEach` isn't logged in a nice way
at all with MSBuild...

Additionally, the `aapt2 compile` calls before this do not list any
errors...

So with dotnet#2881, I added `try-catch` to log the inner exception:

    Unhandled exception: System.IndexOutOfRangeException: Index was outside the bounds of the array.
        at System.Collections.Generic.List`1.Add(T item)
        at Xamarin.Android.Tasks.Aapt2Compile.ProcessDirectory(ITaskItem resourceDirectory)

That is when I spotted the issue:

* Running `Parallel.ForEach` on `ProcessDirectory`
* Parallel calls to `archives.Add()`, where `archives` is a
  `List<ITaskItem>` instance variable in the MSBuild task.

The simple fix here is to put a `lock(foo)` around the `.Add()` call.

I will improve the error logging in dotnet#2881.
@jonathanpeppers
Copy link
Member Author

With the latest change to call Cancel() the errors are more reasonable:

image

We still got 5 errors, but were getting like 20 before.

jonpryor pushed a commit that referenced this pull request Mar 27, 2019
Context: #2881
Context: http://build.devdiv.io/2531303

I have noticed a random failure in our MSBuild tests such as:

	E:\A\_work\1214\s\bin\Release\lib\xamarin.android\xbuild\Xamarin\Android\Xamarin.Android.Aapt2.targets(84,3): One or more errors occurred.
	    at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
	    at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
	    at System.Threading.Tasks.Task.Wait()
	    at System.Threading.Tasks.Parallel.ForWorker[TLocal](Int32 fromInclusive, Int32 toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
	    at System.Threading.Tasks.Parallel.ForEachWorker[TSource,TLocal](TSource[] array, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
	    at System.Threading.Tasks.Parallel.ForEachWorker[TSource,TLocal](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
	    at System.Threading.Tasks.Parallel.ForEach[TSource](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body)
	    at Xamarin.Android.Tasks.Aapt2Compile.DoExecute()
	    at Xamarin.Android.Tasks.Aapt2Compile.<Execute>b__15_0()
	    at System.Threading.Tasks.Task.InnerInvoke()
	    at System.Threading.Tasks.Task.Execute() [E:\A\_work\1214\s\bin\TestRelease\temp\BuildAMassiveApp\App1\App1.csproj]

An exception during `Parallel.ForEach()` isn't logged in a nice way
at all with MSBuild.  Additionally, the `aapt2 compile` calls before
this do not list any errors.

With #2881, I added `try-catch` to log the inner exception, and got:

	Unhandled exception: System.IndexOutOfRangeException: Index was outside the bounds of the array.
	    at System.Collections.Generic.List`1.Add(T item)
	    at Xamarin.Android.Tasks.Aapt2Compile.ProcessDirectory(ITaskItem resourceDirectory)

That is when I spotted the issue:

  * Running `Parallel.ForEach` on `ProcessDirectory()`
  * Parallel calls to `archives.Add()`, where `archives` is a
    `List<ITaskItem>` instance variable in the MSBuild task.

The simple fix here is to put a `lock(lockObject)` around the
`archives.Add()` call.
@jonathanpeppers jonathanpeppers changed the title [Xamarin.Android.Build.Tasks] aapt2 logging for Parallel.ForEach [Xamarin.Android.Build.Tasks] helpers for AsyncTask + PCL Mar 27, 2019
@jonathanpeppers jonathanpeppers changed the title [Xamarin.Android.Build.Tasks] helpers for AsyncTask + PCL [Xamarin.Android.Build.Tasks] helpers for AsyncTask + TPL Mar 27, 2019
@jonpryor
Copy link
Member

@monojenkins build

Unhandled exceptions from a `Parallel.ForEach` log an exception in
MSBuild such as:

    E:\A\_work\1214\s\bin\Release\lib\xamarin.android\xbuild\Xamarin\Android\Xamarin.Android.Aapt2.targets(84,3): One or more errors occurred.
        at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
        at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
        at System.Threading.Tasks.Task.Wait()
        at System.Threading.Tasks.Parallel.ForWorker[TLocal](Int32 fromInclusive, Int32 toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
        at System.Threading.Tasks.Parallel.ForEachWorker[TSource,TLocal](TSource[] array, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
        at System.Threading.Tasks.Parallel.ForEachWorker[TSource,TLocal](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
        at System.Threading.Tasks.Parallel.ForEach[TSource](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body)
        at Xamarin.Android.Tasks.Aapt2Compile.DoExecute()
        at Xamarin.Android.Tasks.Aapt2Compile.<Execute>b__15_0()
        at System.Threading.Tasks.Task.InnerInvoke()
        at System.Threading.Tasks.Task.Execute() [E:\A\_work\1214\s\bin\TestRelease\temp\BuildAMassiveApp\App1\App1.csproj]

`One or more errors occurred` is not helpful, and it looks like this
could happen in several places throughout our MSBuild tasks. The only
way to get a better error is to add a `try-catch` and manually log the
exception.

A couple extension methods can simplify where we use the PCL, and fix
this problem at the same time:

* `Parallel.ForEach` -> `this.ParallelForEach()`:
  * Can setup `ParallelOptions` & `CancellationToken`
  * Adds a `try-catch` block that reports `XA0000` on unhandled
    exceptions
  * Calls `AsyncTask.Cancel()` if there is an exception
* `this.ParallelForEachWithLock` passes an `object lockObject` to the
  inner method for any needed locking.
* `Task.Run` -> `this.RunTask()`
  * Sets up the `CancellationToken`

With these extension methods we can also drop this `using` in several
files:

    using ThreadingTasks = System.Threading.Tasks;

It is a bit annoying to have to disambiguate an MSBuild task from a
threading task.
@jonathanpeppers jonathanpeppers marked this pull request as ready for review March 28, 2019 15:46
@jonathanpeppers
Copy link
Member Author

jonathanpeppers commented Mar 29, 2019

I think all the build failures are OK:

@jonpryor jonpryor merged commit 040c92a into dotnet:master Mar 29, 2019
@jonathanpeppers jonathanpeppers deleted the aapt2-unhandled-exception branch March 29, 2019 17:09
jonpryor pushed a commit that referenced this pull request Apr 3, 2019
Context: #2881
Context: http://build.devdiv.io/2531303

I have noticed a random failure in our MSBuild tests such as:

	E:\A\_work\1214\s\bin\Release\lib\xamarin.android\xbuild\Xamarin\Android\Xamarin.Android.Aapt2.targets(84,3): One or more errors occurred.
	    at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
	    at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
	    at System.Threading.Tasks.Task.Wait()
	    at System.Threading.Tasks.Parallel.ForWorker[TLocal](Int32 fromInclusive, Int32 toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
	    at System.Threading.Tasks.Parallel.ForEachWorker[TSource,TLocal](TSource[] array, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
	    at System.Threading.Tasks.Parallel.ForEachWorker[TSource,TLocal](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Action`3 bodyWithStateAndIndex, Func`4 bodyWithStateAndLocal, Func`5 bodyWithEverything, Func`1 localInit, Action`1 localFinally)
	    at System.Threading.Tasks.Parallel.ForEach[TSource](IEnumerable`1 source, ParallelOptions parallelOptions, Action`1 body)
	    at Xamarin.Android.Tasks.Aapt2Compile.DoExecute()
	    at Xamarin.Android.Tasks.Aapt2Compile.<Execute>b__15_0()
	    at System.Threading.Tasks.Task.InnerInvoke()
	    at System.Threading.Tasks.Task.Execute() [E:\A\_work\1214\s\bin\TestRelease\temp\BuildAMassiveApp\App1\App1.csproj]

An exception during `Parallel.ForEach()` isn't logged in a nice way
at all with MSBuild.  Additionally, the `aapt2 compile` calls before
this do not list any errors.

With #2881, I added `try-catch` to log the inner exception, and got:

	Unhandled exception: System.IndexOutOfRangeException: Index was outside the bounds of the array.
	    at System.Collections.Generic.List`1.Add(T item)
	    at Xamarin.Android.Tasks.Aapt2Compile.ProcessDirectory(ITaskItem resourceDirectory)

That is when I spotted the issue:

  * Running `Parallel.ForEach` on `ProcessDirectory()`
  * Parallel calls to `archives.Add()`, where `archives` is a
    `List<ITaskItem>` instance variable in the MSBuild task.

The simple fix here is to put a `lock(lockObject)` around the
`archives.Add()` call.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants