-
Notifications
You must be signed in to change notification settings - Fork 1k
Improve thread safe Utils methods for incrementing identifiers #3221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve thread safe Utils methods for incrementing identifiers #3221
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves thread safety for identifier incrementation methods in the OPC UA .NET Standard stack by addressing wraparound behavior and ensuring no duplicate or zero IDs are generated.
- Updates utility methods to use atomic operations with proper type handling (
uintinstead oflong) - Implements safer increment logic that prevents generation of zero values during wraparound
- Adds comprehensive concurrency testing for wraparound scenarios in
MonitoredItemIdFactory
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
Tests/Opc.Ua.Server.Tests/MonitoredItemIdFactoryTests.cs |
Adds test for concurrent ID generation during wraparound scenarios |
Stack/Opc.Ua.Core/Types/Utils/Utils.cs |
Updates identifier utility methods with thread-safe atomic operations and zero-prevention logic |
| Multiple server/client files | Changes identifier fields from long to uint for consistency with new utility methods |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| } while (exchangedValue != value); | ||
| return (uint)Interlocked.Read(ref identifier); | ||
| return value; |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method should return the final value of the identifier after the operation, but it returns the old value instead of reading the current identifier value. This could return stale data if another thread modified the identifier between the last read and the return statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation needs to change, no matter, no one is using the retur value
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3221 +/- ##
==========================================
+ Coverage 57.62% 57.65% +0.03%
==========================================
Files 355 355
Lines 78679 78688 +9
Branches 13857 13860 +3
==========================================
+ Hits 45335 45366 +31
+ Misses 29092 29069 -23
- Partials 4252 4253 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
| } while (exchangedValue != value); | ||
| return (uint)Interlocked.Read(ref identifier); | ||
| return value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation needs to change, no matter, no one is using the retur value
Proposed changes
Thanks to @marcschier for bringing this up.
Types of changes
Checklist