-
Notifications
You must be signed in to change notification settings - Fork 532
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] close XAAssemblyResolvers
#9531
Conversation
Context: https://discord.com/channels/732297728826277939/732297916680765551/1308554103206580244 Fixes: #9133 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` 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: 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. 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/>` MSBuild task.
Actually, there might be a test failure:
|
Then dispose later if appropriate
I still see the |
NativeCodeGenState.Template = templateCodeGenState; | ||
BuildEngine4.RegisterTaskObjectAssemblyLocal (ProjectSpecificTaskObjectKey (NativeCodeGenStateRegisterTaskKey), nativeCodeGenStates, RegisteredTaskObjectLifetime.Build); | ||
|
||
NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent = templateCodeGenState.JniAddNativeMethodRegistrationAttributePresent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonathanpeppers: My "spider-sense" is that this change is the cause of the test failure.
Before this PR, state.JniAddNativeMethodRegistrationAttributePresent
is not modified until after WriteTypeMappings()
is invoked:
WriteTypeMappings (state); if (!tmg.Generate (Debug, SkipJniAddNativeMethodRegistrationAttributeScan, TypemapOutputDirectory, GenerateNativeAssembly)) { state.JniAddNativeMethodRegistrationAttributePresent = skipJniAddNativeMethodRegistrationAttributeScan;
With this change, you're setting NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent
before WriteTypeMappings()
has had a chance to execute, and thus before it has had a chance to set state.JniAddNativeMethodRegistrationAttributePresent
. Consequently, NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent
will always be false
, when it needs to be true
for Java.Interop-Tests
to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps all you need to do is "move this line" to be after line 262?
/* 262 */ WriteTypeMappings (state);
/* 263 */ NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent = NativeCodeGenState.TemplateJniAddNativeMethodRegistrationAttributePresent || state.JniAddNativeMethodRegistrationAttributePresent;
/* 264 */ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving it down, fixes it. It worked before, because NativeCodeGenState.Template
had a reference to the object. 👍
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
Context: https://discord.com/channels/732297728826277939/732297916680765551/1308554103206580244
Fixes: #9133
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
file (while the file was locked), where I could observe the objects in memory:Then realized the problem was:
We opened some
.dll
files withMono.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:NativeCodeGenState
also holds aXAAssemblyResolver
in aResolver
property. This means thisXAAssemblyResolver
instance would also be kept alive.It appears we only use the static
Template
property for abool
flag, so I changed the property to abool
instead.After this change, we can safely dispose
Resolver
instances. I looped over theNativeCodeGenState
instances disposing of eachResolver
at the end of the<GenerateJavaStubs/>
MSBuild task.