fix: detach master connection closed event handler (#135)#217
fix: detach master connection closed event handler (#135)#217JoshuaVlantis wants to merge 3 commits into
Conversation
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
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.
Drop the target framework change from this PR as requested in review. Updating and consolidating target frameworks is tracked separately.
|
Continuing review now and diving into it properly! I'm fresh to this project but would be fun to contribute at bit. All tests pass and your solution for the issue make sense to me. One thought, should we consolidate this logic with the one on line 181 to 189? On those lines we only remove the handler if it successfully removes the dictionary entry. Can use a helper method, or duplication is fine too IMO. With helper method it would be: private void OnMasterConnectionClosedHandler(object sender, TcpConnectionEventArgs e)
{
RemoveAndDisposeMasterConnection(e.EndPoint);
}
private void RemoveAndDisposeMasterConnection(string endpointKey)
{
if (!_masters.TryRemove(endpointKey, out var connection))
{
Logger.Warning($"EndPoint {endpointKey} cannot be removed, it does not exist.");
return;
}
connection.ModbusMasterTcpConnectionClosed -= OnMasterConnectionClosedHandler;
connection.Dispose();
Logger.Information($"Removed Master {endpointKey}");
}We would get some nice logging in both cases, it leaves the risk that the even fires duplicates times but it will no longer throw and the logging may be useful if we ever suspect unwanted duplicate triggering of the event. Also in suggestion removed comment with issue reference from the code, since it does not look to be a convention used in this library. The reference to specific issue from tests is nice though, I would keep it. Let me know what you think. |
Summary
Closes #135.
ModbusTcpSlaveNetwork.OnMasterConnectionClosedHandlerremoved the connection from the masters dictionary and disposed it, but never unsubscribed its own delegate from the connection'sModbusMasterTcpConnectionClosedevent. It also threwArgumentExceptionfrom inside an event invocation when the endpoint had already been removed (e.g. by a concurrent dispose path), which is hostile to the event source thread.This change detaches the handler from
senderbefore disposing and logs a warning instead of throwing when the master is already gone.It also multi-targets
NModbus.UnitTestsatnet6.0;net8.0(in addition to the existingnet4.6) so the suite can run on Linux .NET runtimes.Test plan
dotnet test NModbus.UnitTests/NModbus.UnitTests.csproj -f net8.0 -c Releasepasses after the fixOnMasterConnectionClosedHandler_UnknownEndPoint_DoesNotThrowtest (reflects the private handler) confirms the no-throw behaviorClosingClientConnection_DetachesEventAndRemovesFromMasterstest uses a realTcpListener/TcpClientand verifies via reflection that the event field on the connection is null after close