Fix unsubscribe from LogEntriesChanged event handler (#872)

* Fix unsubscribe from LogEntriesChanged event handler

* .

* f

examples/WireMock.Net.Console.Proxy.Net452/Program.cs

@@ -1,6 +1,6 @@
-using System;
+using System;
+using System.Collections.Specialized;
 using System.Net.Http;
-using Newtonsoft.Json;
 using WireMock.Server;
 using WireMock.Settings;
 
@@ -26,10 +26,8 @@ namespace WireMock.Net.Console.Proxy.Net452
                 }
             });
 
-            server.LogEntriesChanged += (sender, eventRecordArgs) =>
+            System.Console.WriteLine("Subscribing to LogEntriesChanged");
-            {
+            server.LogEntriesChanged += Server_LogEntriesChanged;
-                System.Console.WriteLine(JsonConvert.SerializeObject(eventRecordArgs.NewItems, Formatting.Indented));
-            };
 
             var uri = new Uri(urls[0]);
             var form = new MultipartFormDataContent
@@ -38,9 +36,23 @@ namespace WireMock.Net.Console.Proxy.Net452
             };
             new HttpClient().PostAsync(uri, form).GetAwaiter().GetResult();
 
+            System.Console.WriteLine("Unsubscribing to LogEntriesChanged");
+            server.LogEntriesChanged -= Server_LogEntriesChanged;
+
+            form = new MultipartFormDataContent
+            {
+                { new StringContent("data2"), "test2", "test2.txt" }
+            };
+            new HttpClient().PostAsync(uri, form).GetAwaiter().GetResult();
+
             System.Console.WriteLine("Press any key to stop the server");
             System.Console.ReadKey();
             server.Stop();
         }
+
+        private static void Server_LogEntriesChanged(object sender, NotifyCollectionChangedEventArgs eventRecordArgs)
+        {
+            System.Console.WriteLine("Server_LogEntriesChanged : {0}", eventRecordArgs.NewItems.Count);
+        }
     }
 }

src/WireMock.Net/Server/WireMockServer.LogEntries.cs

@@ -14,26 +14,12 @@ namespace WireMock.Server;
 
 public partial class WireMockServer
 {
-    /// <inheritdoc cref="IWireMockServer.LogEntriesChanged" />
+    /// <inheritdoc />
     [PublicAPI]
     public event NotifyCollectionChangedEventHandler LogEntriesChanged
     {
-        add
+        add => _logEntriesChanged += value;
-        {
+        remove => _logEntriesChanged -= value;
-            _options.LogEntries.CollectionChanged += (sender, eventRecordArgs) =>
-            {
-                try
-                {
-                    value(sender, eventRecordArgs);
-                }
-                catch (Exception exception)
-                {
-                    _options.Logger.Error("Error calling the LogEntriesChanged event handler: {0}", exception.Message);
-                }
-            };
-        }
-
-        remove => _options.LogEntries.CollectionChanged -= value;
     }
 
     /// <inheritdoc cref="IWireMockServer.LogEntries" />
@@ -90,4 +76,24 @@ public partial class WireMockServer
 
         return false;
     }
+
+    private NotifyCollectionChangedEventHandler? _logEntriesChanged;
+
+    private void LogEntries_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
+    {
+        if (_logEntriesChanged is { })
+        {
+            foreach (var handler in _logEntriesChanged.GetInvocationList())
+            {
+                try
+                {
+                    handler.DynamicInvoke(this, e);
+                }
+                catch (Exception exception)
+                {
+                    _options.Logger.Error("Error calling the LogEntriesChanged event handler: {0}", exception.Message);
+                }
+            }
+        }
+    }
 }

src/WireMock.Net/Server/WireMockServer.cs

@@ -90,6 +90,8 @@ public partial class WireMockServer : IWireMockServer
     /// </summary>
     public void Dispose()
     {
+        _options.LogEntries.CollectionChanged -= LogEntries_CollectionChanged;
+
         Dispose(true);
         GC.SuppressFinalize(this);
     }
@@ -308,6 +310,8 @@ public partial class WireMockServer : IWireMockServer
 
         WireMockMiddlewareOptionsHelper.InitFromSettings(settings, _options);
 
