FileLoadException: Add Profiler fix #671

zacharycmontoya · 2020-03-04T21:23:04Z

Problem

Issue 1

Our CLR Profiler currently assumes that as long as the Datadog.Trace.ClrProfiler.Managed assembly has been loaded domain-neutral once, all AppDomains can use that assembly. Under this false assumption, we always instrument domain-neutral assemblies which adds an AssemblyReference to our managed profiler and affects how the CLR shares code between AppDomains.

However, some applications may have BindingRedirect's specified on dependencies of the managed profiler like System.Net.Http which can force Datadog.Trace.ClrProfiler.Managed to no longer be loaded domain-neutral. When we instrument a domain-neutral assembly with a non-domain-neutral Datadog.Trace.ClrProfiler.Managed in one AppDomain and then another AppDomain tries to use the instrumented domain-neutral assembly, the second AppDomain crashes with the FileLoadException.

Issue 2

Our new mechanism for automatically instrumenting AspNet uses the CLR Profiler to intercept a method call inside System.Web to call into Datadog.Trace.ClrProfiler.Managed. System.Web is always loaded domain-neutral but, as noted above, there are circumstances that can cause our assembly to not be loaded domain-neutral. We need to change the way we invoke the AspNet integration so that the assembly that runs the integration code is always domain-neutral.

Changes proposed in this pull request:

Addressing Issue 1

Modify the profiler to never inject method calls to Datadog.Trace.ClrProfiler.Managed if the calling assembly was loaded domain-neutral.

Addressing Issue 2

Move basic types like InterceptMethodAttribute and friends to a new Datadog.Trace.ClrProfiler.Managed.Core that has no external dependencies.
Move the AspNet integration into a new assembly called Datadog.Trace.AspNet.Loader whose only dependencies are Datadog.Trace.ClrProfiler.Managed.Core and System.Web. This assembly closure guarantees that the assembly will always be loaded domain-neutral. This is now a safe entry point for System.Web to call into during application setup. Also, the method responsible for adding the AspNet integration does not wrap any methods (that would require Sigil).
To enable the minimal method call above that is simply called and returns to the caller without wrapping any target method, modify the method replacement infrastructure to support an action. The default value is ReplaceTargetMethod which performs the usual method replacement as before, but a new action InsertFirst will add a call to the method as the first instruction in the matching caller method. When generating integrations.json, add checks on the new InsertFirst integration methods.

Consequences

Fix Loading this assembly would produce a different grant set from other instances. (Exception from HRESULT: 0x80131401)
For ASP.NET applications (or other scenarios where LoaderOptimizationAttribute=MultiDomainHost), there may be missing spans from calls between framework assemblies. Affected integrations include:
- HttpMessageHandler
- WebRequest
- Wcf (server)

@DataDog/apm-dotnet

src/Datadog.Trace.ClrProfiler.Managed/Integrations/WebRequestIntegration.cs

src/Datadog.Trace.ClrProfiler.Native/cor_profiler.cpp

…us know it is no longer safe to instrument domain-neutral assemblies. We still crash because at this point we've already added a dependency to System.Web.

…odsCore instead of System.Web.Compilation.BuildManager.InvokePreStartInitMethods

…at holds base classes needed for automatic instrumentation that does not rely on the Tracer.

…ple input assemblies with InterceptMethodAttributes

…er.Managed to Datadog.Trace.ClrProfiler.Managed.Utilities

…getMethod, and modify GenerateIntegrationDefinitions to remove the previous fallback implementation. The result is the exact same integrations.json file, and the explicit attribute may be necessary for doing the AspNet injection I'd like to do.

…ke when it sees a wrapper method, giving flexibility to either replace the target method or insert the method call at the beginning or end of the calling method. All existing integrations have the action "ReplaceTargetMethod" but we may have variations in the future.

…rProfiler.Managed.Utilities. This name is likely temporary as the naming convention doesn't make much sense. The most important quality of this assembly is it has no framework dependencies outside System.Web. Modify the profiler to insert method calls at the beginning of the instrumented method if they have the InsertFirst action.

