[release/6.0] Unloadability bugs fixes

[janvorli]

This change ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

[ghost]

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 ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

Author:	janvorli
Assignees:	janvorli
Labels:	Servicing-consider, area-AssemblyLoader-coreclr
Milestone:	6.0.x

[jeffschwMSFT]

Approved. Please get a code review and we will take for 6.0.x

[jkotas]

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

Just making sure - have you verified that these cases work the same as before? I understand that these cases are not explicitly throwing an exception. I am worried that this change is not introducing a crash or some other type of bad behavior.

[janvorli]

have you verified that these cases work the same as before?

I have not tried to figure out all the cases where it would accidentally work, so it is hard to answer this question. There was actually one particular case I had in mind. An app explicitly holding a reference to the ALC used for resolving assemblies for the other ALC or the managed Assembly for the whole time the resolved assembly stuff is actively used or for the lifetime of the "parent" ALC. That basically "simulates" what the change is doing. It seems like people might be doing that.
I'll verify this case works the same with and without this change.

[janvorli]

@jkotas I've tried the case I've mentioned both with and without my change and also with and without GC stress 3. It behaved the same way in both cases.

[JulieLeeMSFT]

Will sync offline with Jan first and then put servicing label after that.

[carlossanlop]

CI failure is unrelated.

[janvorli]

This change ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

[ghost]

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 ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

Author:	janvorli
Assignees:	janvorli
Labels:	Servicing-consider, area-AssemblyLoader-coreclr
Milestone:	6.0.x

[jeffschwMSFT]

Approved. Please get a code review and we will take for 6.0.x

[jkotas]

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

Just making sure - have you verified that these cases work the same as before? I understand that these cases are not explicitly throwing an exception. I am worried that this change is not introducing a crash or some other type of bad behavior.

[janvorli]

have you verified that these cases work the same as before?

I have not tried to figure out all the cases where it would accidentally work, so it is hard to answer this question. There was actually one particular case I had in mind. An app explicitly holding a reference to the ALC used for resolving assemblies for the other ALC or the managed Assembly for the whole time the resolved assembly stuff is actively used or for the lifetime of the "parent" ALC. That basically "simulates" what the change is doing. It seems like people might be doing that.
I'll verify this case works the same with and without this change.

[janvorli]

@jkotas I've tried the case I've mentioned both with and without my change and also with and without GC stress 3. It behaved the same way in both cases.

[JulieLeeMSFT]

Will sync offline with Jan first and then put servicing label after that.

[carlossanlop]

CI failure is unrelated.

[janvorli]

This change ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

[ghost]

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 ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

Author:	janvorli
Assignees:	janvorli
Labels:	Servicing-consider, area-AssemblyLoader-coreclr
Milestone:	6.0.x

[jeffschwMSFT]

Approved. Please get a code review and we will take for 6.0.x

[jkotas]

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

Just making sure - have you verified that these cases work the same as before? I understand that these cases are not explicitly throwing an exception. I am worried that this change is not introducing a crash or some other type of bad behavior.

[janvorli]

have you verified that these cases work the same as before?

I have not tried to figure out all the cases where it would accidentally work, so it is hard to answer this question. There was actually one particular case I had in mind. An app explicitly holding a reference to the ALC used for resolving assemblies for the other ALC or the managed Assembly for the whole time the resolved assembly stuff is actively used or for the lifetime of the "parent" ALC. That basically "simulates" what the change is doing. It seems like people might be doing that.
I'll verify this case works the same with and without this change.

[janvorli]

@jkotas I've tried the case I've mentioned both with and without my change and also with and without GC stress 3. It behaved the same way in both cases.

[JulieLeeMSFT]

Will sync offline with Jan first and then put servicing label after that.

[carlossanlop]

CI failure is unrelated.

[janvorli]

This change ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

[ghost]

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 ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

Author:	janvorli
Assignees:	janvorli
Labels:	Servicing-consider, area-AssemblyLoader-coreclr
Milestone:	6.0.x

[jeffschwMSFT]

Approved. Please get a code review and we will take for 6.0.x

[jkotas]

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

Just making sure - have you verified that these cases work the same as before? I understand that these cases are not explicitly throwing an exception. I am worried that this change is not introducing a crash or some other type of bad behavior.

[janvorli]

have you verified that these cases work the same as before?

I have not tried to figure out all the cases where it would accidentally work, so it is hard to answer this question. There was actually one particular case I had in mind. An app explicitly holding a reference to the ALC used for resolving assemblies for the other ALC or the managed Assembly for the whole time the resolved assembly stuff is actively used or for the lifetime of the "parent" ALC. That basically "simulates" what the change is doing. It seems like people might be doing that.
I'll verify this case works the same with and without this change.

[janvorli]

@jkotas I've tried the case I've mentioned both with and without my change and also with and without GC stress 3. It behaved the same way in both cases.

[JulieLeeMSFT]

Will sync offline with Jan first and then put servicing label after that.

[carlossanlop]

CI failure is unrelated.

[janvorli]

This change ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

[ghost]

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 ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

Author:	janvorli
Assignees:	janvorli
Labels:	Servicing-consider, area-AssemblyLoader-coreclr
Milestone:	6.0.x

[jeffschwMSFT]

Approved. Please get a code review and we will take for 6.0.x

[jkotas]

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

Just making sure - have you verified that these cases work the same as before? I understand that these cases are not explicitly throwing an exception. I am worried that this change is not introducing a crash or some other type of bad behavior.