+        _options.LogEntries.CollectionChanged += LogEntries_CollectionChanged;
+
         _matcherMapper = new MatcherMapper(_settings);
         _mappingConverter = new MappingConverter(_matcherMapper);
         _mappingToFileSaver = new MappingToFileSaver(_settings, _mappingConverter);

test/WireMock.Net.Tests/ObservableLogEntriesTest.cs

@@ -1,10 +1,12 @@
 using System;
 using System.Collections.Generic;
+using System.Collections.Specialized;
 using System.Linq;
 using System.Net;
 using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
+using FluentAssertions;
 using Moq;
 using NFluent;
 using WireMock.Logging;
@@ -14,101 +16,142 @@ using WireMock.Server;
 using WireMock.Settings;
 using Xunit;
 
-namespace WireMock.Net.Tests
+namespace WireMock.Net.Tests;
+
+public class ObservableLogEntriesTest
 {
-    public class ObservableLogEntriesTest
+    [Fact]
+    public async Task WireMockServer_LogEntriesChanged_WithException_Should_LogError()
     {
-        [Fact]
+        // Assign
-        public async Task WireMockServer_LogEntriesChanged_WithException_Should_LogError()
+        string path = $"/log_{Guid.NewGuid()}";
+        var loggerMock = new Mock<IWireMockLogger>();
+        loggerMock.Setup(l => l.Error(It.IsAny<string>(), It.IsAny<object[]>()));
+        var settings = new WireMockServerSettings
         {
-            // Assign
+            Logger = loggerMock.Object
-            string path = $"/log_{Guid.NewGuid()}";
+        };
-            var loggerMock = new Mock<IWireMockLogger>();
+        var server = WireMockServer.Start(settings);
-            loggerMock.Setup(l => l.Error(It.IsAny<string>(), It.IsAny<object[]>()));
-            var settings = new WireMockServerSettings
-            {
-                Logger = loggerMock.Object
-            };
-            var server = WireMockServer.Start(settings);
 
-            server
+        server
-                .Given(Request.Create()
+            .Given(Request.Create()
-                    .WithPath(path)
+                .WithPath(path)
-                    .UsingGet())
+                .UsingGet())
-                .RespondWith(Response.Create()
+            .RespondWith(Response.Create()
-                    .WithBody(@"{ msg: ""Hello world!""}"));
+                .WithBody(@"{ msg: ""Hello world!""}"));
 
-            server.LogEntriesChanged += (sender, args) => throw new Exception();
+        server.LogEntriesChanged += (sender, args) => throw new Exception();
 
-            // Act
+        // Act
-            await new HttpClient().GetAsync($"http://localhost:{server.Ports[0]}{path}").ConfigureAwait(false);
+        await new HttpClient().GetAsync($"http://localhost:{server.Ports[0]}{path}").ConfigureAwait(false);
 
-            // Assert
+        // Assert
-            loggerMock.Verify(l => l.Error(It.IsAny<string>(), It.IsAny<object[]>()), Times.Once);
+        loggerMock.Verify(l => l.Error(It.IsAny<string>(), It.IsAny<object[]>()), Times.Once);
-        }
+    }
 
