From aacb5cc01b1e8c2b28aabdbe9bd2d3f70fc358de Mon Sep 17 00:00:00 2001
From: Martijn Laarman <Mpdreamz@gmail.com>
Date: Mon, 5 Feb 2024 12:15:21 +0100
Subject: [PATCH] Address a few synchronization issues in the codebase (#2276)

* ensure Add() is locked on composite disposable

* Ensure _dynamicMethods is readonly in ILHelpersExtensions

* fix synchronization issue in ModuleLookup

* _reflectionObject should not be static in BinaryConverter

* dotnet format
---
 src/Elastic.Apm/ApmAgentExtensions.cs         | 10 ++++++--
 .../Converters/BinaryConverter.cs             |  4 +--
 .../DuckTyping/ILHelpersExtensions.cs         |  2 +-
 .../Reflection/ModuleLookup.cs                | 25 ++++++++++++-------
 4 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/src/Elastic.Apm/ApmAgentExtensions.cs b/src/Elastic.Apm/ApmAgentExtensions.cs
index 9c505e0fb..de238058c 100644
--- a/src/Elastic.Apm/ApmAgentExtensions.cs
+++ b/src/Elastic.Apm/ApmAgentExtensions.cs
@@ -109,8 +109,14 @@ public CompositeDisposable Add(IDisposable disposable)
 			if (_isDisposed)
 				throw new ObjectDisposedException(nameof(CompositeDisposable));
 
-			_disposables.Add(disposable);
-			return this;
+			lock (_lock)
+			{
+				if (_isDisposed)
+					throw new ObjectDisposedException(nameof(CompositeDisposable));
+
+				_disposables.Add(disposable);
+				return this;
+			}
 		}
 
 		public void Dispose()
diff --git a/src/Elastic.Apm/Libraries/Newtonsoft.Json/Converters/BinaryConverter.cs b/src/Elastic.Apm/Libraries/Newtonsoft.Json/Converters/BinaryConverter.cs
index 2df2811eb..70f605932 100644
--- a/src/Elastic.Apm/Libraries/Newtonsoft.Json/Converters/BinaryConverter.cs
+++ b/src/Elastic.Apm/Libraries/Newtonsoft.Json/Converters/BinaryConverter.cs
@@ -45,7 +45,7 @@ internal class BinaryConverter : JsonConverter
 #if HAVE_LINQ
         private const string BinaryTypeName = "System.Data.Linq.Binary";
         private const string BinaryToArrayName = "ToArray";
-        private static ReflectionObject? _reflectionObject;
+        private ReflectionObject? _reflectionObject;
 #endif
 
         /// <summary>
@@ -89,7 +89,7 @@ private byte[] GetByteArray(object value)
         }
 
 #if HAVE_LINQ
-        private static void EnsureReflectionObject(Type t)
+        private void EnsureReflectionObject(Type t)
         {
             if (_reflectionObject == null)
             {
diff --git a/src/profiler/Elastic.Apm.Profiler.Managed/DuckTyping/ILHelpersExtensions.cs b/src/profiler/Elastic.Apm.Profiler.Managed/DuckTyping/ILHelpersExtensions.cs
index ae9775a7d..93aba89b1 100644
--- a/src/profiler/Elastic.Apm.Profiler.Managed/DuckTyping/ILHelpersExtensions.cs
+++ b/src/profiler/Elastic.Apm.Profiler.Managed/DuckTyping/ILHelpersExtensions.cs
@@ -21,7 +21,7 @@ namespace Elastic.Apm.Profiler.Managed.DuckTyping
 	/// </summary>
 	internal static class ILHelpersExtensions
 	{
-		private static List<DynamicMethod> _dynamicMethods = new List<DynamicMethod>();
+		private static readonly List<DynamicMethod> _dynamicMethods = new();
 
 		internal static DynamicMethod GetDynamicMethodForIndex(int index)
 		{
diff --git a/src/profiler/Elastic.Apm.Profiler.Managed/Reflection/ModuleLookup.cs b/src/profiler/Elastic.Apm.Profiler.Managed/Reflection/ModuleLookup.cs
index 013b88ab8..cb206ba12 100644
--- a/src/profiler/Elastic.Apm.Profiler.Managed/Reflection/ModuleLookup.cs
+++ b/src/profiler/Elastic.Apm.Profiler.Managed/Reflection/ModuleLookup.cs
@@ -22,15 +22,17 @@ internal static class ModuleLookup
 		/// </summary>
 		private const int MaxFailures = 50;
 
-		private static ManualResetEventSlim _populationResetEvent = new ManualResetEventSlim(initialState: true);
-		private static ConcurrentDictionary<Guid, Module> _modules = new ConcurrentDictionary<Guid, Module>();
+		private static readonly ManualResetEventSlim _populationResetEvent = new(initialState: true);
+		private static readonly ConcurrentDictionary<Guid, Module> _modules = new();
 
-		private static int _failures;
-		private static bool _shortCircuitLogicHasLogged = false;
+		private static long _failures;
 
 		public static Module GetByPointer(long moduleVersionPointer) =>
 			Get(Marshal.PtrToStructure<Guid>(new IntPtr(moduleVersionPointer)));
 
+		private static readonly object _logLock = new();
+		private static bool _shortCircuitLogicHasLogged = false;
+
 		public static Module Get(Guid moduleVersionId)
 		{
 			// First attempt at cached values with no blocking
@@ -44,15 +46,20 @@ public static Module Get(Guid moduleVersionId)
 			if (_modules.TryGetValue(moduleVersionId, out value))
 				return value;
 
-			if (_failures >= MaxFailures)
+			var failures = Interlocked.Read(ref _failures);
+			if (failures >= MaxFailures)
 			{
-				// For some unforeseeable reason we have failed on a lot of AppDomain lookups
-				if (!_shortCircuitLogicHasLogged)
+				if (_shortCircuitLogicHasLogged)
+					return null;
+				lock (_logLock)
 				{
+					if (_shortCircuitLogicHasLogged)
+						return null;
 					Logger.Log(LogLevel.Warn,
 						"Elastic APM is unable to continue attempting module lookups for this AppDomain. Falling back to legacy method lookups.");
-				}
+					_shortCircuitLogicHasLogged = true;
 
+				}
 				return null;
 			}
 
@@ -65,7 +72,7 @@ public static Module Get(Guid moduleVersionId)
 			}
 			catch (Exception ex)
 			{
-				_failures++;
+				Interlocked.Increment(ref _failures);
 				Logger.Log(LogLevel.Error, ex, "Error when populating modules.");
 			}
 			finally