[janvorli]

have you verified that these cases work the same as before?

I have not tried to figure out all the cases where it would accidentally work, so it is hard to answer this question. There was actually one particular case I had in mind. An app explicitly holding a reference to the ALC used for resolving assemblies for the other ALC or the managed Assembly for the whole time the resolved assembly stuff is actively used or for the lifetime of the "parent" ALC. That basically "simulates" what the change is doing. It seems like people might be doing that.
I'll verify this case works the same with and without this change.

[janvorli]

@jkotas I've tried the case I've mentioned both with and without my change and also with and without GC stress 3. It behaved the same way in both cases.

[JulieLeeMSFT]

Will sync offline with Jan first and then put servicing label after that.

[carlossanlop]

CI failure is unrelated.

[janvorli]

This change ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

[ghost]

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 ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

Author:	janvorli
Assignees:	janvorli
Labels:	Servicing-consider, area-AssemblyLoader-coreclr
Milestone:	6.0.x

[jeffschwMSFT]

Approved. Please get a code review and we will take for 6.0.x

[jkotas]

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

Just making sure - have you verified that these cases work the same as before? I understand that these cases are not explicitly throwing an exception. I am worried that this change is not introducing a crash or some other type of bad behavior.

[janvorli]

have you verified that these cases work the same as before?

I have not tried to figure out all the cases where it would accidentally work, so it is hard to answer this question. There was actually one particular case I had in mind. An app explicitly holding a reference to the ALC used for resolving assemblies for the other ALC or the managed Assembly for the whole time the resolved assembly stuff is actively used or for the lifetime of the "parent" ALC. That basically "simulates" what the change is doing. It seems like people might be doing that.
I'll verify this case works the same with and without this change.

[janvorli]

@jkotas I've tried the case I've mentioned both with and without my change and also with and without GC stress 3. It behaved the same way in both cases.

[JulieLeeMSFT]

Will sync offline with Jan first and then put servicing label after that.

[carlossanlop]

CI failure is unrelated.

[janvorli]

This change ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

[ghost]

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 ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

Author:	janvorli
Assignees:	janvorli
Labels:	Servicing-consider, area-AssemblyLoader-coreclr
Milestone:	6.0.x

[jeffschwMSFT]

Approved. Please get a code review and we will take for 6.0.x

[jkotas]

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

Just making sure - have you verified that these cases work the same as before? I understand that these cases are not explicitly throwing an exception. I am worried that this change is not introducing a crash or some other type of bad behavior.

[janvorli]

have you verified that these cases work the same as before?

I have not tried to figure out all the cases where it would accidentally work, so it is hard to answer this question. There was actually one particular case I had in mind. An app explicitly holding a reference to the ALC used for resolving assemblies for the other ALC or the managed Assembly for the whole time the resolved assembly stuff is actively used or for the lifetime of the "parent" ALC. That basically "simulates" what the change is doing. It seems like people might be doing that.
I'll verify this case works the same with and without this change.

[janvorli]

@jkotas I've tried the case I've mentioned both with and without my change and also with and without GC stress 3. It behaved the same way in both cases.

[JulieLeeMSFT]

Will sync offline with Jan first and then put servicing label after that.

[carlossanlop]

CI failure is unrelated.

[janvorli]

This change ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

[ghost]

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 ports two unloadability fixes for issues found by a customer.

• Rework unloadability fix #68550 that adds reference between native
  LoaderAllocators 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. This port was done manually as some of
  the related code was cleaned up in the main and data structures are slightly
  different in 6.0.
• Fix locking around m_LoaderAllocatorReferences iteration #68336 that adds a missing lock around m_LoaderAllocatorReferences
  iteration during assembly load context destruction.

There is one part of #68550 that was intentionally not ported. 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.
In case we decide to port that breaking change too, we can just remove the 2nd
commit in this PR.

Customer Impact

Without this change, .NET 6 apps that return collectible Assembly from the AssemblyLoadContext.Load override or the AssemblyLoadContext.Resolving event will likely crash intermittently when operating on the loaded Assembly.

Testing

coreclr and libraries tests locally also with GC stress 3 and with runincontext mode. The customer that has reported this issue is testing a patched coreclr.dll with this fix in their environment.

Risk

Low, it affects only collectible assemblies resolved by one collectible AssemblyLoadContext from another collectible one.

Author:	janvorli
Assignees:	janvorli
Labels:	Servicing-consider, area-AssemblyLoader-coreclr
Milestone:	6.0.x

[jeffschwMSFT]

Approved. Please get a code review and we will take for 6.0.x

[jkotas]

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

Just making sure - have you verified that these cases work the same as before? I understand that these cases are not explicitly throwing an exception. I am worried that this change is not introducing a crash or some other type of bad behavior.

[janvorli]

have you verified that these cases work the same as before?

I have not tried to figure out all the cases where it would accidentally work, so it is hard to answer this question. There was actually one particular case I had in mind. An app explicitly holding a reference to the ALC used for resolving assemblies for the other ALC or the managed Assembly for the whole time the resolved assembly stuff is actively used or for the lifetime of the "parent" ALC. That basically "simulates" what the change is doing. It seems like people might be doing that.
I'll verify this case works the same with and without this change.

[janvorli]

@jkotas I've tried the case I've mentioned both with and without my change and also with and without GC stress 3. It behaved the same way in both cases.

[JulieLeeMSFT]

Will sync offline with Jan first and then put servicing label after that.

[carlossanlop]

CI failure is unrelated.