[main] Fix LoaderAllocator computation for a generic type instance #111706
Conversation
ghost
added
the
dotnet-policy-service
bot
added
the
community-contribution
|
I would be also happy to add a test for this case - which suite should I use for it? StaticsUnloaded.cs? |
| pLoaderModule = pDefinitionModule; | ||
| } | ||
| // Use the definition module as the loader module by default | ||
| Module *pLoaderModule = pDefinitionModule; |
There was a problem hiding this comment.
if pDefinitionModule != null we effectively fall through to it if it is collectible or not at the end, so I've decided to remove the if block
…r allocator of a generic type instance (cherry picked from commit b3a73f3)
alexey-zakharov
force-pushed
the
dotnet-upstream/fix-computeloaderallocator
branch
from
5d15fef to
8aa00c8
Compare
There was a problem hiding this comment.
As you note, a test would be helpful. However, since the behavior of this sort of computation is not actually specified and may change in the future without warning, I'd rather not merge it in with existing tests, and would prefer this to be a separate test (and for the test to have some comments in it indicating that failure is reasonable when running on different runtimes). I'm pretty flexible on naming though, as I don't have a strong opinion. I'm notoriously terrible at naming stuff.
I've looked at the best location for the test and found that there is
|
| Assert.Contains("System.Runtime.Loader.DefaultAssemblyLoadContext", alc.ToString()); | ||
| Assert.Contains(alc, AssemblyLoadContext.All); | ||
| Assert.Contains(Assembly.GetCallingAssembly(), alc.Assemblies); | ||
| Assert.Contains(typeof(int).Assembly, alc.Assemblies); |
There was a problem hiding this comment.
I've updated the test because it was failing as the calling assembly for the test method is xunit assembly which is not in the Default ALC and I suppose is test runner implementation specific
CorLib assembly which is where type of int is is more stable check parameter imho
|
@davidwrighton I don't see any suspicious test failures - do you think the changes are good to go? |
2 participants
Purpose of this PR
Improve logic of computing LoaderAllocator for a generic type instance to avoid AssemblyLoadContext leaks.
Resolves #111611
Problem:
ClassLoader::ComputeLoaderModuleWorkeruses a LoaderAllocator creation number to determine the latest LoaderAllocator to place a class in. However the method does not account the number of the type definition itself when calculating the latest number and this can lead into a situation when the generic instance is put into the older LoaderAllocator.E.g. if we have type
ClassAfrom AssemblyLoadContextAwith number 2 and typeClassBfrom AssemblyLoadContextBwith number 5 the resulting loader allocator for the type ClassB would correspond to AssemblyLoadContextA. IfClassBcontains a static, such static would be then placed under the LoaderAllocatorA, preventing AssemblyLoadContextBfrom unloading.Proposed solution:
The PR updates the
ComputeLoaderModuleWorkerlogic to account for the LoaderAllocator of a defining type, so we use it if it has the latest creation number.(cherry picked from commit b3a73f3)