Move startup logic into a new assembly Datadog.Trace.AspNet.Loader Update integrations and wix files with the new assembly.

…lse whenever we see it being loaded not domain-neutral. Only emit the warning when it was previously loaded domain-neutral.

…rently stop a lot of IIS applications from getting System.Net.Http and System.Data spans.

Refactor profiler code.

… re-use code.

src/Datadog.Trace.AspNet.Loader/Startup.cs

bobuva

ok, thanks.

src/Datadog.Trace.ClrProfiler.Managed.Core/MethodReplacementActionType.cs

…unt for the new projects.

src/Datadog.Trace.ClrProfiler.Managed.Core/Datadog.Trace.ClrProfiler.Managed.Core.csproj

tools/PrepareRelease/SetAllVersions.cs

src/Datadog.Trace.ClrProfiler.Managed.Core/MethodReplacementActionType.cs

bobuva

Looks good.

…atadog.Trace.ClrProfiler.Managed.Core to AssemblyVersion 1.0.0.0

Fix FileLoadException: Loading this assembly would produce a different grant set from other instances. (Exception from HRESULT: 0x80131401) ## Problem ### Issue 1 Our CLR Profiler currently assumes that as long as the `Datadog.Trace.ClrProfiler.Managed` assembly has been loaded domain-neutral once, all AppDomains can use that assembly. Under this false assumption, we always instrument domain-neutral assemblies which adds an AssemblyReference to our managed profiler and affects how the CLR shares code between AppDomains. However, some applications may have BindingRedirect's specified on dependencies of the managed profiler like `System.Net.Http` which can force `Datadog.Trace.ClrProfiler.Managed` to no longer be loaded domain-neutral. When we instrument a domain-neutral assembly with a non-domain-neutral `Datadog.Trace.ClrProfiler.Managed` in one AppDomain and then another AppDomain tries to use the instrumented domain-neutral assembly, the second AppDomain crashes with the FileLoadException. ### Issue 2 Our new mechanism for automatically instrumenting AspNet uses the CLR Profiler to intercept a method call inside `System.Web` to call into `Datadog.Trace.ClrProfiler.Managed`. `System.Web` is always loaded domain-neutral but, as noted above, there are circumstances that can cause our assembly to not be loaded domain-neutral. We need to change the way we invoke the AspNet integration so that the assembly that runs the integration code is always domain-neutral. ## Changes proposed in this pull request: ### Addressing Issue 1 - Modify the profiler to never inject method calls to `Datadog.Trace.ClrProfiler.Managed` if the calling assembly was loaded domain-neutral. ### Addressing Issue 2 - Move basic types like `InterceptMethodAttribute` and friends to a new `Datadog.Trace.ClrProfiler.Managed.Core` that has no external dependencies. - Move the AspNet integration into a new assembly called `Datadog.Trace.AspNet.Loader` whose only dependencies are `Datadog.Trace.ClrProfiler.Managed.Core` and `System.Web`. This assembly closure guarantees that the assembly will always be loaded domain-neutral. This is now a safe entry point for `System.Web` to call into during application setup. Also, the method responsible for adding the AspNet integration does not wrap any methods (that would require `Sigil`). - To enable the minimal method call above that is simply called and returns to the caller without wrapping any target method, modify the method replacement infrastructure to support an `action`. The default value is `ReplaceTargetMethod` which performs the usual method replacement as before, but a new action `InsertFirst` will add a call to the method as the first instruction in the matching caller method. When generating `integrations.json`, add checks on the new `InsertFirst` integration methods. ## Consequences - Fix `Loading this assembly would produce a different grant set from other instances. (Exception from HRESULT: 0x80131401)` - For ASP.NET applications (or other scenarios where `LoaderOptimizationAttribute=MultiDomainHost`), there may be missing spans from calls between framework assemblies. Affected integrations include: - HttpMessageHandler - WebRequest - Wcf (server)

