Skip to content

Commit

Permalink
[Xamarin.Android.Build.Tasks] fix potential AsyncTask.Log calls
Browse files Browse the repository at this point in the history 
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`.
  • Loading branch information
@jonathanpeppers
jonathanpeppers committed Jan 28, 2020
1 parent a2a76c3 commit 8e24138
Show file tree
Hide file tree
Showing 14 changed files with 121 additions and 106 deletions.
2 changes: 1 addition & 1 deletion .external
View file
@@ -1,2 +1,2 @@
xamarin/monodroid:master@78d565884f7dd2cf76e748125485f7ed0e03eec4
xamarin/monodroid:asynctask.log@d41bb23122094f2bc11055927e84ac8165711595
mono/mono:2019-10@18920a83f423fb864a2263948737681968f5b2c8
10 changes: 5 additions & 5 deletions src/Xamarin.Android.Build.Tasks/Tasks/Aapt.cs
View file
Expand Up @@ -267,25 +267,25 @@ protected string GenerateCommandLineCommands (string ManifestFile, string curren

Directory.CreateDirectory (manifestDir);
manifestFile = Path.Combine (manifestDir, Path.GetFileName (ManifestFile));
ManifestDocument manifest = new ManifestDocument (ManifestFile, this.Log);
ManifestDocument manifest = new ManifestDocument (ManifestFile);
manifest.SdkVersion = AndroidSdkPlatform;
if (!string.IsNullOrEmpty (VersionCodePattern)) {
try {
manifest.CalculateVersionCode (currentAbi, VersionCodePattern, VersionCodeProperties);
} catch (ArgumentOutOfRangeException ex) {
Log.LogCodedError ("XA0003", ManifestFile, 0, ex.Message);
LogCodedError ("XA0003", ManifestFile, 0, ex.Message);
return string.Empty;
}
}
if (currentAbi != null && string.IsNullOrEmpty (VersionCodePattern)) {
manifest.SetAbi (currentAbi);
}
if (!manifest.ValidateVersionCode (out string error, out string errorCode)) {
Log.LogCodedError (errorCode, ManifestFile, 0, error);
LogCodedError (errorCode, ManifestFile, 0, error);
return string.Empty;
}
manifest.ApplicationName = ApplicationName;
manifest.Save (manifestFile);
manifest.Save (LogWarning, manifestFile);

cmd.AppendSwitchIfNotNull ("-M ", manifestFile);
var designerDirectory = Path.IsPathRooted (JavaDesignerOutputDirectory) ? JavaDesignerOutputDirectory : Path.Combine (WorkingDirectory, JavaDesignerOutputDirectory);
Expand Down Expand Up @@ -348,7 +348,7 @@ protected string GenerateCommandLineCommands (string ManifestFile, string curren

var extraArgsExpanded = ExpandString (ExtraArgs);
if (extraArgsExpanded != ExtraArgs)
Log.LogDebugMessage (" ExtraArgs expanded: {0}", extraArgsExpanded);
LogDebugMessage (" ExtraArgs expanded: {0}", extraArgsExpanded);

if (!string.IsNullOrWhiteSpace (extraArgsExpanded))
cmd.AppendSwitch (extraArgsExpanded);
Expand Down
14 changes: 7 additions & 7 deletions src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Link.cs
View file
Expand Up @@ -107,25 +107,25 @@ string GenerateCommandLineCommands (string ManifestFile, string currentAbi, stri
string manifestDir = Path.Combine (Path.GetDirectoryName (ManifestFile), currentAbi != null ? currentAbi : "manifest");
Directory.CreateDirectory (manifestDir);
string manifestFile = Path.Combine (manifestDir, Path.GetFileName (ManifestFile));
ManifestDocument manifest = new ManifestDocument (ManifestFile, this.Log);
ManifestDocument manifest = new ManifestDocument (ManifestFile);
manifest.SdkVersion = AndroidSdkPlatform;
if (!string.IsNullOrEmpty (VersionCodePattern)) {
try {
manifest.CalculateVersionCode (currentAbi, VersionCodePattern, VersionCodeProperties);
} catch (ArgumentOutOfRangeException ex) {
Log.LogCodedError ("XA0003", ManifestFile, 0, ex.Message);
LogCodedError ("XA0003", ManifestFile, 0, ex.Message);
return string.Empty;
}
}
if (currentAbi != null && string.IsNullOrEmpty (VersionCodePattern)) {
manifest.SetAbi (currentAbi);
}
if (!manifest.ValidateVersionCode (out string error, out string errorCode)) {
Log.LogCodedError (errorCode, ManifestFile, 0, error);
LogCodedError (errorCode, ManifestFile, 0, error);
return string.Empty;
}
manifest.ApplicationName = ApplicationName;
manifest.Save (manifestFile);
manifest.Save (LogWarning, manifestFile);

cmd.AppendSwitchIfNotNull ("--manifest ", manifestFile);
if (!string.IsNullOrEmpty (JavaDesignerOutputDirectory)) {
Expand All @@ -142,7 +142,7 @@ string GenerateCommandLineCommands (string ManifestFile, string currentAbi, stri
if (File.Exists (flata)) {
cmd.AppendSwitchIfNotNull ("-R ", flata);
} else {
Log.LogDebugMessage ("Archive does not exist: " + flata);
LogDebugMessage ("Archive does not exist: " + flata);
}
}
}
Expand All @@ -152,7 +152,7 @@ string GenerateCommandLineCommands (string ManifestFile, string currentAbi, stri
if (File.Exists (flata)) {
cmd.AppendSwitchIfNotNull ("-R ", flata);
} else {
Log.LogDebugMessage ("Archive does not exist: " + flata);
LogDebugMessage ("Archive does not exist: " + flata);
}
}

Expand All @@ -175,7 +175,7 @@ string GenerateCommandLineCommands (string ManifestFile, string currentAbi, stri

var extraArgsExpanded = ExpandString (ExtraArgs);
if (extraArgsExpanded != ExtraArgs)
Log.LogDebugMessage (" ExtraArgs expanded: {0}", extraArgsExpanded);
LogDebugMessage (" ExtraArgs expanded: {0}", extraArgsExpanded);

if (!string.IsNullOrWhiteSpace (extraArgsExpanded))
cmd.AppendSwitch (extraArgsExpanded);
Expand Down
5 changes: 4 additions & 1 deletion src/Xamarin.Android.Build.Tasks/Tasks/AndroidTask.cs
View file
Expand Up @@ -34,12 +34,15 @@ public abstract class AndroidAsyncTask : AsyncTask

public abstract string TaskPrefix { get; }

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

public override bool Execute ()
{
try {
return RunTask ();
} catch (Exception ex) {
Log.LogUnhandledException (TaskPrefix, ex);
this.LogUnhandledException (TaskPrefix, ex);
return false;
}
}
Expand Down
14 changes: 2 additions & 12 deletions src/Xamarin.Android.Build.Tasks/Tasks/Aot.cs
View file
Expand Up @@ -84,7 +84,7 @@ public class Aot : AndroidAsyncTask

public override bool RunTask ()
{
if (EnableLLVM && !NdkUtil.Init (Log, AndroidNdkDirectory))
if (EnableLLVM && !NdkUtil.Init (LogCodedError, AndroidNdkDirectory))
return false;

return base.RunTask ();
Expand Down Expand Up @@ -176,11 +176,6 @@ static string QuoteFileName(string fileName)
return builder.ToString();
}

static bool ValidateAotConfiguration (TaskLoggingHelper log, AndroidTargetArch arch, bool enableLLVM)
{
return true;
}

int GetNdkApiLevel(string androidNdkPath, string androidApiLevel, AndroidTargetArch arch)
{
var manifest = AndroidAppManifest.Load (ManifestFile.ItemSpec, MonoAndroidHelper.SupportedVersions);
Expand Down Expand Up @@ -312,12 +307,7 @@ IEnumerable<Config> GetAotConfigs ()
throw new Exception ("Unsupported Android target architecture ABI: " + abi);
}

if (EnableLLVM && !NdkUtil.ValidateNdkPlatform (Log, AndroidNdkDirectory, arch, enableLLVM:EnableLLVM)) {
yield return Config.Invalid;
yield break;
}

if (!ValidateAotConfiguration(Log, arch, EnableLLVM)) {
if (EnableLLVM && !NdkUtil.ValidateNdkPlatform (LogMessage, LogCodedError, AndroidNdkDirectory, arch, enableLLVM:EnableLLVM)) {
yield return Config.Invalid;
yield break;
}
Expand Down
10 changes: 5 additions & 5 deletions src/Xamarin.Android.Build.Tasks/Tasks/CalculateLayoutCodeBehind.cs
View file
Expand Up @@ -119,7 +119,7 @@ public async override System.Threading.Tasks.Task RunTaskAsync ()

GenerateLayoutBindings.BindingGeneratorLanguage gen;
if (!GenerateLayoutBindings.KnownBindingGenerators.TryGetValue (OutputLanguage, out gen) || gen == null) {
Log.LogDebugMessage ($"Language {OutputLanguage} isn't supported, will use {GenerateLayoutBindings.DefaultOutputGenerator.Name} instead");
LogDebugMessage ($"Language {OutputLanguage} isn't supported, will use {GenerateLayoutBindings.DefaultOutputGenerator.Name} instead");
sourceFileExtension = GenerateLayoutBindings.DefaultOutputGenerator.Extension;
} else
sourceFileExtension = OutputFileExtension;
Expand All @@ -138,7 +138,7 @@ public async override System.Threading.Tasks.Task RunTaskAsync ()
if (layoutsByName.Count >= ParallelGenerationThreshold) {
// NOTE: Update the tests in $TOP_DIR/tests/CodeBehind/UnitTests/BuildTests.cs if this message
// is changed!
Log.LogDebugMessage ($"Parsing layouts in parallel (threshold of {ParallelGenerationThreshold} layouts met)");
LogDebugMessage ($"Parsing layouts in parallel (threshold of {ParallelGenerationThreshold} layouts met)");

await this.WhenAll (layoutsByName, kvp =>
ParseAndLoadGroup (layoutsByName, kvp.Key, kvp.Value.InputItems, ref kvp.Value.LayoutBindingItems, ref kvp.Value.LayoutPartialClassItems));
Expand All @@ -160,11 +160,11 @@ public async override System.Threading.Tasks.Task RunTaskAsync ()

LayoutBindingFiles = layoutBindingFiles.ToArray ();
if (LayoutBindingFiles.Length == 0)
Log.LogDebugMessage (" No layout file qualifies for code-behind generation");
LogDebugMessage (" No layout file qualifies for code-behind generation");
LayoutPartialClassFiles = layoutPartialClassFiles.ToArray ();

Log.LogDebugTaskItems (" LayoutBindingFiles:", LayoutBindingFiles, true);
Log.LogDebugTaskItems (" LayoutPartialClassFiles:", LayoutPartialClassFiles, true);
LogDebugTaskItems (" LayoutBindingFiles:", LayoutBindingFiles);
LogDebugTaskItems (" LayoutPartialClassFiles:", LayoutPartialClassFiles);
}

void ParseAndLoadGroup (Dictionary <string, LayoutGroup> groupIndex, string groupName, List<ITaskItem> items, ref List<ITaskItem> layoutBindingFiles, ref List<ITaskItem> layoutPartialClassFiles)
Expand Down
2 changes: 1 addition & 1 deletion src/Xamarin.Android.Build.Tasks/Tasks/CheckGoogleSdkRequirements.cs
View file
Expand Up @@ -22,7 +22,7 @@ public class CheckGoogleSdkRequirements : AndroidTask

public override bool RunTask ()
{
ManifestDocument manifest = new ManifestDocument (ManifestFile, this.Log);
ManifestDocument manifest = new ManifestDocument (ManifestFile);

var compileSdk = MonoAndroidHelper.SupportedVersions.GetApiLevelFromFrameworkVersion (TargetFrameworkVersion);

Expand Down
6 changes: 3 additions & 3 deletions src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
View file
Expand Up @@ -236,7 +236,7 @@ void Run (DirectoryAssemblyResolver res)
}

// Step 3 - Merge [Activity] and friends into AndroidManifest.xml
var manifest = new ManifestDocument (ManifestTemplate, this.Log);
var manifest = new ManifestDocument (ManifestTemplate);

manifest.PackageName = PackageName;
manifest.ApplicationName = ApplicationName ?? PackageName;
Expand All @@ -250,10 +250,10 @@ void Run (DirectoryAssemblyResolver res)
manifest.NeedsInternet = NeedsInternet;
manifest.InstantRunEnabled = InstantRunEnabled;

var additionalProviders = manifest.Merge (all_java_types, ApplicationJavaClass, EmbedAssemblies, BundledWearApplicationName, MergedManifestDocuments);
var additionalProviders = manifest.Merge (Log, all_java_types, ApplicationJavaClass, EmbedAssemblies, BundledWearApplicationName, MergedManifestDocuments);

using (var stream = new MemoryStream ()) {
manifest.Save (stream);
manifest.Save (Log, stream);

// Only write the new manifest if it actually changed
MonoAndroidHelper.CopyIfStreamChanged (stream, MergedAndroidManifestOutput);
Expand Down
14 changes: 7 additions & 7 deletions src/Xamarin.Android.Build.Tasks/Tasks/GenerateLayoutBindings.cs
View file
Expand Up @@ -96,17 +96,17 @@ public async override System.Threading.Tasks.Task RunTaskAsync ()
BindingGenerator generator = GetBindingGenerator (OutputLanguage);

if (generator == null) {
Log.LogMessage ($"Unknown binding output language '{OutputLanguage}', will use {DefaultOutputGenerator.Name} instead");
LogMessage ($"Unknown binding output language '{OutputLanguage}', will use {DefaultOutputGenerator.Name} instead");
generator = DefaultOutputGenerator.Creator ();
}

if (generator == null) {
// Should "never" happen
Log.LogError ($"Cannot find binding generator for language {OutputLanguage} or {DefaultOutputGenerator.Name}");
LogError ($"Cannot find binding generator for language {OutputLanguage} or {DefaultOutputGenerator.Name}");
return;
}

Log.LogDebugMessage ($"Generating {generator.LanguageName} binding sources");
LogDebugMessage ($"Generating {generator.LanguageName} binding sources");

var layoutGroups = new Dictionary <string, LayoutGroup> (StringComparer.Ordinal);
string layoutGroupName;
Expand All @@ -133,7 +133,7 @@ public async override System.Threading.Tasks.Task RunTaskAsync ()
if (!GetRequiredMetadata (item, CalculateLayoutCodeBehind.LayoutGroupMetadata, out layoutGroupName))
return;
if (!layoutGroups.TryGetValue (layoutGroupName, out group) || group == null) {
Log.LogError ($"Partial class item without associated binding for layout group '{layoutGroupName}'");
LogError ($"Partial class item without associated binding for layout group '{layoutGroupName}'");
return;
}

Expand Down Expand Up @@ -177,7 +177,7 @@ public async override System.Threading.Tasks.Task RunTaskAsync ()
if (ResourceFiles.Length >= CalculateLayoutCodeBehind.ParallelGenerationThreshold) {
// NOTE: Update the tests in $TOP_DIR/tests/CodeBehind/UnitTests/BuildTests.cs if this message
// is changed!
Log.LogDebugMessage ($"Generating binding code in parallel (threshold of {CalculateLayoutCodeBehind.ParallelGenerationThreshold} layouts met)");
LogDebugMessage ($"Generating binding code in parallel (threshold of {CalculateLayoutCodeBehind.ParallelGenerationThreshold} layouts met)");
var fileSet = new ConcurrentBag <string> ();
await this.WhenAll (layoutGroups, kvp => GenerateSourceForLayoutGroup (generator, kvp.Value, rpath => fileSet.Add (rpath)));
generatedFilePaths = fileSet;
Expand All @@ -190,8 +190,8 @@ public async override System.Threading.Tasks.Task RunTaskAsync ()

GeneratedFiles = generatedFilePaths.Where (gfp => !String.IsNullOrEmpty (gfp)).Select (gfp => new TaskItem (gfp)).ToArray ();
if (GeneratedFiles.Length == 0)
Log.LogWarning ("No layout binding source files generated");
Log.LogDebugTaskItems (" GeneratedFiles:", GeneratedFiles);
LogWarning ("No layout binding source files generated");
LogDebugTaskItems (" GeneratedFiles:", GeneratedFiles);
}

void GenerateSourceForLayoutGroup (BindingGenerator generator, LayoutGroup group, Action <string> pathAdder)
Expand Down
4 changes: 2 additions & 2 deletions src/Xamarin.Android.Build.Tasks/Tasks/ManifestMerger.cs
View file
Expand Up @@ -39,9 +39,9 @@ public override bool Execute ()
bool result = base.Execute ();
if (!result)
return result;
var m = new ManifestDocument (tempFile, Log);
var m = new ManifestDocument (tempFile);
using (var ms = new MemoryStream ()) {
m.Save (ms);
m.Save (Log, ms);
MonoAndroidHelper.CopyIfStreamChanged (ms, OutputManifestFile);
return result;
}
Expand Down
26 changes: 17 additions & 9 deletions src/Xamarin.Android.Build.Tasks/Tasks/NdkUtils.cs
View file
Expand Up @@ -25,18 +25,21 @@ public static class NdkUtil

public static bool Init (string ndkPath)
{
return Init (null, ndkPath); // For tests which don't have access to a TaskLoggingHelper
return Init (delegate { }, ndkPath); // For tests which don't have access to a TaskLoggingHelper
}

public static bool Init (TaskLoggingHelper log, string ndkPath)
public static bool Init (TaskLoggingHelper log, string ndkPath) =>
Init ((c, m) => log.LogCodedError (c, m), ndkPath);

public static bool Init (Action<string, string> logError, string ndkPath)
{
Version ndkVersion;
bool hasNdkVersion = GetNdkToolchainRelease (ndkPath ?? "", out ndkVersion);

if (!hasNdkVersion) {
log?.LogCodedError ("XA5101",
"Could not locate the Android NDK. Please make sure the Android NDK is installed in the Android SDK Manager, " +
"or if using a custom NDK path, please ensure the $(AndroidNdkDirectory) MSBuild property is set to the custom path.");
logError ("XA5101",
"Could not locate the Android NDK. Please make sure the Android NDK is installed in the Android SDK Manager, " +
"or if using a custom NDK path, please ensure the $(AndroidNdkDirectory) MSBuild property is set to the custom path.");
return false;
}

Expand All @@ -46,18 +49,23 @@ public static bool Init (TaskLoggingHelper log, string ndkPath)
}

public static bool ValidateNdkPlatform (TaskLoggingHelper log, string ndkPath, AndroidTargetArch arch, bool enableLLVM)
{
return ValidateNdkPlatform ((m) => log.LogMessage (m), (c, m) => log.LogCodedError (c, m), ndkPath, arch, enableLLVM);
}

public static bool ValidateNdkPlatform (Action<string> logMessage, Action<string, string> logError, string ndkPath, AndroidTargetArch arch, bool enableLLVM)
{
if (!UsingClangNDK)
return NdkUtilOld.ValidateNdkPlatform (log, ndkPath, arch, enableLLVM);
return NdkUtilOld.ValidateNdkPlatform (logMessage, logError, ndkPath, arch, enableLLVM);

// Check that we have a compatible NDK version for the targeted ABIs.
Version ndkVersion;
bool hasNdkVersion = GetNdkToolchainRelease (ndkPath, out ndkVersion);

if (hasNdkVersion && ndkVersion.Major < 19) {
log.LogMessage (MessageImportance.High,
"The detected Android NDK version is incompatible with this version of Xamarin.Android, " +
"please upgrade to NDK r19 or newer.");
logMessage (
"The detected Android NDK version is incompatible with this version of Xamarin.Android, " +
"please upgrade to NDK r19 or newer.");
}

return true;
Expand Down
18 changes: 9 additions & 9 deletions src/Xamarin.Android.Build.Tasks/Tasks/NdkUtilsOld.cs
View file
Expand Up @@ -16,30 +16,30 @@ namespace Xamarin.Android.Tasks
{
public static class NdkUtilOld {

public static bool ValidateNdkPlatform (TaskLoggingHelper log, string ndkPath, AndroidTargetArch arch, bool enableLLVM)
public static bool ValidateNdkPlatform (Action<string> logMessage, Action<string, string> logError, string ndkPath, AndroidTargetArch arch, bool enableLLVM)
{
// Check that we have a compatible NDK version for the targeted ABIs.
NdkVersion ndkVersion;
bool hasNdkVersion = GetNdkToolchainRelease (ndkPath, out ndkVersion);

if (IsNdk64BitArch(arch) && hasNdkVersion && ndkVersion.Version < 10) {
log.LogMessage (MessageImportance.High,
"The detected Android NDK version is incompatible with the targeted 64-bit architecture, " +
"please upgrade to NDK r10 or newer.");
logMessage (
"The detected Android NDK version is incompatible with the targeted 64-bit architecture, " +
"please upgrade to NDK r10 or newer.");
}

// NDK r10d is buggy and cannot link x86_64 ABI shared libraries because they are 32-bits.
// See https://code.google.com/p/android/issues/detail?id=161421
if (enableLLVM && ndkVersion.Version == 10 && ndkVersion.Revision == "d" && arch == AndroidTargetArch.X86_64) {
log.LogCodedError ("XA3004", "Android NDK r10d is buggy and provides an incompatible x86_64 libm.so. " +
"See https://code.google.com/p/android/issues/detail?id=161422.");
logError ("XA3004", "Android NDK r10d is buggy and provides an incompatible x86_64 libm.so. " +
"See https://code.google.com/p/android/issues/detail?id=161422.");
return false;
}

if (enableLLVM && (ndkVersion.Version < 10 || (ndkVersion.Version == 10 && ndkVersion.Revision[0] < 'd'))) {
log.LogCodedError ("XA3005",
"The detected Android NDK version is incompatible with the targeted LLVM configuration, " +
"please upgrade to NDK r10d or newer.");
logError ("XA3005",
"The detected Android NDK version is incompatible with the targeted LLVM configuration, " +
"please upgrade to NDK r10d or newer.");
}

return true;
Expand Down

0 comments on commit 8e24138

Please sign in to comment.