[Xamarin.Android.Build.Tasks] fix potential AsyncTask.Log calls

[jonathanpeppers]

Changes: https://github.com/xamarin/monodroid/compare/78d56588...017b3618

Bump to xamarin/monodroid/master@017b3618

We have had a continuing concern of accidentally calling
AsyncTask.Log from a background thread. If done so, this can cause a
hang inside Visual Studio on Windows.

I finally had the idea of creating a build error if we inadvertently
do this:

[Obsolete("You should not use the 'Log' property directly for AsyncTask. Use the 'Log*' methods instead.", error: true)]
public new TaskLoggingHelper Log { get; }

This uncovered 26 build errors! Some were actually on the main thread
and completely OK, but several were not OK.

In most of the places, I could simply change Log.LogDebugMessage to
LogDebugMessage.

However a few of the more troublesome places:

• ManifestDocument required a TaskLoggingHelper for its ctor. I
  moved this to only the methods that need it.

• NdkUtils and NdkUtilsOld I used Action<string> and
  Action<string, string> callbacks instead.

• The <Aot/> task had an empty ValidateAotConfiguration method I
  removed.

Going forward, it should be a lot harder for us to introduce a hang by
using AsyncTask.Log.

[jonathanpeppers]

I actually found this was a real hang when using ReSharper:

I'll test and see if the changes here fix the issue.

[jonpryor]

I realize it has error:true, but should this also throw an exception for Really Good Measure?

Obsolete (..., error: true)]
public new TaskLoggingHelpe Log {
    get => throw new NotSupportedException ("Should not be reached!");
}

Admittedly, if the new Log property is accessed e.g. via Reflection, it'll "just" return null (and possibly NRE?), and it really shouldn't happen at all anyway, but... defense in depth? ¯_(ツ)_/¯

[dellis1972]

I would be careful about throwing an exception. the base class AsyncTask uses Log allot when on the UI thread. See https://github.com/xamarin/Xamarin.Build.AsyncTask/blob/master/Xamarin.Build.AsyncTask/AsyncTask.cs#L138 note all the #pragma stuff which ignore that Obsolete warning.

[jonathanpeppers]

Because I used the new keyword, the base class shouldn't be able to call the property. I think throwing might be better than the NRE, as I could put a better exception message.

[jonathanpeppers]

Or maybe I should just return base.Log? Might be the absolute safest option.

[dellis1972]

I actually found this was a real hang when using ReSharper:

I'll test and see if the changes here fix the issue.

@jonathanpeppers that is not the hang. That is where its manifesting. The WaitForCompletion is the method which waits on all the events while a task is running. It times out every 10 milliseconds

https://github.com/xamarin/Xamarin.Build.AsyncTask/blob/master/Xamarin.Build.AsyncTask/AsyncTask.cs#L284

That is by design and is exactly how the ToolTask works in MSbuild itself. So the problem isn't that method exactly, its something it trying to use the UI thread while that Wait is in that 10 millisecond "wait". That then causes the lockup. So the cause will be elsewhere.

[jonathanpeppers]

@dellis1972 sorry I didn't explain what the picture was.

In a new project, in the 16.5 Preview, I used ReSharper's "new class" dialog. This triggered a design-time build and the UI of VS was completely stuck: only option is to end task.

I saved a dump file of devenv.exe and opened it in a new VS. It was on this line, which means it was probably spinning in that loop forever:

Since CalculateLayoutCodeBehind had direct Log calls, could it be the cause?

I was going re-test with a build from master after this is merged, but I can probably try the build from this PR.

[jonathanpeppers]

I found the hang continued to happen until I changed:

-<Target Name="_FindLayoutsForBinding" Condition=" '$(Language)' == 'C#' ">
+<Target Name="_FindLayoutsForBinding" Condition=" '$(AndroidGenerateLayoutBindings)' == 'True' And '$(Language)' == 'C#' ">

I'll make that change in a different PR, probably should be doing this anyway?

[dellis1972]

@jonathanpeppers what you got was a deadlock, the IDE/UI thread was in System.Threading.WaitHandle.WaitAny and then something else tried to call a method in there.
Having Log.XXX calls on a background thread will definitely do that.