Skip to content

Fix duplicating integrations due to multiple Initialize calls from different AppDomains. #1794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tonyredondo merged 4 commits into from
Sep 21, 2021
Merged

Fix duplicating integrations due to multiple Initialize calls from different AppDomains. #1794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
00c43e3
Fix duplicating integrations due to multiple AppDomains.
tonyredondo Sep 20, 2021
394f842
Add comments.
tonyredondo Sep 20, 2021
3925992
fix locks.
tonyredondo Sep 21, 2021
f8c9236
Merge branch 'master' into tony/calltarget-definitions-appdomain-fix
tonyredondo Sep 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions tracer/src/Datadog.Trace.ClrProfiler.Native/cor_profiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1091,11 +1091,22 @@ HRESULT STDMETHODCALLTYPE CorProfiler::JITInlining(FunctionID callerId, Function
//
// InitializeProfiler method
//
void CorProfiler::InitializeProfiler(CallTargetDefinition* items, int size)
void CorProfiler::InitializeProfiler(WCHAR* id, CallTargetDefinition* items, int size)
{
auto _ = trace::Stats::Instance()->InitializeProfilerMeasure();

Logger::Info("InitializeProfiler: received from managed side: ", size, " integrations.");
WSTRING definitionsId = WSTRING(id);
Logger::Info("InitializeProfiler: received id: ", definitionsId, " from managed side with ", size,
" integrations.");

std::scoped_lock<std::mutex> definitionsLock(definitions_ids_lock_);

if (definitions_ids_.find(definitionsId) != definitions_ids_.end())
Comment thread
tonyredondo marked this conversation as resolved.
{
Logger::Info("InitializeProfiler: Id already processed.");
return;
}

if (items != nullptr && rejit_handler != nullptr)
{
std::vector<IntegrationMethod> integrationMethods;
Expand Down Expand Up @@ -1167,7 +1178,9 @@ void CorProfiler::InitializeProfiler(CallTargetDefinition* items, int size)
integrationMethods.push_back(integration);
}

std::lock_guard<std::mutex> guard(module_id_to_info_map_lock_);
std::scoped_lock<std::mutex> moduleLock(module_id_to_info_map_lock_);

definitions_ids_.emplace(definitionsId);

for (const auto& moduleItem : module_id_to_info_map_)
{
Expand Down
5 changes: 4 additions & 1 deletion tracer/src/Datadog.Trace.ClrProfiler.Native/cor_profiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ class CorProfiler : public CorProfilerBase
RuntimeInformation runtime_information_;
std::vector<IntegrationMethod> integration_methods_;

std::unordered_set<WSTRING> definitions_ids_;
std::mutex definitions_ids_lock_;

// Startup helper variables
bool first_jit_compilation_completed = false;

Expand Down Expand Up @@ -146,7 +149,7 @@ class CorProfiler : public CorProfilerBase
//
// Add Integrations methods
//
void InitializeProfiler(CallTargetDefinition* items, int size);
void InitializeProfiler(WCHAR* id, CallTargetDefinition* items, int size);
};

// Note: Generally you should not have a single, global callback implementation,
Expand Down
6 changes: 3 additions & 3 deletions tracer/src/Datadog.Trace.ClrProfiler.Native/interop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ EXTERN_C VOID STDAPICALLTYPE GetAssemblyAndSymbolsBytes(BYTE** pAssemblyArray, i
return trace::profiler->GetAssemblyAndSymbolsBytes(pAssemblyArray, assemblySize, pSymbolsArray, symbolsSize);
}

EXTERN_C VOID STDAPICALLTYPE InitializeProfiler(trace::CallTargetDefinition* items, int size)
EXTERN_C VOID STDAPICALLTYPE InitializeProfiler(WCHAR* id, trace::CallTargetDefinition* items, int size)
{
return trace::profiler->InitializeProfiler(items, size);
return trace::profiler->InitializeProfiler(id, items, size);
}

#ifndef _WIN32
Expand All @@ -43,7 +43,7 @@ EXTERN_C char *dddlerror (void)
{
trace::Logger::Error("dlerror: ", errorPtr);
}

return errorPtr;
}

Expand Down
6 changes: 3 additions & 3 deletions tracer/src/Datadog.Trace/ClrProfiler/Instrumentation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@ public static void Initialize()
try
{
Log.Debug("Sending CallTarget integration definitions to native library.");
var definitions = InstrumentationDefinitions.GetAllDefinitions();
NativeMethods.InitializeProfiler(definitions);
foreach (var def in definitions)
var payload = InstrumentationDefinitions.GetAllDefinitions();
NativeMethods.InitializeProfiler(payload.DefinitionsId, payload.Definitions);
foreach (var def in payload.Definitions)
{
def.Dispose();
}
Expand Down
18 changes: 16 additions & 2 deletions tracer/src/Datadog.Trace/ClrProfiler/InstrumentationDefinitions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,23 @@ internal static partial class InstrumentationDefinitions
{
private static string assemblyFullName = typeof(InstrumentationDefinitions).Assembly.FullName;

internal static NativeCallTargetDefinition[] GetAllDefinitions()
internal static Payload GetAllDefinitions()
{
return GetDefinitionsArray();
return new Payload
{
// Fixed Id for definitions payload (to avoid loading same integrations from multiple AppDomains)
DefinitionsId = "FFAFA5168C4F4718B40CA8788875C2DA",

@zacharycmontoya zacharycmontoya Sep 20, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this ever change? Do you expect a scenario where we'd need to load definitions twice?

@tonyredondo tonyredondo Sep 20, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't expect that scenario tbh. I think we should load the integrations only once (like we did with the json file). So I don't expect that Id to change.


// Autogenerated definitions array
Definitions = GetDefinitionsArray(),
};
}

internal struct Payload
{
public string DefinitionsId { get; set; }

public NativeCallTargetDefinition[] Definitions { get; set; }
}
}
}
11 changes: 7 additions & 4 deletions tracer/src/Datadog.Trace/ClrProfiler/NativeCallTargetDefinition.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@
// </copyright>

using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Runtime.InteropServices;
using Datadog.Trace.Logging;

namespace Datadog.Trace.ClrProfiler
{
Expand Down Expand Up @@ -88,6 +84,13 @@ public NativeCallTargetDefinition(

public void Dispose()
{
var ptr = TargetSignatureTypes;
for (var i = 0; i < TargetSignatureTypesLength; i++)
{
Marshal.FreeHGlobal(Marshal.ReadIntPtr(ptr));
ptr += Marshal.SizeOf(typeof(IntPtr));
}

Marshal.FreeHGlobal(TargetSignatureTypes);
}
}
Expand Down
10 changes: 5 additions & 5 deletions tracer/src/Datadog.Trace/ClrProfiler/NativeMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ public static bool IsProfilerAttached()
return NonWindows.IsProfilerAttached();
}

public static void InitializeProfiler(NativeCallTargetDefinition[] methodArrays)
public static void InitializeProfiler(string id, NativeCallTargetDefinition[] methodArrays)
{
if (IsWindows)
{
Windows.InitializeProfiler(methodArrays, methodArrays.Length);
Windows.InitializeProfiler(id, methodArrays, methodArrays.Length);
}
else
{
NonWindows.InitializeProfiler(methodArrays, methodArrays.Length);
NonWindows.InitializeProfiler(id, methodArrays, methodArrays.Length);
}
}

Expand All @@ -43,7 +43,7 @@ private static class Windows
public static extern bool IsProfilerAttached();

[DllImport("Datadog.Trace.ClrProfiler.Native.dll")]
public static extern void InitializeProfiler([In] NativeCallTargetDefinition[] methodArrays, int size);
public static extern void InitializeProfiler([MarshalAs(UnmanagedType.LPWStr)] string id, [In] NativeCallTargetDefinition[] methodArrays, int size);
}

// assume .NET Core if not running on Windows
Expand All @@ -53,7 +53,7 @@ private static class NonWindows
public static extern bool IsProfilerAttached();

[DllImport("Datadog.Trace.ClrProfiler.Native")]
public static extern void InitializeProfiler([In] NativeCallTargetDefinition[] methodArrays, int size);
public static extern void InitializeProfiler([MarshalAs(UnmanagedType.LPWStr)] string id, [In] NativeCallTargetDefinition[] methodArrays, int size);
}
}
}