Merge pull request #5555 from Particular/patch721

Patch 7.2.1

Author: andreasohlund

src/NServiceBus.Core.Tests/MessageMapper/MessageMapperTests.cs

@@ -156,15 +156,13 @@ public void Should_map_structs()
         }
 
         [Test]
-        public void Should_fail_for_interfaces_with_invalid_attributes()
+        public void Should_handle_interfaces_that_have_attributes_with_nullable_properties()
         {
-            var ex = Assert.Throws<ArgumentException>(() => 
-                new MessageMapper().Initialize(new[]
-                {
-                    typeof(MessageInterfaceWithPropertyWithIllegalAttribute)
-                }));
+            var mapper = new MessageMapper();
 
-            StringAssert.Contains("An invalid type was used as a custom attribute constructor argument, field or property.", ex.Message);
+            var messageInstance = mapper.CreateInstance<MessageInterfaceWithNullablePropertyAttribute>();
+
+            Assert.IsNotNull(messageInstance);
         }
 
         public class SampleMessageClass
@@ -229,20 +227,19 @@ interface IPrivateInterfaceMessage
         }
 
         [AttributeUsage(AttributeTargets.Property)]
-        public class IllegalAttribute : Attribute
+        public class NullablePropertyAttribute : Attribute
         {
-            // our mapper does not support custom attributes with nullable properties
             public int? IntKey { get; set; }
 
-            public IllegalAttribute(int x)
+            public NullablePropertyAttribute(int x)
             {
                 IntKey = x;
             }
         }
 
-        public interface MessageInterfaceWithPropertyWithIllegalAttribute
+        public interface MessageInterfaceWithNullablePropertyAttribute
         {
-            [Illegal(0)]
+            [NullableProperty(0)]
             object Value { get; set; }
         }
 

src/NServiceBus.Core.Tests/NServiceBus.Core.Tests.csproj

@@ -16,6 +16,12 @@
     <Reference Include="System.Transactions" />
   </ItemGroup>
 
+  <!-- Workaround reference because we can't use nullable reference types yet -->
+  <ItemGroup>
+    <Reference Include="WithDodgyNullable" HintPath="TestDlls\WithDodgyNullable.dll" />
+  </ItemGroup>
+  <!-- End workaround -->
+
   <ItemGroup>
     <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.2.0" />
     <PackageReference Include="Mono.Cecil" Version="0.10.4" />

src/NServiceBus.Core.Tests/Persistence/InMemory/ClientIdStorageTests.cs

@@ -0,0 +1,48 @@
+namespace NServiceBus.Persistence.InMemory.Tests
+{
+    using NUnit.Framework;
+
+    [TestFixture]
+    class ClientIdStorageTests
+    {
+        [Test]
+        public void Should_detect_duplicates()
+        {
+            var clientIdStorage = new ClientIdStorage(10);
+
+            clientIdStorage.RegisterClientId("A");
+            clientIdStorage.RegisterClientId("B");
+
+            Assert.True(clientIdStorage.IsDuplicate("A"));
+            Assert.False(clientIdStorage.IsDuplicate("C"));
+        }
+
+        [Test]
+        public void Should_evict_oldest_entry_when_LRU_reaches_limit()
+        {
+            var clientIdStorage = new ClientIdStorage(2);
+
+            clientIdStorage.RegisterClientId("A");
+            clientIdStorage.RegisterClientId("B");
+            clientIdStorage.RegisterClientId("C");
+
+            Assert.False(clientIdStorage.IsDuplicate("A"));
+        }
+
+        [Test]
+        public void Should_reset_time_added_for_existing_IDs_when_checked()
+        {
+            var clientIdStorage = new ClientIdStorage(2);
+
+            clientIdStorage.RegisterClientId("A");
+            clientIdStorage.RegisterClientId("B");
+
+            Assert.True(clientIdStorage.IsDuplicate("A"));
+
+            clientIdStorage.RegisterClientId("C");
+
+            Assert.False(clientIdStorage.IsDuplicate("B"));
+            Assert.True(clientIdStorage.IsDuplicate("A"));
+        }
+    }
+}

src/NServiceBus.Core.Tests/Persistence/InMemory/InMemoryGatewayDeduplicationTests.cs

