Disable noncollectible alc finalization #21189
Conversation
The constructor of AssemblyLoadContext was missing a call to GC.SuppressFinalize for non-collectible AssemblyLoadContext. That means that the finalizer can be executed and unloading being initialized even for non-collectible AssemblyLoadContext. This change fixes that.
The finalizer for the non-collectible AssemblyLoadContext is never called and so it should not be in the finalization queue.
|
Is this fixing a real bug, or is this just "nice to have" ? |
| @@ -94,6 +101,7 @@ internal AssemblyLoadContext(bool fRepresentsTPALoadContext, bool isCollectible) | |||
| ~AssemblyLoadContext() | |||
| { | |||
| // Only valid for a Collectible ALC. Non-collectible ALCs have the finalizer suppressed. | |||
| Debug.Assert(IsCollectible); | |||
There was a problem hiding this comment.
Note that you can get here for completely uninitialized objects, e.g. when the JITing of a constructor for a custom ALC fails: We have allocated the object by the time we are JITing the constructor already, but none of the constructor has run.
There was a problem hiding this comment.
Hmm, I didn't know that. I assume then that this could possibly be the real reason behind #21100. Then it seems that I could use e.g. the unloadLock as a guard (if it is null, the constructor was not executed and thus we should skip the whole finalizer code)
|
@jkotas I have discovered it when I was looking into #21100. I have then realized that it should not be causing that issue, but I have realized it would be nice to not to keep non-collectible ALCs on finalizer queue for no reason. |
|
Sounds good. I think this needs to be fixed up to be robust for uninitialized objects (that may fix a real corner case bug actually). |
Check if the constructor was executed before we run the finalizer code.
| InitiateUnload(); | ||
| // Use the unloadLock as a guard to detect the corner case when the constructor of the AssemblyLoadContext was not executed | ||
| // e.g. due to the JIT failing to JIT it. | ||
| if (unloadLock != null) |
There was a problem hiding this comment.
This is pretty subtle since unloadLock is created by the implicit assignment first, and IsCollectible that is assigned after is asserted below. It probably works ok as long as there is no thread abort, but it would be nice to make it more correct by construction.
| InitiateUnload(); | ||
| // Use the unloadLock as a guard to detect the corner case when the constructor of the AssemblyLoadContext was not executed | ||
| // e.g. due to the JIT failing to JIT it. | ||
| if (unloadLock != null) |
There was a problem hiding this comment.
Nit: While you are on it - could you please change unloadLock to _unloadLock to make it match the coding conventions
|
@jkotas I've made the initialization order of the IsCollectible and _unloadLock explicit and also modified all fields to start with underscore. |
|
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please |
|
HardwareIntrinsics on OSX is known flaky test |
* Disable noncollectible AssemblyLoadContext finalization The constructor of AssemblyLoadContext was missing a call to GC.SuppressFinalize for non-collectible AssemblyLoadContext. That means that the finalizer can be executed and unloading being initialized even for non-collectible AssemblyLoadContext. This change fixes that. * SuppressFinalize non-collectible AssemblyLoadContext The finalizer for the non-collectible AssemblyLoadContext is never called and so it should not be in the finalization queue. * Handle finalizer call without constructor executed Check if the constructor was executed before we run the finalizer code. Commit migrated from dotnet/coreclr@ac732ff
For non collectible AssemblyLoadContext, the finalizer should never be called and thus the AssemblyLoadContext should not be in the finalizer queue.