-        [Fact]
+    [Fact]
-        public async Task WireMockServer_LogEntriesChanged()
+    public async Task WireMockServer_LogEntriesChanged()
+    {
+        // Assign
+        string path = $"/log_{Guid.NewGuid()}";
+        var server = WireMockServer.Start();
+
+        server
+            .Given(Request.Create()
+                .WithPath(path)
+                .UsingGet())
+            .RespondWith(Response.Create()
+                .WithBody(@"{ msg: ""Hello world!""}"));
+
+        int count = 0;
+        server.LogEntriesChanged += (sender, args) => count++;
+
+        // Act 1a
+        await server.CreateClient().GetAsync(path).ConfigureAwait(false);
+
+        // Act 1b
+        await server.CreateClient().GetAsync(path).ConfigureAwait(false);
+
+        // Assert
+        count.Should().Be(2);
+
+        server.Dispose();
+    }
+
+    [Fact]
+    public async Task WireMockServer_LogEntriesChanged_Add_And_Remove_EventHandler()
+    {
+        // Assign
+        string path = $"/log_{Guid.NewGuid()}";
+        var server = WireMockServer.Start();
+
+        server
+            .Given(Request.Create()
+                .WithPath(path)
+                .UsingGet())
+            .RespondWith(Response.Create()
+                .WithBody(@"{ msg: ""Hello world!""}"));
+
+        int count = 0;
+
+        void OnServerOnLogEntriesChanged(object sender, NotifyCollectionChangedEventArgs args) => count++;
+
+        // Add Handler
+        server.LogEntriesChanged += OnServerOnLogEntriesChanged;
+
+        // Act 1
+        await server.CreateClient().GetAsync(path).ConfigureAwait(false);
+
+        // Assert 1
+        count.Should().Be(1);
+
+        // Remove Handler
+        server.LogEntriesChanged -= OnServerOnLogEntriesChanged;
+
+        // Act 2
+        await server.CreateClient().GetAsync(path).ConfigureAwait(false);
+
+        // Assert 2
+        count.Should().Be(1);
+
+        server.Dispose();
+    }
+
+    [Fact]
+    public async Task WireMockServer_LogEntriesChanged_Parallel()
+    {
+        int expectedCount = 10;
+
+        // Assign
+        string path = $"/log_p_{Guid.NewGuid()}";
+        var server = WireMockServer.Start();
+
+        server
+            .Given(Request.Create()
+                .WithPath(path)
+                .UsingGet())
+            .RespondWith(Response.Create()
+                .WithSuccess());
+
+        int count = 0;
+        server.LogEntriesChanged += (sender, args) => count++;
+
+        var http = new HttpClient();
+
+        // Act
+        var listOfTasks = new List<Task<HttpResponseMessage>>();
+        for (var i = 0; i < expectedCount; i++)
         {
-            // Assign
+            Thread.Sleep(50);
-            string path = $"/log_{Guid.NewGuid()}";
+            listOfTasks.Add(http.GetAsync($"{server.Urls[0]}{path}"));
-            var server = WireMockServer.Start();
-
-            server
-                .Given(Request.Create()
-                    .WithPath(path)
-                    .UsingGet())
-                .RespondWith(Response.Create()
-                    .WithBody(@"{ msg: ""Hello world!""}"));
-
-            int count = 0;
-            server.LogEntriesChanged += (sender, args) => count++;
-
-            // Act
-            await new HttpClient().GetAsync($"http://localhost:{server.Ports[0]}{path}").ConfigureAwait(false);
-
-            // Assert
-            Check.That(count).Equals(1);
-
-            server.Dispose();
         }
+        var responses = await Task.WhenAll(listOfTasks).ConfigureAwait(false);
+        var countResponsesWithStatusNotOk = responses.Count(r => r.StatusCode != HttpStatusCode.OK);
 
-        [Fact]
+        // Assert
-        public async Task WireMockServer_LogEntriesChanged_Parallel()
+        Check.That(countResponsesWithStatusNotOk).Equals(0);
-        {
+        Check.That(count).Equals(expectedCount);
-            int expectedCount = 10;
 
-            // Assign
+        server.Dispose();
-            string path = $"/log_p_{Guid.NewGuid()}";
-            var server = WireMockServer.Start();
-
-            server
-                .Given(Request.Create()
-                    .WithPath(path)
-                    .UsingGet())
-                .RespondWith(Response.Create()
-                    .WithSuccess());
-
-            int count = 0;
-            server.LogEntriesChanged += (sender, args) => count++;
-
-            var http = new HttpClient();
-
-            // Act
-            var listOfTasks = new List<Task<HttpResponseMessage>>();
-            for (var i = 0; i < expectedCount; i++)
-            {
-                Thread.Sleep(50);
-                listOfTasks.Add(http.GetAsync($"{server.Urls[0]}{path}"));
-            }
-            var responses = await Task.WhenAll(listOfTasks).ConfigureAwait(false);
-            var countResponsesWithStatusNotOk = responses.Count(r => r.StatusCode != HttpStatusCode.OK);
-
-            // Assert
-            Check.That(countResponsesWithStatusNotOk).Equals(0);
-            Check.That(count).Equals(expectedCount);
-
-            server.Dispose();
-        }
     }
 }