Skip to content

Commit

Permalink
[Xamarin.Android.Build.Tasks] close XAAssemblyResolvers (#9531)
Browse files Browse the repository at this point in the history
Fixes: #9133

Context: https://discord.com/channels/732297728826277939/732297916680765551/1308554103206580244
Context: 86260ed

Various customers have been reporting `UnauthorizedAccessExceptions`
in incremental builds, which seems to be a new problem in .NET 9.
We were not able to reproduce the issue locally, but with the number
of reports it seems to be a real issue.

One customer shared a [`MSBuild.dmp`][0] file (while the file was
locked), where I could observe the objects in memory:

	MemoryMappedViewStream	132
	    Mono.Cecil.PE.Image	100
	        Mono.Cecil.ModuleDefinition	100
	            Mono.Cecil.TypeDefinition	100
	                Mono.Cecil.TypeDefinition[]	100
	                    List<Mono.Cecil.TypeDefinition>	1
	                        Xamarin.Android.Tasks.NativeCodeGenState [Static variable Xamarin.Android.Tasks.NativeCodeGenState.<Template>k__BackingField]	1

Then realized the problem was:

  * We opened some `.dll` files with `Mono.Cecil`.

  * They were never closed.

  * Future incremental build attempts would fail on various
    operations of the same `.dll` files.

We were also storing some static state (`NativeCodeGenState`) to be
shared across multiple MSBuild tasks:

	partial class NativeCodeGenState {
	    public static NativeCodeGenState? Template { get; set; }
	}

`NativeCodeGenState` also holds a `XAAssemblyResolver` in a `Resolver`
property.  This means this `XAAssemblyResolver` instance would *also*
be kept alive.

It appears we only use the static `Template` property for a `bool`
flag, so I changed the property to a `bool` instead:

	partial class NativeCodeGenState {
	    public static bool TemplateJniAddNativeMethodRegistrationAttributePresent { get; set; }
	}

After this change, we can safely dispose `Resolver` instances.
I looped over the `NativeCodeGenState` instances disposing of each
`Resolver` at the end of the `<GenerateJavaStubs/>` and
`<GeneratePackageManagerJava/>` MSBuild tasks.

[0]: https://drive.google.com/file/d/12dOkO6ZdbK67sNeY_PVS4d0mgnLyOwUA/view?pli=1
  • Loading branch information
jonathanpeppers authored Nov 21, 2024
1 parent 22f23dd commit 78f8863
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 5 deletions.
18 changes: 15 additions & 3 deletions src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -222,9 +222,6 @@ void Run (bool useMarshalMethods)
}
JCWGenerator.EnsureAllArchitecturesAreIdentical (Log, nativeCodeGenStates);

NativeCodeGenState.Template = templateCodeGenState;
BuildEngine4.RegisterTaskObjectAssemblyLocal (ProjectSpecificTaskObjectKey (NativeCodeGenStateRegisterTaskKey), nativeCodeGenStates, RegisteredTaskObjectLifetime.Build);

if (useMarshalMethods) {
// We need to parse the environment files supplied by the user to see if they want to use broken exception transitions. This information is needed
// in order to properly generate wrapper methods in the marshal methods assembly rewriter.
Expand Down Expand Up @@ -262,6 +259,9 @@ void Run (bool useMarshalMethods)
WriteTypeMappings (state);
}

// Set for use by <GeneratePackageManagerJava/> task later
NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent = templateCodeGenState.JniAddNativeMethodRegistrationAttributePresent;

var acwMapGen = new ACWMapGenerator (Log);
if (!acwMapGen.Generate (templateCodeGenState, AcwMapFile)) {
Log.LogDebugMessage ("ACW map generation failed");
Expand All @@ -270,6 +270,18 @@ void Run (bool useMarshalMethods)
IList<string> additionalProviders = MergeManifest (templateCodeGenState, MaybeGetArchAssemblies (userAssembliesPerArch, templateCodeGenState.TargetArch));
GenerateAdditionalProviderSources (templateCodeGenState, additionalProviders);

if (useMarshalMethods) {
// Save NativeCodeGenState for <GeneratePackageManagerJava/> task later
Log.LogDebugMessage ($"Saving {nameof (NativeCodeGenState)} to {nameof (NativeCodeGenStateRegisterTaskKey)}");
BuildEngine4.RegisterTaskObjectAssemblyLocal (ProjectSpecificTaskObjectKey (NativeCodeGenStateRegisterTaskKey), nativeCodeGenStates, RegisteredTaskObjectLifetime.Build);
} else {
// Otherwise, dispose all XAAssemblyResolvers
Log.LogDebugMessage ($"Disposing all {nameof (NativeCodeGenState)}.{nameof (NativeCodeGenState.Resolver)}");
foreach (var state in nativeCodeGenStates.Values) {
state.Resolver.Dispose ();
}
}

Dictionary<string, ITaskItem> MaybeGetArchAssemblies (Dictionary<AndroidTargetArch, Dictionary<string, ITaskItem>> dict, AndroidTargetArch arch)
{
if (!dict.TryGetValue (arch, out Dictionary<string, ITaskItem> archDict)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ void AddEnvironment ()
BrokenExceptionTransitions = environmentParser.BrokenExceptionTransitions,
PackageNamingPolicy = pnp,
BoundExceptionType = boundExceptionType,
JniAddNativeMethodRegistrationAttributePresent = NativeCodeGenState.Template != null ? NativeCodeGenState.Template.JniAddNativeMethodRegistrationAttributePresent : false,
JniAddNativeMethodRegistrationAttributePresent = NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent,
HaveRuntimeConfigBlob = haveRuntimeConfigBlob,
NumberOfAssembliesInApk = assemblyCount,
BundledAssemblyNameWidth = assemblyNameWidth,
Expand Down Expand Up @@ -395,6 +395,14 @@ void AddEnvironment ()
}
}

if (nativeCodeGenStates is not null) {
// Dispose all XAAssemblyResolvers
Log.LogDebugMessage ($"Disposing all {nameof (NativeCodeGenState)}.{nameof (NativeCodeGenState.Resolver)}");
foreach (var state in nativeCodeGenStates.Values) {
state.Resolver.Dispose ();
}
}

NativeCodeGenState EnsureCodeGenState (AndroidTargetArch targetArch)
{
if (nativeCodeGenStates == null || !nativeCodeGenStates.TryGetValue (targetArch, out NativeCodeGenState? state)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace Xamarin.Android.Tasks;
/// </summary>
class NativeCodeGenState
{
public static NativeCodeGenState? Template { get; set; }
public static bool TemplateJniAddNativeMethodRegistrationAttributePresent { get; set; }

/// <summary>
/// Target architecture for which this instance was created.
Expand Down

0 comments on commit 78f8863

Please sign in to comment.