Use UnmanagedCallersOnly in AssemblyDependencyResolver.
#119034
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
We now have function pointers and can write this with less magic.
There was a problem hiding this comment.
Pull Request Overview
This PR modernizes the bindings to the hostpolicy library by replacing delegate-based callbacks with function pointers and UnmanagedCallersOnly methods. The change improves performance by eliminating delegate allocations and marshalling overhead while maintaining thread safety through thread-local storage for callback state.
Key changes:
- Replaced delegates with function pointers in hostpolicy interop bindings
- Implemented
UnmanagedCallersOnlycallback methods inAssemblyDependencyResolver - Added thread-local state management to safely store callback data without opaque parameters
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/Common/src/Interop/Interop.HostPolicy.cs | Updated interop bindings to use function pointers instead of delegates |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyDependencyResolver.cs | Replaced delegate callbacks with UnmanagedCallersOnly methods and added thread-local state management |
| src/tests/Common/CoreCLRTestLibrary/HostPolicyMock.cs | Updated mock implementation to use function pointers matching the new interop API |
| ThreadLocalState state = new ThreadLocalState(); | ||
| Debug.Assert(t_threadLocalState == null); // Re-entrant calls should not happen |
There was a problem hiding this comment.
The Debug.Assert for re-entrant calls may not be sufficient protection in release builds. Consider throwing an exception or implementing proper re-entrancy handling, as the assertion is compiled out in release mode and concurrent access could corrupt the thread-local state.
| ThreadLocalState state = new ThreadLocalState(); | |
| Debug.Assert(t_threadLocalState == null); // Re-entrant calls should not happen | |
| if (t_threadLocalState != null) | |
| throw new InvalidOperationException("Re-entrant calls to AssemblyDependencyResolver are not allowed."); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Not necessary; the callback will not run arbitrary code, but a specific function provided by the framework.
| ThreadLocalState? state = t_threadLocalState; | ||
| Debug.Assert(state != null); |
There was a problem hiding this comment.
The Debug.Assert may not provide adequate protection in release builds. If t_threadLocalState is null, the callback will fail silently or cause a NullReferenceException on the next line. Consider using ArgumentNullException.ThrowIf or similar exception throwing mechanism.
| ThreadLocalState? state = t_threadLocalState; | |
| Debug.Assert(state != null); | |
| ArgumentNullException.ThrowIfNull(state); |
Copilot uses AI. Check for mistakes.
| ThreadLocalState? state = t_threadLocalState; | ||
| Debug.Assert(state != null); |
There was a problem hiding this comment.
The Debug.Assert may not provide adequate protection in release builds. If t_threadLocalState is null, the callback will fail silently or cause a NullReferenceException on the next line. Consider using ArgumentNullException.ThrowIf or similar exception throwing mechanism.
| ThreadLocalState? state = t_threadLocalState; | |
| Debug.Assert(state != null); | |
| if (state is null) | |
| throw new InvalidOperationException("Thread local state is not set in ErrorWriterCallback."); |
Copilot uses AI. Check for mistakes.
|
Curious what's the main motivation for this change? The description provides the reasoning of how... but not why. |
Our interop best practices say to prefer function pointers over marshalled delegates. This is one of the last few remaining instances of marshalled delegates in dotnet/runtime libraries. We got rid of them from as many places as possible.
|
|
The lack of context pointer is not ideal in the native API (sorry, my bad). If we could find a way to fix that, I would prefer that over the reliance on thread locals (although I agree it will work). But if it becomes too complicated I think it's OK to keep it as is. |
|
Opened #119139; I can do it in a subsequent PR. |
This PR updates the bindings to the
hostpolicylibrary to use function pointers for the callback parameters. This enables us to useUnmanagedCallersOnlyfor the callback functions inAssemblyDependencyResolver.Because the callbacks in the
hostpolicyAPIs do not take an opaquevoid*parameter, we use a thread-local variable to store the callbacks' state. This is safe to do because:corehost_set_error_writeris documented to be per-thread.corehost_resolve_component_dependenciesgets called only once at the end.If you prefer to add new
hostpolicyAPIs, let me know.