Fix | Load and resolve retry logic providers into the current AssemblyLoadContext - #4588
Open
edwardneal wants to merge 2 commits into
Open
Fix | Load and resolve retry logic providers into the current AssemblyLoadContext#4588edwardneal wants to merge 2 commits into
edwardneal wants to merge 2 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses two interrelated bugs, both of which are triggered when SqlClient is loaded into an alternate AssemblyLoadContext. Both of these relate to
SqlConfigurableRetryLogicLoader.The first bug is that when using the managed SNI, SqlClient blocks an unloadable
AssemblyLoadContextfrom being unloaded. This is completely due to the event handlers: it adds a handler to the default ALC'sResolvingevent, and this strong reference from the default ALC to a type loaded into a secondary ALC prevents that secondary ALC from being unloaded. This is fixed by making sure that the currently-executing ALC unhooks theResolvinghandler when it unloads.The second, related, bug is that we load a configuration's
RetryLogicTypeinto the default AssemblyLoadContext. This means that the type will never be unloaded with the secondary AssemblyLoadContext - and because it needs to reference a type in that secondary ALC, the secondary ALC won't unload either. Fixing this means that we need to load RetryLogicType into the current AssemblyLoadContext, not the default one.To avoid a regression in fixing the second bug, we must also hook to the current AssemblyLoadContext's
Resolvinghandler. This is called to resolve transitive dependencies of the assembly which has to be loaded to locate a configuration's RetryLogicType. The nuance here is that only the current AssemblyLoadContext'sResolvinghandler is called - not the default handler. If those two differ, we'll find ourselves in a position where a transitive dependency can fail to load (or worse, the wrong dependency may load - different ALCs may have different base paths.)With these bugs resolved, #1687 is fixed when using the managed SNI.
I don't think that SqlClient can implement a fix to make the native SNI compatible with unloadable AssemblyLoadContexts - there's a rooted
SNILoadHandlewhich callsSniInitializeandSniTerminate. Unless these two methods are signalling and waiting a semaphore in native code, they seem to perform process-wide initialization and teardown. I think having an AssemblyLoadContext remain uncollectible is safer behaviour than having multiple ALCs referring to the process-wide SNI; one might have an in-flight async request, while the other might be unloading and performing SNI teardown.Issues
Contributes to #1687.
Testing
Existing automated tests (
SqlConfigurationManagerReliabilityTest) continue to pass. I've also added a new test,UnloadableAssemblyLoadContextTest. This test covers the scenario where users are loading and unloading AssemblyLoadContexts.