StartupHook: Introduce AssemblyLoadContext to load agent assemblies #1169
Conversation
This file contains 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
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪Test errors
Expand to view the tests failures
|
|
@gregkalapos did you want to rebase this against master, to pick up the startup hook tests merged in #1167? |
russcam
reviewed
Feb 15, 2021
| "Elastic.Apm.GrpcClient", "Elastic.Apm.Elasticsearch" | ||
| }; | ||
|
|
||
| private readonly string _asssemblyDir; |
There was a problem hiding this comment.
Suggested change
| private readonly string _asssemblyDir; | |
| private readonly string _assemblyDir; |
| } | ||
| catch (Exception e) | ||
| { | ||
| _logger.WriteLine($"{nameof(AgentLoadContext)} Failed loading {assemblyName.Name}, exception: {e}"); |
There was a problem hiding this comment.
Suggested change
| _logger.WriteLine($"{nameof(AgentLoadContext)} Failed loading {assemblyName.Name}, exception: {e}"); | |
| _logger.WriteLine($"{nameof(AgentLoadContext)} Failed loading {assemblyName}, exception: {e}"); |
AssemblyName overrides ToString() to return the FullName, so we could use assemblyName here
| @@ -1,7 +1,7 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
|
|
|||
| <PropertyGroup> | |||
| <TargetFramework>netcoreapp2.2</TargetFramework> | |||
| <TargetFramework>netcoreapp3.1</TargetFramework> | |||
There was a problem hiding this comment.
does this need to be bumped to netcoreapp3.1, or should it stay at netcoreapp2.2? In other places, we've been dropping netcoreapp2.2 since it is EOL'ed, so I'm not against doing so here too, though there would be some housekeeping to do so, such as, removing mentions of netcoreapp2.2 / ASP,NET Core 2.2 in the startup hooks docs.
There was a problem hiding this comment.
Originally I used an API which was only present in 3.1, but it turned out to be not needed. I can revert this. So, given there is no change needed here, I'd say let's keep it as it is. The next push will change it back to 2.2..
| AssemblyLoadContext.Default.Resolving += (context, assemblyName) => | ||
| { | ||
| if (AgentLoadContext.AgentDependencyLibsToLoad.Contains(assemblyName.Name) | ||
| || AgentLoadContext.AgentLibsToLoad.Contains(assemblyName.Name)) |
There was a problem hiding this comment.
Should we make both AgentDependencyLibsToLoad and AgentLibsToLoad to be HashSet<string> for this op?
| { | ||
| if (AppDomain.CurrentDomain.GetAssemblies().Any(n => n.FullName.Equals(assemblyName, StringComparison.Ordinal))) | ||
| { | ||
| Logger.WriteLine($"{assemblyName} is alrady loaded - we don't try to load it"); |
There was a problem hiding this comment.
Suggested change
| Logger.WriteLine($"{assemblyName} is alrady loaded - we don't try to load it"); | |
| Logger.WriteLine($"{assemblyName} is already loaded"); | |
Load Agent assemblies into a separate AssemblyLoadContext
|
This is still work in progress. I'm still working on some cases where the fix does not help. |
|
I gave up on this - loading the agent into a separate AssemblyLoadContext has issues with diagnostic source, since it depends on a static instance. I opened #1227 to solve the Newtonsoft.Json issue by using |
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.
Prior to this PR with the startuphook feature we loaded everything into
AssemblyLoadContext.Default. This PR introduces a separate AssemblyLoadContext to load agent assemblies and dependencies of agent assemblies.Why we need this?
Newtonsoft.JsonVersion="11.0.2"Newtonsoft.JsonwithAssemblyLoadContext.Default, it throws an exception and the agent won't work.The problem this PR solves can be reproduced by creating an empty project, adding
<PackageReference Include="Newtonsoft.Json" Version="9.0.2" />(or any version) to itscsprojfile and use the startuphook feature. Same happens when there is a dependency in the app which depends onNewtonsoft.Json.