diff --git a/NModbus.UnitTests/Device/ModbusTcpSlaveNetworkFixture.cs b/NModbus.UnitTests/Device/ModbusTcpSlaveNetworkFixture.cs new file mode 100644 index 0000000..d5afbab --- /dev/null +++ b/NModbus.UnitTests/Device/ModbusTcpSlaveNetworkFixture.cs @@ -0,0 +1,153 @@ +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/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();