Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rework unloadability fix #68550

Merged
merged 6 commits into from
Apr 27, 2022
Merged

Rework unloadability fix #68550

merged 6 commits into from
Apr 27, 2022

Conversation

janvorli
Copy link
Member

This change reworks the recent fix of dependency between two collectible AssemblyLoadContext's when one context uses the other one for resolving its assemblies. There was a problem with the previous solution that has shown up in the newly added test when running with GCStress 3 enabled.
When we create the reference between the related native LoaderAllocator instances, the managed LoaderAllocator of the AssemblyLoadContext that was used to resolve the assembly needs to be alive so that we can create a handle to it in the context of the parent AssemblyLoadContext and keep it alive for the lifetime of the parent AssemblyLoadContext.
With the previous change, if the AssemblyLoadContext that was used to resolve the assembly was collected before the AssemblyLoadContext.Load override or the related Resolving event returned and the managed representation of the resolved assembly was collected too, we would not be able to create managed System.Reflection.Type instances for the types from the resolved assembly after that. So e.g. calling GetInterfaces on a Type would fail or crash.

This change fixes the problem by moving the creation of reference between the native LoaderAllocators to the RuntimeInvokeHostAssemblyResolver where the managed Assembly reference is still alive and thus the related managed LoaderAllocator must be alive too.

Close #68112

@janvorli
Rework the unloadability fix
c05b0b8 
The recent fix had a problem when GC collected the managed Assembly
loaded via the Load override or the Resolving event before we added the
reference between the related native LoaderAllocators. The managed
LoaderAllocator of the ALC we've loaded the Assembly into was collected
and we couldn't create the managed Type objects for the interfaces from
that assembly in the GetInterfaces call on a type that implemented
those.

This change fixes it by moving the creation of reference between the
native LoaderAllocators to the RuntimeInvokeHostAssemblyResolver where
we still have a live managed reference to the loaded Assembly.
@janvorli
Use LoaderAllocator from binder only
83f0172
@janvorli
Pass binder instead of LoaderAllocator
d833894
@janvorli
Move clearing of the m_pAssemblyLoaderAllocator to a later stage
08c9fb2
@janvorli janvorli added this to the 7.0.0 milestone Apr 26, 2022
@janvorli janvorli self-assigned this Apr 26, 2022
@ghost
Copy link

ghost commented Apr 26, 2022

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

This change reworks the recent fix of dependency between two collectible AssemblyLoadContext's when one context uses the other one for resolving its assemblies. There was a problem with the previous solution that has shown up in the newly added test when running with GCStress 3 enabled.
When we create the reference between the related native LoaderAllocator instances, the managed LoaderAllocator of the AssemblyLoadContext that was used to resolve the assembly needs to be alive so that we can create a handle to it in the context of the parent AssemblyLoadContext and keep it alive for the lifetime of the parent AssemblyLoadContext.
With the previous change, if the AssemblyLoadContext that was used to resolve the assembly was collected before the AssemblyLoadContext.Load override or the related Resolving event returned and the managed representation of the resolved assembly was collected too, we would not be able to create managed System.Reflection.Type instances for the types from the resolved assembly after that. So e.g. calling GetInterfaces on a Type would fail or crash.

This change fixes the problem by moving the creation of reference between the native LoaderAllocators to the RuntimeInvokeHostAssemblyResolver where the managed Assembly reference is still alive and thus the related managed LoaderAllocator must be alive too.

Close #68112

Author: janvorli
Assignees: janvorli
Labels:

area-AssemblyLoader-coreclr
Milestone: 7.0.0

elinor-fung
elinor-fung reviewed Apr 26, 2022
View reviewed changes
src/coreclr/vm/appdomain.cpp Show resolved Hide resolved
src/coreclr/binder/assemblybindercommon.cpp Show resolved Hide resolved
src/coreclr/binder/assemblybindercommon.cpp
@@ -1153,9 +1154,10 @@ namespace BINDER_SPACE

#if !defined(DACCESS_COMPILE)
HRESULT AssemblyBinderCommon::BindUsingHostAssemblyResolver(/* in */ INT_PTR pManagedAssemblyLoadContextToBindWithin,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably do some cleanup around just getting the managed ALC pointer from the AssemblyBinder now that we have the binder - after this change goes in, since we want to backport this.

elinor-fung
elinor-fung approved these changes Apr 27, 2022
View reviewed changes
Copy link
Member

@elinor-fung elinor-fung left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also file a docs issue for how we're now explicitly blocking the non-collectible to collectible assembly: https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md
Or I think marking this with the 'breaking-change' label gives you all the instructions for that.

@janvorli
Update the ResolvedFromDifferentContext test
e5a73df 
Make it test that we throw an exception when the parent ALC is not
collectible.
@janvorli janvorli added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Apr 27, 2022
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Apr 27, 2022
@ghost
Copy link

ghost commented Apr 27, 2022 •
edited by janvorli
Loading

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@janvorli janvorli merged commit ff00ac7 into dotnet:main Apr 27, 2022
@janvorli janvorli mentioned this pull request Apr 27, 2022
Closed
2 tasks
@janvorli janvorli removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Apr 27, 2022
janvorli added a commit to janvorli/runtime that referenced this pull request Apr 28, 2022
@janvorli
[Release/6.0] Port unloadability fixes
e23a69e 
This change ports two unloadability fixes for issues found by a
customer.
* dotnet#68550 that adds reference between native
`LoaderAllocator`s of two collectible `AssemblyLoadContexts` when one of
them is used to resolve assembly using the other one. This reference
ensures that the one that provides the resolved `Assembly` isn't
collected before the one that uses it.
* dotnet#68336 that adds a missing lock around `m_LoaderAllocatorReferences`
iteration during assembly load context destruction.
@janvorli janvorli mentioned this pull request Apr 28, 2022
Merged
carlossanlop pushed a commit that referenced this pull request May 5, 2022
@janvorli
[release/6.0] Unloadability bugs fixes (#68674)
2154551 
* [Release/6.0] Port unloadability fixes

This change ports two unloadability fixes for issues found by a
customer.
* #68550 that adds reference between native
`LoaderAllocator`s of two collectible `AssemblyLoadContexts` when one of
them is used to resolve assembly using the other one. This reference
ensures that the one that provides the resolved `Assembly` isn't
collected before the one that uses it.
* #68336 that adds a missing lock around `m_LoaderAllocatorReferences`
iteration during assembly load context destruction.

* Remove breaking change part

The change in main has added an exception in case a collectible
`Assembly` is resolved into a non-collectible `AssemblyLoadContext`.
Although that is incorrect, there are cases when it would work. So the
added exception is a breaking change that I think we likely don't want
to get into 6.0
@ghost ghost locked as resolved and limited conversation to collaborators May 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@elinor-fung elinor-fung elinor-fung approved these changes

@VSadov VSadov Awaiting requested review from VSadov

Assignees

@janvorli janvorli

Labels
area-AssemblyLoader-coreclr breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Milestone
7.0.0
2 participants
@janvorli @elinor-fung