Improve thread safe Utils methods for incrementing identifiers#3221
Conversation
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
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.
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