@@ -2,59 +2,72 @@
 {
     using System;
     using System.Threading.Tasks;
+    using System.Transactions;
     using Extensibility;
     using Features;
-    using Gateway.Deduplication;
     using NUnit.Framework;
     using Settings;
 
     [TestFixture]
     class InMemoryGatewayDeduplicationTests
     {
         [Test]
-        public async Task Should_return_true_on_first_unique_test()
+        public void Should_have_configured_storage_maxsize()
         {
-            var storage = new InMemoryGatewayDeduplication(2);
+            var settings = new SettingsHolder();
+            var persistenceSettings = new PersistenceExtensions<InMemoryPersistence>(settings);
 
-            await storage.DeduplicateMessage("A", DateTime.UtcNow, new ContextBag());
-            Assert.True(await storage.DeduplicateMessage("B", DateTime.UtcNow, new ContextBag()));
-        }    
+            persistenceSettings.GatewayDeduplicationCacheSize(42);
+
+            Assert.AreEqual(42, settings.Get<int>(InMemoryGatewayPersistence.MaxSizeKey));
+        }
 
         [Test]
-        public async Task Should_return_false_on_second_test()
+        public async Task Should_add_on_transaction_scope_commit()
         {
-            var storage = new InMemoryGatewayDeduplication(2);
+            var storage = CreateInMemoryGatewayDeduplication();
+
+            using (var scope = new TransactionScope(TransactionScopeOption.RequiresNew, TransactionScopeAsyncFlowOption.Enabled))
+            {
+                await storage.DeduplicateMessage("A", DateTime.UtcNow, new ContextBag());
+
+                scope.Complete();
+            }
 
-            await storage.DeduplicateMessage("A", DateTime.UtcNow, new ContextBag());
             Assert.False(await storage.DeduplicateMessage("A", DateTime.UtcNow, new ContextBag()));
-        }    
-        
+        }
+
         [Test]
-        public async Task Should_return_true_if_LRU_reaches_limit()
+        public async Task Should_not_add_on_transaction_scope_abort()
         {
-            var storage = new InMemoryGatewayDeduplication(2);
+            var storage = CreateInMemoryGatewayDeduplication();
+
+            using (new TransactionScope(TransactionScopeOption.RequiresNew, TransactionScopeAsyncFlowOption.Enabled))
+            {
+                await storage.DeduplicateMessage("A", DateTime.UtcNow, new ContextBag());
+
+                // no commit
+            }
 
-            await storage.DeduplicateMessage("A", DateTime.UtcNow, new ContextBag());
-            await storage.DeduplicateMessage("B", DateTime.UtcNow, new ContextBag());
-            await storage.DeduplicateMessage("C", DateTime.UtcNow, new ContextBag());
             Assert.True(await storage.DeduplicateMessage("A", DateTime.UtcNow, new ContextBag()));
         }
 
         [Test]
-        public void Should_have_configured_maxsize()
+        // With the gateway persistence seam v1 design it's only safe to deduplicate when there is a tx scope present since the check happens before
+        // the messages have been pushed to the transport. If we add entries earlier they would be considered duplicate when retrying after something went wrong.
+        // Note: The gateway will always wrap the v1 seam invocation in a transaction scope
+        public async Task Should_only_deduplicate_when_scope_is_present()
         {
-            var feature = new InMemoryGatewayPersistence();
-            var settings = new SettingsHolder();
-            var container = new CommonObjectBuilder(new LightInjectObjectBuilder());
+            var storage = CreateInMemoryGatewayDeduplication();
 
-            var persistenceSettings = new PersistenceExtensions<InMemoryPersistence>(settings);
-            persistenceSettings.GatewayDeduplicationCacheSize(42);
-
-            feature.Setup(new FeatureConfigurationContext(settings, container, null, null, null));
+            await storage.DeduplicateMessage("A", DateTime.UtcNow, new ContextBag());
 
-            var implementation = (InMemoryGatewayDeduplication)container.Build<IDeduplicateMessages>();
-            Assert.AreEqual(42, implementation.maxSize);
+            Assert.True(await storage.DeduplicateMessage("A", DateTime.UtcNow, new ContextBag()));
         }
 
+        InMemoryGatewayDeduplication CreateInMemoryGatewayDeduplication()
+        {
+            return new InMemoryGatewayDeduplication(new ClientIdStorage(10));
+        }
     }
-}
+}

src/NServiceBus.Core/MessageInterfaces/MessageMapper/Reflection/ConcreteProxyCreator.cs

@@ -2,7 +2,6 @@ namespace NServiceBus
 {
     using System;
     using System.Collections.Generic;
-    using System.ComponentModel;
     using System.Linq;
     using System.Reflection;
     using System.Reflection.Emit;
@@ -43,7 +42,7 @@ public Type CreateTypeFrom(Type type)
                     propertyType,
                     null);
 
-                foreach (var customAttribute in prop.GetCustomAttributes(true))
+                foreach (var customAttribute in prop.GetCustomAttributesData())
                 {
                     AddCustomAttributeToProperty(customAttribute, propBuilder);
                 }
@@ -93,85 +92,31 @@ public Type CreateTypeFrom(Type type)
         }
 
         /// <summary>
-        /// Given a custom attribute and property builder, adds an instance of custom attribute
+        /// Given a custom attribute and property builder, adds an instance of the custom attribute
         /// to the property builder
         /// </summary>
-        void AddCustomAttributeToProperty(object customAttribute, PropertyBuilder propBuilder)
+        static void AddCustomAttributeToProperty(CustomAttributeData attributeData, PropertyBuilder propBuilder)
         {
-            var customAttributeBuilder = BuildCustomAttribute(customAttribute);
-            if (customAttributeBuilder != null)
-            {
-                propBuilder.SetCustomAttribute(customAttributeBuilder);
-            }
-        }
+            var namedArguments = attributeData.NamedArguments;
 
-        static CustomAttributeBuilder BuildCustomAttribute(object customAttribute)
-        {
-            ConstructorInfo longestCtor = null;
-            // Get constructor with the largest number of parameters
-            foreach (var cInfo in customAttribute.GetType().GetConstructors().
-                Where(cInfo => longestCtor == null || longestCtor.GetParameters().Length < cInfo.GetParameters().Length))
+            if (namedArguments == null)
             {
-                longestCtor = cInfo;
-            }
+                var attributeBuilder = new CustomAttributeBuilder(
+                    attributeData.Constructor,
+                    attributeData.ConstructorArguments.Select(x => x.Value).ToArray());
 
-            if (longestCtor == null)
-            {
-                return null;
+                propBuilder.SetCustomAttribute(attributeBuilder);
             }
-
-            // For each constructor parameter, get corresponding (by name similarity) property and get its value
-            var args = new object[longestCtor.GetParameters().Length];
-            var position = 0;
-            foreach (var consParamInfo in longestCtor.GetParameters())
+            else
             {
-                var attrPropInfo = customAttribute.GetType().GetProperty(consParamInfo.Name, BindingFlags.Public | BindingFlags.Instance | BindingFlags.IgnoreCase);
-                if (attrPropInfo != null)
-                {
-                    args[position] = attrPropInfo.GetValue(customAttribute, null);
-                }
-                else
-                {
-                    args[position] = null;
-                    var attrFieldInfo = customAttribute.GetType().GetField(consParamInfo.Name, BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.IgnoreCase);
-                    if (attrFieldInfo == null)
-                    {
-                        if (consParamInfo.ParameterType.IsValueType)
-                        {
-                            args[position] = Activator.CreateInstance(consParamInfo.ParameterType);
-                        }
-                    }
-                    else
-                    {
-                        args[position] = attrFieldInfo.GetValue(customAttribute);
-                    }
-                }
-                ++position;
-            }
+                var attributeBuilder = new CustomAttributeBuilder(
+                    attributeData.Constructor,
+                    attributeData.ConstructorArguments.Select(x => x.Value).ToArray(),
+                    namedArguments.Select(x => (PropertyInfo)x.MemberInfo).ToArray(),
+                    namedArguments.Select(x => x.TypedValue.Value).ToArray());
 
-            var propList = new List<PropertyInfo>();
-            var propValueList = new List<object>();
-            foreach (var attrPropInfo in customAttribute.GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance))
-            {
-                if (!attrPropInfo.CanWrite)
-                {
-                    continue;
-                }
-                object defaultValue = null;
-                var defaultValueAttribute = attrPropInfo.GetCustomAttribute<DefaultValueAttribute>(true);
-                if (defaultValueAttribute != null)
-                {
-                    defaultValue = defaultValueAttribute.Value;
-                }
-                var value = attrPropInfo.GetValue(customAttribute, null);
-                if (value == defaultValue)
-                {
-                    continue;
-                }
-                propList.Add(attrPropInfo);
-                propValueList.Add(value);
+                propBuilder.SetCustomAttribute(attributeBuilder);
             }
-            return new CustomAttributeBuilder(longestCtor, args, propList.ToArray(), propValueList.ToArray());
         }
 
         /// <summary>

src/NServiceBus.Core/Persistence/InMemory/Gateway/ClientIdStorage.cs

@@ -0,0 +1,58 @@
+namespace NServiceBus
+{
+    using System.Collections.Generic;
+
+    class ClientIdStorage
+    {
+        public ClientIdStorage(int maxSize)
+        {
+            this.maxSize = maxSize;
+        }
+
+        public bool IsDuplicate(string clientId)
+        {
+            lock (clientIdSet)
+            {
+                // This implementation is not 100% consistent since there is a race condition here where another thread might consider the same message as a non-duplicate.
+                // We consider this good enough since this is the inmemory persister which will not be consistent when the endpoint is scaled out anyway.
+                if (clientIdSet.TryGetValue(clientId, out var existingNode)) // O(1)
+                {
+                    clientIdList.Remove(existingNode); // O(1) operation, because we got the node reference
+                    clientIdList.AddLast(existingNode); // O(1) operation
+
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
+        public void RegisterClientId(string clientId)
+        {
+            lock (clientIdSet)
+            {
+                //another thread might already have added the ID since we checked the last time
+                if (clientIdSet.ContainsKey(clientId))
+                {
+                    // another thread has proceed this ID already and there is a potential duplicate message but there is nothing we can do about it at this stage so just return.
+                    // Throwing would just cause unessessary retries for the client.
+                    return;
+                }
+
+                if (clientIdSet.Count == maxSize)
+                {
+                    var id = clientIdList.First.Value;
+                    clientIdSet.Remove(id); // O(1)
+                    clientIdList.RemoveFirst(); // O(1)
+                }
+
+                var node = clientIdList.AddLast(clientId); // O(1)
+                clientIdSet.Add(clientId, node); // O(1)
+            }
+        }
+
+        readonly int maxSize;
+        readonly LinkedList<string> clientIdList = new LinkedList<string>();
+        readonly Dictionary<string, LinkedListNode<string>> clientIdSet = new Dictionary<string, LinkedListNode<string>>();
+    }
+}