…ain-neutral assemblies (#690) Add new CLR Profiler environment variable `DD_TRACE_DOMAIN_NEUTRAL_INSTRUMENTATION` to override the default instrumentation behavior introduced in #671. The default behavior refuses to instrument a method call inside a domain-neutral assembly, which completely avoids the scenario that can cause a sharing violation (`HRESULT 0x80131401`) seen in #592. However, as noted in the original PR, this can cause some callsites for the HttpMessageHandler, WebClient, and WCF integrations to not be instrumented when the application is hosted in IIS. As a temporary measure to receive the same spans before upgrading to .NET Tracer v1.14.2, this new environment variable can safely be set under one condition: **all IIS Application Pools on the machine have at most one application.** Additional changes in this PR include: - Test the `DD_TRACE_DOMAIN_NEUTRAL_INSTRUMENTATION` flag by: - Modifying Samples.HttpMessageHandler to use `LoaderOptimization.MultiDomainHost` when run on .NET Framework. This simulates IIS where Framework assemblies in the GAC are loaded in domain-neutral. - Injecting the environment variable into the Samples.HttpMessageHandler integration test case - Adding `Datadog.Trace.ClrProfiler.Managed.dll` and its dependencies to the GAC before executing the integration tests - Small fix in `MockTracerAgent` to always get the next port from the `TcpPortProvider` instead of incrementing the port number by 1

zacharycmontoya added area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) area:integrations:aspnet labels Mar 4, 2020

zacharycmontoya self-assigned this Mar 4, 2020

colin-higgins reviewed Mar 6, 2020

View reviewed changes

src/Datadog.Trace.ClrProfiler.Managed/Integrations/WebRequestIntegration.cs Outdated Show resolved Hide resolved

colin-higgins reviewed Mar 6, 2020

View reviewed changes

src/Datadog.Trace.ClrProfiler.Native/cor_profiler.cpp Outdated Show resolved Hide resolved

colin-higgins reviewed Mar 6, 2020

View reviewed changes

src/Datadog.Trace.ClrProfiler.Native/cor_profiler.cpp Outdated Show resolved Hide resolved

zacharycmontoya force-pushed the zach/profiler/fileloadexception/profiler-fix branch 2 times, most recently from 25a7d5a to 1e05292 Compare March 7, 2020 03:59

zacharycmontoya marked this pull request as ready for review March 9, 2020 15:31

zacharycmontoya requested a review from a team as a code owner March 9, 2020 15:31

zacharycmontoya added 20 commits March 9, 2020 08:59

Add flag managed_profiler_unsafe_to_instrument_domain_neutral to let …

4bd29d9

…us know it is no longer safe to instrument domain-neutral assemblies. We still crash because at this point we've already added a dependency to System.Web.

Instrument System.Web.Compilation.BuildManager.InvokePreStartInitMeth…

5eaf18f

…odsCore instead of System.Web.Compilation.BuildManager.InvokePreStartInitMethods

Create a new Datadog.Trace.ClrProfiler.Managed.Utilities project th…

443b16f

…at holds base classes needed for automatic instrumentation that does not rely on the Tracer.

Update the GenerateIntegrationDefinitions to be able to take in multi…

0f73238

…ple input assemblies with InterceptMethodAttributes

Move ClrNames and TypeExtensions helpers from Datadog.Trace.ClrProfil…

06bde92

…er.Managed to Datadog.Trace.ClrProfiler.Managed.Utilities

Rename base profiler assembly to Datadog.Trace.ClrProfiler.Managed.Core.

eccb0a3

Move startup logic into a new assembly Datadog.Trace.AspNet.Loader Update integrations and wix files with the new assembly.

Revert TargetMethod requirement in GenerateIntegrationDefinitions.

6c8df33

Reduce the diff.

45ec908

Remove references to InsertLast action.

0325e3a

Add another check to ensure InsertFirst methods have a void return type.

28a0f4c

Always set managed_profiler_unsafe_to_instrument_domain_neutral to fa…

bd2bf34

…lse whenever we see it being loaded not domain-neutral. Only emit the warning when it was previously loaded domain-neutral.

Full stop, do not instrument domain-neutral assemblies. This will cur…

7d2169b

…rently stop a lot of IIS applications from getting System.Net.Http and System.Data spans.

Revert InterceptMethodAttribute changes in WebRequestIntegration.cs

2f73ab5

For the new projects, set IsPackable=false

37c8830

Improve integration checks on InsertFirst methods.

4813e83

Refactor profiler code.

Add GetWrapperMethodRef so both types of method replacement calls can…

9562479

… re-use code.

Re-order helper methods for hopefully a nicer looking diff

0e2074e

Fix unit-tests build break

bf885dd

zacharycmontoya force-pushed the zach/profiler/fileloadexception/profiler-fix branch from c83fd1e to bf885dd Compare March 9, 2020 16:00

zacharycmontoya added 2 commits March 9, 2020 09:01

Fix rebase for integrations.json

834533a

Fix native unit test build.

53c7d30

bobuva reviewed Mar 9, 2020

View reviewed changes

src/Datadog.Trace.AspNet.Loader/Startup.cs Show resolved Hide resolved

bobuva reviewed Mar 9, 2020

View reviewed changes

src/Datadog.Trace.ClrProfiler.Managed.Core/MethodReplacementActionType.cs Show resolved Hide resolved

src/Datadog.Trace.ClrProfiler.Managed.Core/MethodReplacementActionType.cs Show resolved Hide resolved

Refactoring, documentation update, and PrepareRelease updates to acco…

4ace566

…unt for the new projects.

lucaspimentel reviewed Mar 11, 2020

View reviewed changes

src/Datadog.Trace.ClrProfiler.Managed.Core/Datadog.Trace.ClrProfiler.Managed.Core.csproj Show resolved Hide resolved

lucaspimentel reviewed Mar 11, 2020

View reviewed changes

tools/PrepareRelease/SetAllVersions.cs Show resolved Hide resolved

lucaspimentel approved these changes Mar 11, 2020

View reviewed changes

lucaspimentel reviewed Mar 11, 2020

View reviewed changes

src/Datadog.Trace.ClrProfiler.Managed.Core/MethodReplacementActionType.cs Show resolved Hide resolved

bobuva approved these changes Mar 11, 2020

View reviewed changes

Refactor SetAllVersions.cs and lock Datadog.Trace.AspNet.Loader and D…

d3c5fd8

…atadog.Trace.ClrProfiler.Managed.Core to AssemblyVersion 1.0.0.0

zacharycmontoya merged commit 41458d0 into master Mar 11, 2020

zacharycmontoya deleted the zach/profiler/fileloadexception/profiler-fix branch March 11, 2020 23:06

lucaspimentel added this to the 1.14.2 milestone Mar 13, 2020

lucaspimentel linked an issue Mar 13, 2020 that may be closed by this pull request

Loading this assembly would produce a different grant set from other instances. (Exception from HRESULT: 0x80131401) #592

Closed

xfxf mentioned this pull request Mar 31, 2020

Missing spans between 1.12.0 and 1.15.0 #688

Closed

zacharycmontoya mentioned this pull request Apr 2, 2020

Add CLR Profiler variable to override instrumentation behavior on domain-neutral assemblies #690

Merged

lucaspimentel added area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) and removed area:integrations/aspnet labels Jul 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FileLoadException: Add Profiler fix #671

FileLoadException: Add Profiler fix #671

zacharycmontoya commented Mar 4, 2020 •

edited

Loading

bobuva left a comment

bobuva left a comment

FileLoadException: Add Profiler fix #671

FileLoadException: Add Profiler fix #671

Conversation

zacharycmontoya commented Mar 4, 2020 • edited Loading

Problem

Issue 1

Issue 2

Changes proposed in this pull request:

Addressing Issue 1

Addressing Issue 2

Consequences

bobuva left a comment

Choose a reason for hiding this comment

bobuva left a comment

Choose a reason for hiding this comment

zacharycmontoya commented Mar 4, 2020 •

edited

Loading