Merge pull request #865 from gbirchmeier/i864-dll-race

fix DefaultMessageFactory thread-race issue
connamara · Aug 27, 2024 · 6e127e6 · 6e127e6
2 parents 797a1a7 + e786d63
commit 6e127e6
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 50 deletions.
diff --git a/QuickFIXn/DefaultMessageFactory.cs b/QuickFIXn/DefaultMessageFactory.cs
@@ -1,9 +1,9 @@
-using System;
+#nullable enable
+using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
 using System.Reflection;
-using System.Threading;
 using QuickFix.Fields;
 
 namespace QuickFix
@@ -13,14 +13,12 @@ namespace QuickFix
     /// </summary>
     public class DefaultMessageFactory : IMessageFactory
     {
-        private static int _dllLoadFlag;
-
         /// <summary>
         /// key is BeginString (including the fake FIX50 beginstrings)
         /// </summary>
         private readonly IReadOnlyDictionary<string, IMessageFactory> _factories;
 
-        private QuickFix.Fields.ApplVerID _defaultApplVerId;
+        private readonly QuickFix.Fields.ApplVerID _defaultApplVerId;
 
         /// <summary>
         /// This constructor will
@@ -43,8 +41,10 @@ public DefaultMessageFactory(string defaultApplVerId = QuickFix.FixValues.ApplVe
         /// 2. Use them based on begin strings they support
         /// </summary>
         /// <param name="assemblies">Assemblies that may contain IMessageFactory implementations</param>
-        public DefaultMessageFactory(IEnumerable<Assembly> assemblies)
+        /// <param name="defaultApplVerId">ApplVerID value used by default in Create methods that don't explicitly specify it (only relevant for FIX5+)</param>
+        public DefaultMessageFactory(IEnumerable<Assembly> assemblies, string defaultApplVerId = QuickFix.FixValues.ApplVerID.FIX50SP2)
         {
+            _defaultApplVerId = new ApplVerID(defaultApplVerId);
             var factories = GetMessageFactories(assemblies);
             _factories = ConvertToDictionary(factories);
         }
@@ -61,21 +61,19 @@ public Message Create(string beginString, string msgType)
             return Create(beginString, _defaultApplVerId, msgType);
         }
 
-        public Message Create(string beginString, QuickFix.Fields.ApplVerID applVerID, string msgType)
+        public Message Create(string beginString, QuickFix.Fields.ApplVerID applVerId, string msgType)
         {
-            _factories.TryGetValue(beginString, out IMessageFactory messageFactory);
+            _factories.TryGetValue(beginString, out IMessageFactory? messageFactory);
 
             if (beginString == QuickFix.Values.BeginString_FIXT11 && !Message.IsAdminMsgType(msgType))
             {
-                if (applVerID == null)
-                    applVerID = _defaultApplVerId;
                 _factories.TryGetValue(
-                    QuickFix.FixValues.ApplVerID.ToBeginString(applVerID.Obj),
+                    QuickFix.FixValues.ApplVerID.ToBeginString(applVerId.Obj),
                     out messageFactory);
             }
 
             if (messageFactory != null)
-                return messageFactory.Create(beginString, applVerID, msgType);
+                return messageFactory.Create(beginString, applVerId, msgType);
 
             // didn't find a factory, so return a generic Message object
             var message = new Message();
@@ -87,24 +85,23 @@ public Group Create(string beginString, string msgType, int groupCounterTag)
         {
             string key = beginString;
             if(beginString.Equals(FixValues.BeginString.FIXT11))
-            {
                 key = QuickFix.FixValues.ApplVerID.ToBeginString(_defaultApplVerId.getValue());
-            }
 
-            if (_factories.TryGetValue(key, out var factory))
-            {
+            if (_factories.TryGetValue(key, out IMessageFactory? factory))
                 return factory.Create(beginString, msgType, groupCounterTag);
-            }
-            else
-            {
-                throw new UnsupportedVersion(beginString);
-            }
+
+            throw new UnsupportedVersion(beginString);
         }
 
         #endregion
 
         #region Dynamic assembly load related methods
 
+        /// <summary>
+        /// Creates a dictionary keyed by each IMessageFactory's supported BeginStrings
+        /// </summary>
+        /// <param name="factories"></param>
+        /// <returns></returns>
         private static Dictionary<string, IMessageFactory> ConvertToDictionary(IEnumerable<IMessageFactory> factories)
         {
             var dict = new Dictionary<string, IMessageFactory>();
@@ -119,41 +116,39 @@ private static Dictionary<string, IMessageFactory> ConvertToDictionary(IEnumerab
             return dict;
         }
 
+        private static bool _dllsAreLoaded = false;
+        private static readonly object _dllLoadSync = new object();
+
         private static void LoadLocalDlls()
         {
-            const int @true = 1;
-
-            // Because we want to attempt load assemblies once only
-            var loadFlag = Interlocked.Exchange(ref _dllLoadFlag, @true);
-            if (loadFlag == @true)
-            {
-                return;
-            }
-
-            try
+            lock (_dllLoadSync)
             {
-                var assemblyLocation = Assembly.GetExecutingAssembly().Location;
-                if (String.IsNullOrWhiteSpace(assemblyLocation))
-                {
+                // check again in case the load happened while this thread was waiting for the lock
+                if (_dllsAreLoaded)
                     return;
-                }
 
-                var directory = Path.GetDirectoryName(assemblyLocation);
-                if (String.IsNullOrWhiteSpace(directory))
+                try
                 {
-                    return;
-                }
+                    var assemblyLocation = Assembly.GetExecutingAssembly().Location;
+                    if (String.IsNullOrWhiteSpace(assemblyLocation))
+                        return;
+
+                    var directory = Path.GetDirectoryName(assemblyLocation);
+                    if (String.IsNullOrWhiteSpace(directory))
+                        return;
+
+                    var dlls = Directory.GetFiles(directory, "QuickFix.*.dll");
+                    foreach (var path in dlls)
+                        Assembly.LoadFrom(path);
 
-                var dlls = Directory.GetFiles(directory, "QuickFix.*.dll");
-                foreach (var path in dlls)
+                    _dllsAreLoaded = true;
+                }
+                catch (Exception ex)
                 {
-                    Assembly.LoadFrom(path);
+                    // TODO: can we log this properly instead of Console write?
+                    Console.Error.WriteLine("Found quickfix.*.dll dlls but failed to load them, " + ex);
                 }
             }
-            catch (Exception ex)
-            {
-                Console.Error.WriteLine("Found quickfix.*.dll dlls but failed to load them, " + ex);
-            }
         }
 
         private static ICollection<IMessageFactory> GetMessageFactories(IEnumerable<Assembly> assemblies)
@@ -165,7 +160,7 @@ private static ICollection<IMessageFactory> GetMessageFactories(IEnumerable<Asse
             var factories = new List<IMessageFactory>();
             foreach (var factoryType in factoryTypes)
             {
-                var factory = (IMessageFactory)Activator.CreateInstance(factoryType);
+                var factory = (IMessageFactory)Activator.CreateInstance(factoryType)!;
                 factories.Add(factory);
             }
 
@@ -178,7 +173,7 @@ private static ICollection<Assembly> GetAppDomainAssemblies()
             var assemblies = AppDomain
                 .CurrentDomain
                 .GetAssemblies()
-                .Where(assembly => !assembly.IsDynamic && assembly.GetName().Name.StartsWith("QuickFix", StringComparison.Ordinal))
+                .Where(assembly => !assembly.IsDynamic && assembly.GetName().Name!.StartsWith("QuickFix", StringComparison.Ordinal))
                 .ToList();
             return assemblies;
         }

diff --git a/QuickFIXn/IMessageFactory.cs b/QuickFIXn/IMessageFactory.cs
@@ -14,7 +14,9 @@ public interface IMessageFactory
         ICollection<string> GetSupportedBeginStrings();
 
         /// <summary>
-        /// Creates a message for a specified type and FIX version
+        /// Creates a message for a specified type and FIX version.
+        /// (FIXT11 apps should use the other Create method with explicit applVerId,
+        /// or devise some other way of specifying the FIX version)
         /// </summary>
         /// <param name="beginString">the FIX version (e.g. "FIX.4.2")</param>
         /// <param name="msgType">the FIX message type (e.g. "D" for a NewOrderSingle)</param>
@@ -25,7 +27,7 @@ public interface IMessageFactory
         /// Creates a message for a specified type, FIX version, and ApplVerID
         /// </summary>
         /// <param name="beginString">the FIX version (e.g. "FIX.4.2")</param>
-        /// <param name="applVerId">the ApplVerID (for example "6" for FIX44)</param>
+        /// <param name="applVerId">the ApplVerID to use if the BeginString is not specific enough (e.g. if it's FIXT11)</param>
         /// <param name="msgType">the FIX message type (e.g. "D" for a NewOrderSingle)</param>
         /// <returns>a message instance of proper derived type</returns>
         Message Create(string beginString, QuickFix.Fields.ApplVerID applVerId, string msgType);

diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
@@ -92,6 +92,10 @@ What's New
 * #839 - change ScreenLog to output FIX messages with "|" instead of non-visible SOH (gbirchmeier)
 * #844 - implement "Weekdays" setting (MichalUssuri/gbirchmeier)
 * #859 - implement proper path searching for CA certs in config (gbirchmeier)
+* #864 - when multiple threads race to init DefaultMessageFactory,
+         fix it so that later threads do not return before the first thread
+         finishes loading all the dlls (gbirchmeier, with thanks to diagnosis by Brian Leach aka baffled)
+         (Note: this may be the cause of spurious "incorrect BeginString" issues that have been observed)
 
 ### v1.11.2:
 * same as v1.11.1, but I fixed the readme in the pushed nuget packages