From 50c14b8e4e9dc2fb8f5d4491800be916596eeb61 Mon Sep 17 00:00:00 2001 From: Mark Randell Date: Thu, 9 Dec 2021 15:26:23 -0330 Subject: [PATCH] many improvements including deadlock avoidance by removing use of locks, replacement of slow reflection based method invokes with direct interface calls, fix publish calls where generic type is base type of T such as object, a few minor performance tweaks. All tests passing. --- .editorconfig | 132 +++++++++++ src/Hellang.MessageBus/HelperExtensions.cs | 46 +++- src/Hellang.MessageBus/IMessageBus.cs | 6 +- src/Hellang.MessageBus/MessageBus.cs | 205 ++++++++++++------ .../MessageBusTests.cs | 13 +- .../Subscribers/UIThreadSingleSubscriber.cs | 3 +- 6 files changed, 318 insertions(+), 87 deletions(-) create mode 100644 .editorconfig diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 0000000..e537528 --- /dev/null +++ b/.editorconfig @@ -0,0 +1,132 @@ +# To learn more about .editorconfig see https://aka.ms/editorconfigdocs +############################### +# Core EditorConfig Options # +############################### +# All files +[*] +indent_style = space + +# XML project files +[*.{csproj,vbproj,vcxproj,vcxproj.filters,proj,projitems,shproj}] +indent_size = 2 + +# XML config files +[*.{props,targets,ruleset,config,nuspec,resx,vsixmanifest,vsct}] +indent_size = 2 + +# Code files +[*.{cs,csx,vb,vbx}] +indent_size = 4 +insert_final_newline = true +charset = utf-8-bom +############################### +# .NET Coding Conventions # +############################### +[*.{cs,vb}] +# Organize usings +dotnet_sort_system_directives_first = true +# this. preferences +dotnet_style_qualification_for_field = false:silent +dotnet_style_qualification_for_property = false:silent +dotnet_style_qualification_for_method = false:silent +dotnet_style_qualification_for_event = false:silent +# Language keywords vs BCL types preferences +dotnet_style_predefined_type_for_locals_parameters_members = true:silent +dotnet_style_predefined_type_for_member_access = true:silent +# Parentheses preferences +dotnet_style_parentheses_in_arithmetic_binary_operators = always_for_clarity:silent +dotnet_style_parentheses_in_relational_binary_operators = always_for_clarity:silent +dotnet_style_parentheses_in_other_binary_operators = always_for_clarity:silent +dotnet_style_parentheses_in_other_operators = never_if_unnecessary:silent +# Modifier preferences +dotnet_style_require_accessibility_modifiers = for_non_interface_members:silent +dotnet_style_readonly_field = true:suggestion +# Expression-level preferences +dotnet_style_object_initializer = true:suggestion +dotnet_style_collection_initializer = true:suggestion +dotnet_style_explicit_tuple_names = true:suggestion +dotnet_style_null_propagation = true:suggestion +dotnet_style_coalesce_expression = true:suggestion +dotnet_style_prefer_is_null_check_over_reference_equality_method = true:silent +dotnet_style_prefer_inferred_tuple_names = true:suggestion +dotnet_style_prefer_inferred_anonymous_type_member_names = true:suggestion +dotnet_style_prefer_auto_properties = true:silent +dotnet_style_prefer_conditional_expression_over_assignment = true:silent +dotnet_style_prefer_conditional_expression_over_return = true:silent +############################### +# Naming Conventions # +############################### +# Style Definitions +dotnet_naming_style.pascal_case_style.capitalization = pascal_case +# Use PascalCase for constant fields +dotnet_naming_rule.constant_fields_should_be_pascal_case.severity = suggestion +dotnet_naming_rule.constant_fields_should_be_pascal_case.symbols = constant_fields +dotnet_naming_rule.constant_fields_should_be_pascal_case.style = pascal_case_style +dotnet_naming_symbols.constant_fields.applicable_kinds = field +dotnet_naming_symbols.constant_fields.applicable_accessibilities = * +dotnet_naming_symbols.constant_fields.required_modifiers = const +############################### +# C# Coding Conventions # +############################### +[*.cs] +# var preferences +csharp_style_var_for_built_in_types = true:silent +csharp_style_var_when_type_is_apparent = true:silent +csharp_style_var_elsewhere = true:silent +# Expression-bodied members +csharp_style_expression_bodied_methods = false:silent +csharp_style_expression_bodied_constructors = false:silent +csharp_style_expression_bodied_operators = false:silent +csharp_style_expression_bodied_properties = true:silent +csharp_style_expression_bodied_indexers = true:silent +csharp_style_expression_bodied_accessors = true:silent +# Pattern matching preferences +csharp_style_pattern_matching_over_is_with_cast_check = true:suggestion +csharp_style_pattern_matching_over_as_with_null_check = true:suggestion +# Null-checking preferences +csharp_style_throw_expression = true:suggestion +csharp_style_conditional_delegate_call = true:suggestion +# Modifier preferences +csharp_preferred_modifier_order = public,private,protected,internal,static,extern,new,virtual,abstract,sealed,override,readonly,unsafe,volatile,async:suggestion +# Expression-level preferences +csharp_prefer_braces = true:silent +csharp_style_deconstructed_variable_declaration = true:suggestion +csharp_prefer_simple_default_expression = true:suggestion +csharp_style_pattern_local_over_anonymous_function = true:suggestion +csharp_style_inlined_variable_declaration = true:suggestion +############################### +# C# Formatting Rules # +############################### +# New line preferences +csharp_new_line_before_open_brace = all +csharp_new_line_before_else = true +csharp_new_line_before_catch = true +csharp_new_line_before_finally = true +csharp_new_line_before_members_in_object_initializers = true +csharp_new_line_before_members_in_anonymous_types = true +csharp_new_line_between_query_expression_clauses = true +# Indentation preferences +csharp_indent_case_contents = true +csharp_indent_switch_labels = true +csharp_indent_labels = flush_left +# Space preferences +csharp_space_after_cast = false +csharp_space_after_keywords_in_control_flow_statements = true +csharp_space_between_method_call_parameter_list_parentheses = false +csharp_space_between_method_declaration_parameter_list_parentheses = false +csharp_space_between_parentheses = false +csharp_space_before_colon_in_inheritance_clause = true +csharp_space_after_colon_in_inheritance_clause = true +csharp_space_around_binary_operators = before_and_after +csharp_space_between_method_declaration_empty_parameter_list_parentheses = false +csharp_space_between_method_call_name_and_opening_parenthesis = false +csharp_space_between_method_call_empty_parameter_list_parentheses = false +# Wrapping preferences +csharp_preserve_single_line_statements = true +csharp_preserve_single_line_blocks = true +############################### +# VB Coding Conventions # +############################### +[*.vb] +# Modifier preferences +visual_basic_preferred_modifier_order = Partial,Default,Private,Protected,Public,Friend,NotOverridable,Overridable,MustOverride,Overloads,Overrides,MustInherit,NotInheritable,Static,Shared,Shadows,ReadOnly,WriteOnly,Dim,Const,WithEvents,Widening,Narrowing,Custom,Async:suggestion diff --git a/src/Hellang.MessageBus/HelperExtensions.cs b/src/Hellang.MessageBus/HelperExtensions.cs index ec79255..67e95a8 100644 --- a/src/Hellang.MessageBus/HelperExtensions.cs +++ b/src/Hellang.MessageBus/HelperExtensions.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; using System.Reflection; @@ -48,16 +49,36 @@ public static void ForEach(this IEnumerable source, Action action) /// /// Removes all items in the specified list which matches the specified predicate. /// - /// The type of items. + /// The key type of items. + /// The value type of items. /// The list. /// The predicate. - public static void RemoveAll(this IList list, Func predicate) + public static int RemoveAll(this ConcurrentDictionary list, Func predicate) { - var toRemove = list.Where(predicate).ToList(); - foreach (var item in toRemove) + int i = 0; + foreach (var kvp in list.Where(kvp => predicate(kvp.Value))) { - list.Remove(item); + if (list.TryRemove(kvp.Key, out _)) i++; } + + return i; + } + + /// + /// Removes all items in the specified list which matches the specified predicate. + /// + /// The key type of items. + /// The value type of items. + /// The list. + /// The predicate. + public static bool RemoveFirst(this ConcurrentDictionary list, Func predicate) + { + foreach (var kvp in list.Where(kvp => predicate(kvp.Value))) + { + return (list.TryRemove(kvp.Key, out _)); + } + + return false; } /// @@ -68,7 +89,8 @@ public static void RemoveAll(this IList list, Func predicate) /// The handle method for the specified message type or null. public static MethodInfo GetHandleMethodFor(this Type type, Type messageType) { - var m= type.GetMethods(BindingFlags.Public | BindingFlags.Instance).SingleOrDefault(method => method.IsHandleMethodFor(messageType)); + var m = type.GetMethods(BindingFlags.Public | BindingFlags.Instance) + .SingleOrDefault(method => method.IsHandleMethodFor(messageType)); if (m == null) { // look for explicit method implementation @@ -87,7 +109,7 @@ public static MethodInfo GetHandleMethodFor(this Type type, Type messageType) /// /// The type. /// The first generic argument of the specified type. - private static Type FirstGenericArgument(this Type type) + internal static Type FirstGenericArgument(this Type type) { return type.GetGenericArguments().First(); } @@ -97,7 +119,7 @@ private static Type FirstGenericArgument(this Type type) /// /// The type. /// List of interface types. - private static IEnumerable GetHandleInterfaces(this Type type) + internal static IEnumerable GetHandleInterfaces(this Type type) { return type.GetInterfaces().Where(IsHandleInterface); } @@ -124,9 +146,9 @@ private static bool IsHandleInterface(this Type type) /// private static bool IsHandleMethodFor(this MethodInfo method, Type messageType) { - return method.Name == "Handle" - && method.ReturnType == typeof(void) - && method.HasSingleParameterOfType(messageType); + return method.Name == "Handle" + && method.ReturnType == typeof(void) + && method.HasSingleParameterOfType(messageType); } /// @@ -145,4 +167,4 @@ private static bool HasSingleParameterOfType(this MethodBase method, Type parame return parameters.First().ParameterType == parameterType; } } -} \ No newline at end of file +} diff --git a/src/Hellang.MessageBus/IMessageBus.cs b/src/Hellang.MessageBus/IMessageBus.cs index b80fd2c..cf77c31 100644 --- a/src/Hellang.MessageBus/IMessageBus.cs +++ b/src/Hellang.MessageBus/IMessageBus.cs @@ -10,13 +10,13 @@ public interface IMessageBus : IHideObjectMembers /// through implementations of . /// /// The target to subscribe for event publication. - void Subscribe(object target); + bool Subscribe(object target); /// /// Unsubscribes the specified target from all events. /// /// The target to unsubscribe. - void Unsubscribe(object target); + bool Unsubscribe(object target); /// /// Publishes a new message of the given message type. @@ -36,4 +36,4 @@ public interface IMessageBus : IHideObjectMembers /// void Clear(); } -} \ No newline at end of file +} diff --git a/src/Hellang.MessageBus/MessageBus.cs b/src/Hellang.MessageBus/MessageBus.cs index eed6a29..03cb530 100644 --- a/src/Hellang.MessageBus/MessageBus.cs +++ b/src/Hellang.MessageBus/MessageBus.cs @@ -9,7 +9,9 @@ namespace Hellang.MessageBus /// /// A marker interface for classes that can subscribe to messages. /// - public interface IHandle { } + public interface IHandle + { + } /// /// Denotes a class which can handle a particular type of message. @@ -29,17 +31,19 @@ public interface IHandle : IHandle /// handling should be done on the UI thread. /// [AttributeUsage(AttributeTargets.Method)] - public class HandleOnUIThreadAttribute : Attribute { } + public class HandleOnUiThreadAttribute : Attribute + { + } /// /// Enables loosely-coupled publication of and subscription to messages. /// public class MessageBus : IMessageBus { - private static Action _uiThreadMarshaller; + private static Action uiThreadMarshaller; - private readonly List _subscribers = new List(); - private readonly object _lock = new object(); + private readonly ConcurrentDictionary + subscribers = new ConcurrentDictionary(); /// /// Initializes a new instance of the class without UI thread marshalling. @@ -52,7 +56,7 @@ public MessageBus() { } /// The action for marshalling invocation to the UI thread. public MessageBus(Action uiThreadMarshaller) { - _uiThreadMarshaller = uiThreadMarshaller; + MessageBus.uiThreadMarshaller = uiThreadMarshaller; } /// @@ -60,24 +64,22 @@ public MessageBus(Action uiThreadMarshaller) /// through implementations of . /// /// The target to subscribe for event publication. - public void Subscribe(object target) + public bool Subscribe(object target) { - WhileLocked(() => - { - if (!_subscribers.Any(s => s.Matches(target))) - { - _subscribers.Add(new Subscriber(target)); - } - }); + if (target == null) throw new ArgumentNullException(nameof(target)); + var hash = target.GetHashCode(); + return subscribers.TryAdd(hash, new Subscriber(target, hash)); } /// /// Unsubscribes the specified target from all events. /// /// The target to unsubscribe. - public void Unsubscribe(object target) + public bool Unsubscribe(object target) { - WhileLocked(() => _subscribers.RemoveAll(s => s.Matches(target))); + if (target == null) throw new ArgumentNullException(nameof(target)); + var hash = target.GetHashCode(); + return subscribers.TryRemove(hash, out _); } /// @@ -86,7 +88,7 @@ public void Unsubscribe(object target) /// The type of message to publish. public void Publish() where T : new() { - Publish(new T()); + PublishCore(new T(), typeof(T)); } /// @@ -96,66 +98,72 @@ public void Unsubscribe(object target) /// The message. public void Publish(T message) { - WhileLocked(() => _subscribers.RemoveAll(s => !s.Handle(message))); + PublishCore(message, message.GetType()); } /// - /// Clears all subscribers. + /// Publishes the specified message and removes all dead subscribers. /// - public void Clear() + protected virtual void PublishCore(T message, Type t) { - WhileLocked(() => _subscribers.Clear()); + subscribers.RemoveAll(s => !s.Handle(message, t)); } - private void WhileLocked(Action action) + /// + /// Clears all subscribers. + /// + public void Clear() { - lock (_lock) { action(); } + subscribers.Clear(); } + /// /// A is a wrapper for an instance subscribed /// to messages from a . It can have many handler methods - /// which is represented by the class. + /// which is represented by the class. /// - private class Subscriber + private class Subscriber : IEquatable { - private static readonly ConcurrentDictionary> HandlerCache = new ConcurrentDictionary>(); + private static readonly ConcurrentDictionary> handlerCache = + new ConcurrentDictionary>(); - private readonly WeakReference _weakReference; - private readonly IList _handlers; + private readonly WeakReference weakReference; + private readonly IList handlers; + private readonly int hash; /// /// Initializes a new instance of the class. /// /// The target to subscribe. - public Subscriber(object target) + /// + public Subscriber(object target, int hash) { - _weakReference = new WeakReference(target); - _handlers = GetHandlers(target); + if (target == null) throw new ArgumentNullException(nameof(target)); + this.hash = hash; + weakReference = new WeakReference(target); + handlers = GetHandlers(target); } - /// - /// Checks if the specified target matches the subscribed target. - /// - /// The target to match. - /// true if the target matches, false otherwise. - public bool Matches(object target) - { - return _weakReference.Target == target; - } + //internal bool IsAlive => _weakReference.IsAlive; /// /// Handles the specified message. /// /// The type of message to handle. /// The message. - /// true if the message was handled successfully, false if the target is dead. - public bool Handle(T message) + /// + /// true if the target handler is alive, false if the target is dead. + public bool Handle(T message, Type messageType) { - var target = _weakReference.Target; + var target = weakReference.Target; if (target == null) return false; - - _handlers.Where(h => h.CanHandle(typeof(T))).ForEach(h => h.Invoke(target, message)); + // high traffic method, do not use linq here. + foreach (var h in handlers) + { + if (!h.CanHandle(messageType)) continue; + h.Invoke(target, message); + } return true; } @@ -165,18 +173,19 @@ public bool Handle(T message) /// /// The target. /// List of handlers. - private static IList GetHandlers(object target) + private static IList GetHandlers(object target) { var targetType = target.GetType(); - var handlers = HandlerCache.GetOrAdd(targetType, AddFactory); + var handlers = handlerCache.GetOrAdd(targetType, AddFactory); return handlers; - static IList AddFactory(Type targetType) { + static IList AddFactory(Type targetType) + { // No handlers cached, use reflection to get them. return CreateHandlers(targetType).ToArray(); } } - + /// /// Gets a list of handlers for the specified type. /// @@ -184,37 +193,82 @@ static IList AddFactory(Type targetType) { /// /// List of handlers for the specified type. /// - private static IEnumerable CreateHandlers(Type targetType) + private static IEnumerable CreateHandlers(Type targetType) { - foreach (var messageType in targetType.GetMessageTypes()) + foreach (var handleInterface in targetType.GetHandleInterfaces()) { + var messageType = handleInterface.FirstGenericArgument(); var handlerMethod = targetType.GetHandleMethodFor(messageType); if (handlerMethod == null) continue; - - yield return new Handler(messageType, handlerMethod); + var handler = (IHandler)Activator.CreateInstance(typeof(Handler<>).MakeGenericType(messageType)); + handler.Initialize(handlerMethod.HasAttribute()); + yield return handler; } } /// - /// The class is a wrapper + /// Checks if the specified target matches the subscribed target. + /// + /// The target to match. + /// true if the target matches, false otherwise. + public bool Matches(object target) + { + return weakReference.IsAlive && Equals(weakReference.Target, target); + } + + public bool Equals(Subscriber other) + { + if (other is null || !weakReference.IsAlive || !other.weakReference.IsAlive) return false; + if (ReferenceEquals(this, other)) return true; + return hash == other.hash || Equals(weakReference.Target, other.weakReference.Target); + } + + public override bool Equals(object obj) + { + if (ReferenceEquals(null, obj)) return false; + if (ReferenceEquals(this, obj)) return true; + if (obj.GetType() != this.GetType()) return false; + return Equals((Subscriber)obj); + } + + public override int GetHashCode() => hash; + + public static bool operator ==(Subscriber left, Subscriber right) + { + return Equals(left, right); + } + + public static bool operator !=(Subscriber left, Subscriber right) + { + return !Equals(left, right); + } + + /// + /// The class is a wrapper /// for a method which can handle a specific message type. /// - private class Handler + private class Handler : IHandler { - private readonly Type _messageType; - private readonly MethodInfo _method; - private readonly bool _shouldMarshalToUIThread; + private readonly Type messageType; + + //private readonly MethodInfo method; + private bool shouldMarshalToUiThread; /// - /// Initializes a new instance of the class. + /// Initializes a new instance of the class. + /// + public Handler() + { + this.messageType = typeof(T); + } + + /// + /// Initializes a new instance of the class. /// - /// Type of the message. - /// The method. - public Handler(Type messageType, MethodInfo method) + /// true if invocation should be performed on UI thread + public void Initialize(bool shouldMarshalToUi) { - _messageType = messageType; - _method = method; - _shouldMarshalToUIThread = method.HasAttribute(); + this.shouldMarshalToUiThread = shouldMarshalToUi; } /// @@ -226,7 +280,7 @@ public Handler(Type messageType, MethodInfo method) /// public bool CanHandle(Type messageType) { - return _messageType.IsAssignableFrom(messageType); + return this.messageType.IsAssignableFrom(messageType); } /// @@ -236,11 +290,15 @@ public bool CanHandle(Type messageType) /// The message. public void Invoke(object target, object message) { - void Method() => _method.Invoke(target, new[] { message }); + void Method() + { + var actualHandler = (IHandle)target; + actualHandler.Handle((T)message); + } - if (_shouldMarshalToUIThread && _uiThreadMarshaller != null) + if (shouldMarshalToUiThread && uiThreadMarshaller != null) { - _uiThreadMarshaller.Invoke(Method); + uiThreadMarshaller.Invoke(Method); return; } @@ -249,4 +307,11 @@ public void Invoke(object target, object message) } } } -} \ No newline at end of file + + internal interface IHandler + { + void Initialize(bool shouldMarshalToUiThread); + bool CanHandle(Type messageType); + void Invoke(object target, object message); + } +} diff --git a/test/Hellang.MessageBus.Tests/MessageBusTests.cs b/test/Hellang.MessageBus.Tests/MessageBusTests.cs index 5d9f4f0..9dcd8bc 100644 --- a/test/Hellang.MessageBus.Tests/MessageBusTests.cs +++ b/test/Hellang.MessageBus.Tests/MessageBusTests.cs @@ -172,8 +172,19 @@ public void MessagesAreHandledByExplicitSubscriber() { bus.Publish(); + Assert.That(target.MessageHandleCount, Is.EqualTo(1)); + } + [Test] + public void MessagesAreHandledWhenPublishedAsObjectGenericType() { + var bus = new DirectDispatchMessageBus(); + + var target = new ExplicitSubscriber(); + bus.Subscribe(target); + + bus.Publish(new TestMessage()); + Assert.That(target.MessageHandleCount, Is.EqualTo(1)); } } -} \ No newline at end of file +} diff --git a/test/Hellang.MessageBus.Tests/TestObjects/Subscribers/UIThreadSingleSubscriber.cs b/test/Hellang.MessageBus.Tests/TestObjects/Subscribers/UIThreadSingleSubscriber.cs index f8ac078..88662d6 100644 --- a/test/Hellang.MessageBus.Tests/TestObjects/Subscribers/UIThreadSingleSubscriber.cs +++ b/test/Hellang.MessageBus.Tests/TestObjects/Subscribers/UIThreadSingleSubscriber.cs @@ -1,10 +1,11 @@ using Hellang.MessageBus.Tests.TestObjects.Messages; +using Hellang.MessageBus; namespace Hellang.MessageBus.Tests.TestObjects.Subscribers { public class UIThreadSingleSubscriber : SingleSubscriber { - [HandleOnUIThread] + [HandleOnUiThreadAttribute] public override void Handle(TestMessage message) { base.Handle(message);