From 292935d14771367e7fb66a5d2e8887b53058145a Mon Sep 17 00:00:00 2001 From: Joshua Vlantis Date: Tue, 12 May 2026 12:20:54 +0200 Subject: [PATCH 1/3] fix: detach master connection closed event handler (#135) ModbusTcpSlaveNetwork.OnMasterConnectionClosedHandler never unsubscribed its own delegate from the closing ModbusMasterTcpConnection's ModbusMasterTcpConnectionClosed event, and it threw ArgumentException from inside an event invocation if the endpoint had already been removed from the masters dictionary (e.g. by a concurrent dispose path). Throwing from an event sink is hostile to the source thread. Detach the handler from the connection before disposing and log a warning instead of throwing when the master is already gone. Also multi-target NModbus.UnitTests at net6.0 and net8.0 so the test suite can run on Linux .NET runtimes alongside the existing net4.6 target. Tests: * OnMasterConnectionClosedHandler_UnknownEndPoint_DoesNotThrow * ClosingClientConnection_DetachesEventAndRemovesFromMasters --- .../Device/ModbusTcpSlaveNetworkFixture.cs | 151 ++++++++++++++++++ NModbus.UnitTests/NModbus.UnitTests.csproj | 3 +- NModbus/Device/ModbusTcpSlaveNetwork.cs | 15 +- 3 files changed, 163 insertions(+), 6 deletions(-) create mode 100644 NModbus.UnitTests/Device/ModbusTcpSlaveNetworkFixture.cs diff --git a/NModbus.UnitTests/Device/ModbusTcpSlaveNetworkFixture.cs b/NModbus.UnitTests/Device/ModbusTcpSlaveNetworkFixture.cs new file mode 100644 index 0000000..c1e724d --- /dev/null +++ b/NModbus.UnitTests/Device/ModbusTcpSlaveNetworkFixture.cs @@ -0,0 +1,151 @@ +using System; +using System.Collections; +using System.Net; +using System.Net.Sockets; +using System.Reflection; +using System.Threading; +using System.Threading.Tasks; +using NModbus.Device; +using NModbus.Logging; +using Xunit; + +namespace NModbus.UnitTests.Device +{ + /// + /// Regression coverage for issue #135: when a master TCP connection closed, + /// ModbusTcpSlaveNetwork.OnMasterConnectionClosedHandler tore the + /// connection out of the masters dictionary and disposed it, but never + /// unsubscribed its own delegate from the connection's + /// ModbusMasterTcpConnectionClosed event. The handler also threw + /// from inside an event invocation if the + /// endpoint had already been removed (e.g. by a concurrent dispose path), + /// which is unsafe behavior for an event sink. + /// + public class ModbusTcpSlaveNetworkFixture + { + private static IModbusFactory Factory => new ModbusFactory(); + + [Fact] + public void OnMasterConnectionClosedHandler_UnknownEndPoint_DoesNotThrow() + { + using var listener = new ListenerHandle(); + using var network = new ModbusTcpSlaveNetwork(listener.Listener, Factory, NullModbusLogger.Instance); + + // Invoke the private handler with an endpoint that was never registered. + // Before the fix this threw ArgumentException; after the fix it logs and returns. + MethodInfo handler = typeof(ModbusTcpSlaveNetwork).GetMethod( + "OnMasterConnectionClosedHandler", + BindingFlags.Instance | BindingFlags.NonPublic); + Assert.NotNull(handler); + + object eventArgs = new TcpConnectionEventArgs("127.0.0.1:0"); + + Exception captured = null; + try + { + handler.Invoke(network, new[] { (object)null, eventArgs }); + } + catch (TargetInvocationException tie) + { + captured = tie.InnerException; + } + + Assert.Null(captured); + } + + [Fact] + public async Task ClosingClientConnection_DetachesEventAndRemovesFromMasters() + { + using var listener = new ListenerHandle(); + using var network = new ModbusTcpSlaveNetwork(listener.Listener, Factory, NullModbusLogger.Instance); + + using var cts = new CancellationTokenSource(); + Task listenTask = network.ListenAsync(cts.Token); + + using (var client = new TcpClient()) + { + await client.ConnectAsync(IPAddress.Loopback, listener.Port).ConfigureAwait(false); + + // Wait until the slave network registers the connection. + IDictionary masters = await WaitForMasterCountAsync(network, expected: 1).ConfigureAwait(false); + Assert.Single(masters); + + object connection = FirstValue(masters); + EventHandler beforeSubscribers = GetClosedEventSubscribers(connection); + Assert.NotNull(beforeSubscribers); + Assert.Single(beforeSubscribers.GetInvocationList()); + + // Closing the client triggers HandleRequestAsync's 0-byte path which fires the event. + client.Close(); + + masters = await WaitForMasterCountAsync(network, expected: 0).ConfigureAwait(false); + Assert.Empty(masters); + + EventHandler afterSubscribers = GetClosedEventSubscribers(connection); + Assert.Null(afterSubscribers); + } + + cts.Cancel(); + try { await listenTask.ConfigureAwait(false); } + catch (OperationCanceledException) { /* expected */ } + catch (SocketException) { /* AcceptTcpClientAsync may surface this when the listener is stopped */ } + } + + private static async Task WaitForMasterCountAsync(ModbusTcpSlaveNetwork network, int expected) + { + FieldInfo field = typeof(ModbusTcpSlaveNetwork).GetField( + "_masters", + BindingFlags.Instance | BindingFlags.NonPublic); + var masters = (IDictionary)field.GetValue(network); + + for (int i = 0; i < 50; i++) + { + if (masters.Count == expected) + { + return masters; + } + + await Task.Delay(100).ConfigureAwait(false); + } + + return masters; + } + + private static object FirstValue(IDictionary masters) + { + foreach (object value in masters.Values) + { + return value; + } + + return null; + } + + private static EventHandler GetClosedEventSubscribers(object connection) + { + FieldInfo field = connection.GetType().GetField( + "ModbusMasterTcpConnectionClosed", + BindingFlags.Instance | BindingFlags.NonPublic); + return (EventHandler)field.GetValue(connection); + } + + private sealed class ListenerHandle : IDisposable + { + public ListenerHandle() + { + Listener = new TcpListener(IPAddress.Loopback, 0); + Listener.Start(); + Port = ((IPEndPoint)Listener.LocalEndpoint).Port; + } + + public TcpListener Listener { get; } + + public int Port { get; } + + public void Dispose() + { + try { Listener.Stop(); } catch { /* ignore */ } + } + } + } +} diff --git a/NModbus.UnitTests/NModbus.UnitTests.csproj b/NModbus.UnitTests/NModbus.UnitTests.csproj index 77fb2fa..223ec4f 100644 --- a/NModbus.UnitTests/NModbus.UnitTests.csproj +++ b/NModbus.UnitTests/NModbus.UnitTests.csproj @@ -1,12 +1,11 @@ - net4.6 + net4.6;net6.0;net8.0 true NModbus.UnitTests NModbus.UnitTests true - 1.0.4 false diff --git a/NModbus/Device/ModbusTcpSlaveNetwork.cs b/NModbus/Device/ModbusTcpSlaveNetwork.cs index 7e7c221..81128cb 100644 --- a/NModbus/Device/ModbusTcpSlaveNetwork.cs +++ b/NModbus/Device/ModbusTcpSlaveNetwork.cs @@ -214,12 +214,19 @@ private void OnTimer(object sender, ElapsedEventArgs e) #endif private void OnMasterConnectionClosedHandler(object sender, TcpConnectionEventArgs e) { - ModbusMasterTcpConnection connection; + // Issue #135: detach our handler from the connection so the network + // doesn't keep getting called back after the connection is gone, and + // so that a re-entrant or double-fired close event can't crash an + // event-source thread with ArgumentException. + if (sender is ModbusMasterTcpConnection senderConnection) + { + senderConnection.ModbusMasterTcpConnectionClosed -= OnMasterConnectionClosedHandler; + } - if (!_masters.TryRemove(e.EndPoint, out connection)) + if (!_masters.TryRemove(e.EndPoint, out ModbusMasterTcpConnection connection)) { - string msg = $"EndPoint {e.EndPoint} cannot be removed, it does not exist."; - throw new ArgumentException(msg); + Logger.Warning($"EndPoint {e.EndPoint} cannot be removed, it does not exist."); + return; } connection.Dispose(); From ffd7d231a5179f8310c390ae283d9378ec4a05ef Mon Sep 17 00:00:00 2001 From: Joshua Vlantis Date: Tue, 12 May 2026 12:29:23 +0200 Subject: [PATCH 2/3] fix(tests): use classic using blocks for C# 7.3 compatibility The net4.6 target in NModbus.UnitTests uses C# 7.3, which does not support 'using declarations' (CS8370). Rewrite the test fixture to use classic using-statement blocks so the project builds under net4.6 in addition to net6.0 and net8.0. --- .../Device/ModbusTcpSlaveNetworkFixture.cs | 90 ++++++++++--------- 1 file changed, 46 insertions(+), 44 deletions(-) diff --git a/NModbus.UnitTests/Device/ModbusTcpSlaveNetworkFixture.cs b/NModbus.UnitTests/Device/ModbusTcpSlaveNetworkFixture.cs index c1e724d..d5afbab 100644 --- a/NModbus.UnitTests/Device/ModbusTcpSlaveNetworkFixture.cs +++ b/NModbus.UnitTests/Device/ModbusTcpSlaveNetworkFixture.cs @@ -28,67 +28,69 @@ public class ModbusTcpSlaveNetworkFixture [Fact] public void OnMasterConnectionClosedHandler_UnknownEndPoint_DoesNotThrow() { - using var listener = new ListenerHandle(); - using var network = new ModbusTcpSlaveNetwork(listener.Listener, Factory, NullModbusLogger.Instance); + using (var listener = new ListenerHandle()) + using (var network = new ModbusTcpSlaveNetwork(listener.Listener, Factory, NullModbusLogger.Instance)) + { + // Invoke the private handler with an endpoint that was never registered. + // Before the fix this threw ArgumentException; after the fix it logs and returns. + MethodInfo handler = typeof(ModbusTcpSlaveNetwork).GetMethod( + "OnMasterConnectionClosedHandler", + BindingFlags.Instance | BindingFlags.NonPublic); + Assert.NotNull(handler); - // Invoke the private handler with an endpoint that was never registered. - // Before the fix this threw ArgumentException; after the fix it logs and returns. - MethodInfo handler = typeof(ModbusTcpSlaveNetwork).GetMethod( - "OnMasterConnectionClosedHandler", - BindingFlags.Instance | BindingFlags.NonPublic); - Assert.NotNull(handler); + object eventArgs = new TcpConnectionEventArgs("127.0.0.1:0"); - object eventArgs = new TcpConnectionEventArgs("127.0.0.1:0"); + Exception captured = null; + try + { + handler.Invoke(network, new[] { (object)null, eventArgs }); + } + catch (TargetInvocationException tie) + { + captured = tie.InnerException; + } - Exception captured = null; - try - { - handler.Invoke(network, new[] { (object)null, eventArgs }); + Assert.Null(captured); } - catch (TargetInvocationException tie) - { - captured = tie.InnerException; - } - - Assert.Null(captured); } [Fact] public async Task ClosingClientConnection_DetachesEventAndRemovesFromMasters() { - using var listener = new ListenerHandle(); - using var network = new ModbusTcpSlaveNetwork(listener.Listener, Factory, NullModbusLogger.Instance); + using (var listener = new ListenerHandle()) + using (var network = new ModbusTcpSlaveNetwork(listener.Listener, Factory, NullModbusLogger.Instance)) + using (var cts = new CancellationTokenSource()) + { + Task listenTask = network.ListenAsync(cts.Token); - using var cts = new CancellationTokenSource(); - Task listenTask = network.ListenAsync(cts.Token); + using (var client = new TcpClient()) + { + await client.ConnectAsync(IPAddress.Loopback, listener.Port).ConfigureAwait(false); - using (var client = new TcpClient()) - { - await client.ConnectAsync(IPAddress.Loopback, listener.Port).ConfigureAwait(false); + // Wait until the slave network registers the connection. + IDictionary masters = await WaitForMasterCountAsync(network, expected: 1).ConfigureAwait(false); + Assert.Single(masters); - // Wait until the slave network registers the connection. - IDictionary masters = await WaitForMasterCountAsync(network, expected: 1).ConfigureAwait(false); - Assert.Single(masters); + object connection = FirstValue(masters); + EventHandler beforeSubscribers = GetClosedEventSubscribers(connection); + Assert.NotNull(beforeSubscribers); + Assert.Single(beforeSubscribers.GetInvocationList()); - object connection = FirstValue(masters); - EventHandler beforeSubscribers = GetClosedEventSubscribers(connection); - Assert.NotNull(beforeSubscribers); - Assert.Single(beforeSubscribers.GetInvocationList()); + // Closing the client triggers HandleRequestAsync's 0-byte path which fires the event. + client.Close(); - // Closing the client triggers HandleRequestAsync's 0-byte path which fires the event. - client.Close(); + masters = await WaitForMasterCountAsync(network, expected: 0).ConfigureAwait(false); + Assert.Empty(masters); - masters = await WaitForMasterCountAsync(network, expected: 0).ConfigureAwait(false); - Assert.Empty(masters); + EventHandler afterSubscribers = GetClosedEventSubscribers(connection); + Assert.Null(afterSubscribers); + } - EventHandler afterSubscribers = GetClosedEventSubscribers(connection); - Assert.Null(afterSubscribers); + cts.Cancel(); + try { await listenTask.ConfigureAwait(false); } + catch (OperationCanceledException) { /* expected */ } + catch (SocketException) { /* AcceptTcpClientAsync may surface this when the listener is stopped */ } } - - cts.Cancel(); - try { await listenTask.ConfigureAwait(false); } - catch (OperationCanceledException) { /* expected */ } - catch (SocketException) { /* AcceptTcpClientAsync may surface this when the listener is stopped */ } } private static async Task WaitForMasterCountAsync(ModbusTcpSlaveNetwork network, int expected) From 74ba57aeb37bd79ff4f52f1ee8132e1bf3bec410 Mon Sep 17 00:00:00 2001 From: JoshuaVlantis Date: Wed, 10 Jun 2026 09:02:22 +0200 Subject: [PATCH 3/3] chore: keep test project on net4.6 only Drop the target framework change from this PR as requested in review. Updating and consolidating target frameworks is tracked separately. --- NModbus.UnitTests/NModbus.UnitTests.csproj | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/NModbus.UnitTests/NModbus.UnitTests.csproj b/NModbus.UnitTests/NModbus.UnitTests.csproj index 223ec4f..77fb2fa 100644 --- a/NModbus.UnitTests/NModbus.UnitTests.csproj +++ b/NModbus.UnitTests/NModbus.UnitTests.csproj @@ -1,11 +1,12 @@ - net4.6;net6.0;net8.0 + net4.6 true NModbus.UnitTests NModbus.UnitTests true + 1.0.4 false