fix: detach master connection closed event handler (#135)#217
Open
JoshuaVlantis wants to merge 3 commits into
Open
fix: detach master connection closed event handler (#135)#217JoshuaVlantis wants to merge 3 commits into
JoshuaVlantis 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.
Flundrahn
reviewed
May 22, 2026
Drop the target framework change from this PR as requested in review. Updating and consolidating target frameworks is tracked separately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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