Skip to content

[Server] Improve Request Queue, fix Event handling, add Tests - #3611

Merged
marcschier merged 21 commits into
masterfrom
romanett/EventFixes
Mar 20, 2026
Merged

marcschier merged 21 commits into
masterfrom
romanett/EventFixes

Conversation

@romanett

@romanett romanett commented Mar 12, 2026

Copy link
Copy Markdown
Contributor

Proposed changes

  • implement RequestQueue using Channels
  • add event load test
  • add event integration test verifying event from server object is received
  • fix Client Reconnect Test
  • fix DiagnosticsNodeManager not creating RootNotifier for Server object
  • add Benchmarks for MonitoredItem.QueueEvent
  • add serveral UnitTest for DiagnosticsNodeManager

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds functionality)
  • Test enhancement (non-breaking change to increase test coverage)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected, requires version increase of Nuget packages)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc.
  • I have signed the CLA.
  • I ran tests locally with my changes, all passed.
  • I fixed all failing tests in the CI pipelines.
  • I fixed all introduced issues with CodeQL and LGTM.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added necessary documentation (if appropriate).
  • Any dependent changes have been merged and published in downstream modules.

@CLAassistant

CLAassistant commented Mar 12, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ romanett
✅ marcschier
❌ Copilot
You have signed the CLA already but the status is still pending? Let us recheck it.

@romanett romanett changed the title Romanett/event fixes [Server] Improve Request Queue, fix Event handling, add Tests Mar 19, 2026
@romanett
romanett marked this pull request as ready for review March 19, 2026 06:21
@romanett
romanett requested a review from marcschier March 19, 2026 06:21
@marcschier
marcschier merged commit 66f4439 into master Mar 20, 2026
232 of 237 checks passed
@romanett
romanett deleted the romanett/EventFixes branch March 20, 2026 08:25
marcschier added a commit that referenced this pull request Jul 21, 2026
# Description

Fixes a server-side lost wake-up in `SessionPublishQueue` that can
permanently stop monitored-item notifications while the Session and
SecureChannel remain healthy.

In 1.5.378, `PublishAsync` checked for a ready Subscription under
`m_subscriptionPublishLock` and then queued the Publish request under
`m_lock`. A Subscription could become ready between those operations, be
marked `ReadyToPublish` because no request was queued yet, and then
leave the newly queued request stranded. Later timer ticks skipped
assignment because the Subscription was already marked ready.

This change backports the relevant `SessionPublishQueue` correction from
#3611 and incorporates the review follow-up:

- Makes the ready-Subscription check and Publish-request enqueue atomic
under `m_lock`.
- Synchronizes `PublishCompleted`, `Requeue`, and timer-driven
assignment with the same lock.
- Removes `m_subscriptionPublishLock`.
- Invokes `SessionClosed` callbacks outside the queue lock.
- Retries timer-driven assignment for already-ready, non-publishing
Subscriptions instead of skipping them.
- Adds deterministic regression tests for the lost-wakeup and requeue
paths.

## Related Issues

- Fixes #3997
- Backports the relevant queue fix from #3611

## Testing

- The focused `SessionPublishQueueRaceTests` fixture passes on net472,
net48, net8.0, net9.0, and net10.0.
- Earlier branch validation ran the full `UA.slnx` on net10.0 and net48
with zero failures.
- Three pre-existing Quickstarts CA1823 warnings remain unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants