Fix reverse connect hold time when several Servers share one listener - #4342
Conversation
All reverse connections which are held by the ReverseConnectManager share a single cancellation token source, so registering a waiting connection wakes up every held connection. The hold loop stopped after a single re-match attempt, which rejected the connections of all other Servers even though most of their own hold time was left. The listener then force faulted those sockets and WaitForConnectionAsync for those Servers timed out, which is why a separate listener port per Server was needed. The hold loop now re-arms after a wakeup and keeps the connection for the remainder of its own hold time when the registration which caused the wakeup was for another Server. A committed Stop/Dispose cancels the hold token without renewing it, so a held callback also checks the callback drain before it re-arms the hold. Without that check it would spin on the already cancelled token until the hold time expires and block the drain until then. Relates to #3985 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes reverse-connect hold-time behavior when multiple Servers share a single listener by ensuring held callbacks re-check registrations after wakeups (instead of dropping the connection after a single wake). This addresses the multi-server timeout symptom reported in #3985 and preserves the existing stop/dispose drain guarantees.
Changes:
- Rework
ReverseConnectManager.OnConnectionWaitingAsynchold loop to re-arm waiting after unrelated registrations wake all held callbacks, and add a drain-pending guard. - Add regression test covering two held callbacks where registering/waiting for one Server must not release the other.
- Document
HoldTimesemantics for shared-listener scenarios indocs/ReverseConnect.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Opc.Ua.Client/ReverseConnectManager.cs | Fix hold-loop behavior to keep unmatched reverse connections held for their remaining hold time; add drain-pending detection. |
| tests/Opc.Ua.Client.Tests/ClientBuilder/ReverseConnectManagerLifecycleTests.cs | Add regression test proving a held connection survives a registration for a different Server. |
| docs/ReverseConnect.md | Document HoldTime semantics and stop/dispose behavior for held reverse connections. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code coverage✅ Coverage gate passed.
Uncovered changed lines
Coverage is above the recorded baseline - consider ratcheting Thresholds live in |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4342 +/- ##
==========================================
- Coverage 80.21% 80.20% -0.01%
==========================================
Files 2014 2014
Lines 275050 275058 +8
Branches 47852 47854 +2
==========================================
- Hits 220625 220615 -10
- Misses 37507 37516 +9
- Partials 16918 16927 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…#4341) # Description Fixes the reverse connect problem reported in #3985: a Client which uses one reverse connect listener port for several Servers connects to the first Server, and every following `WaitForConnectionAsync` times out. A separate listener port per Server was the only workaround. ## Root cause Confirming the analysis in [mrsuciu's comment on #3985](#3985): - Servers which send `ReverseHello` before the application registered a waiting connection for them are held in `OnConnectionWaitingAsync` for `HoldTime`. - All held connections share a single `CancellationTokenSource`, so `RegisterWaitingConnection` wakes up **every** held connection, not only the one it was registered for. - The hold loop was `while (!matched) { ... break; }` with an unconditional `break`, so each woken connection got exactly one re-match attempt. The connections which did not match the new registration fell out of the loop unaccepted, although most of their own hold time was left. - An unaccepted connection is force faulted by the listener (`TcpReverseConnectChannel`, "The reverse connection was rejected by the client"), so that Server is dropped and only returns on its next `ReverseHello` interval, long after the Client timed out waiting for it. Separate ports avoid the shared wakeup path, which is exactly why that workaround succeeds. ## Changes - `ReverseConnectManager.OnConnectionWaitingAsync` re-arms the hold after a wakeup: it re-matches, and if the registration which caused the wakeup was for another Server the connection keeps waiting for the remainder of its own hold time. The loop is still bounded by `HoldTime`, so a Server which is never registered for is rejected exactly as before. - A shutdown flag set by `StopService`/`Dispose` (cleared by `StartService`) releases held connections immediately during teardown, instead of parking them until the hold time expires. `Dispose` now also cancels the token source before disposing it, so a held callback can no longer read a disposed token. - `Docs/ReverseConnect.md`: new "Sharing a listener across multiple Servers" section documenting that the listener port staying in `LISTENING` is expected, that one shared `ReverseConnectManager` should serve all Servers on the same Client Url, and the `HoldTime`/`WaitTimeout` semantics. No public API, signature or serialization change. ## Validation - New `ReverseConnectManagerUnitTests` (3 tests): the two Server regression, hold time expiry still rejecting, and dispose releasing a held connection. They drive the callback through an internal test hook, so no listener or socket is involved. Verified that the regression test fails against the previous behaviour with "The reverse connection of the second server was released before its hold time expired." - New tests pass on net10.0 and net48. - Full reverse connect suite (`FullyQualifiedName~ReverseConnect`, including the existing `ReverseConnectTest` server fixture): 33/33 pass on net10.0. ## Related Issues - Relates to #3985. - `master` carries the identical defect: the same `break` survived #4009 and #4059 and is still present in `src/Opc.Ua.Client/ReverseConnectManager.cs`. The equivalent fix for `master` is #4342 (it uses `TimeProvider` and breaks on a pending callback drain instead of a new flag), so the issue is intentionally not auto closed here. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Description
Fixes the reverse connect problem reported in #3985 on
master. #4009 and #4059 addressed the listener startup diagnostics and the async lifecycle, but the actual hold time defect described in mrsuciu's comment on the issue survived both refactors unchanged.Symptom: a Client which uses one reverse connect listener port for several Servers connects to the first Server, and every following
WaitForConnectionAsynctimes out. A separate listener port per Server was the only workaround.Root cause
ReverseHellobefore the application registered a waiting connection for them are held inOnConnectionWaitingAsyncforHoldTime.CancellationTokenSource, soRegisterWaitingConnection/WaitForConnectionAsyncwakes up every held connection, not only the one it was registered for.while (!matched) { ... break; }with an unconditionalbreak, so each woken connection got exactly one re-match attempt. The connections which did not match the new registration fell out of the loop unaccepted, although most of their own hold time was left.TcpReverseConnectChannel, "The reverse connection was rejected by the client"), so that Server is dropped and only returns on its nextReverseHellointerval, long after the Client timed out waiting for it.Separate ports avoid the shared wakeup path, which is exactly why that workaround succeeds.
Changes
OnConnectionWaitingAsyncre-arms the hold after a wakeup: it re-matches, and if the registration which caused the wakeup was for another Server the connection keeps waiting for the remainder of its own hold time. The loop is still bounded byHoldTime, so a Server which is never registered for is rejected exactly as before.IsCallbackDrainPending()check before the hold is re-armed.DrainConnectionCallbacksAsynccancels the hold token without renewing it, so a re-armed hold would spin on the already cancelled token until the hold time expires and would block the drain until then. With the check a held callback releases its transport immediately, which keeps the existingStopDrainsHeldConnectionCallbackBeforeListenerCloseandDisposeDrainsHeldConnectionCallbackBeforeListenerDisposalguarantees intact.docs/ReverseConnect.md: document theHoldTimesemantics in the "Sharing a listener across multiple Servers" section.No public API change.
Validation
HeldConnectionSurvivesRegistrationForAnotherServertest inReverseConnectManagerLifecycleTestsdrives two held callbacks throughInvokeConnectionWaitingForTestand proves the second Server keeps its connection when the first Server is registered. Verified that it fails against the previous behaviour with "the reverse connection of the second Server was released before its hold time expired".FullyQualifiedName~ReverseConnect(183 tests, includingReverseConnectManagerTests,ReverseConnectManagerLifecycleTestsandReverseConnectHostTests): all pass on net10.0 and net48.Related Issues
master378).Checklist
🤖 Generated with Claude Code