[Server] ActivateSessionAsync / CreateSessionAsync / FindServersAsync / GetEndpointsAsync / CloseSessionAsync #3225
Conversation
…AsyncNodeManager Refactor MasterNodeManager to allow access to IAsyncNodeManager from NodeManagers & NamespaceManagers Properties
…stead of ref globalIdCounter
…eManagerMIMethods
romanett
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3225 +/- ##
===========================================
- Coverage 57.55% 26.30% -31.26%
===========================================
Files 356 340 -16
Lines 78784 76927 -1857
Branches 13870 13528 -342
===========================================
- Hits 45347 20235 -25112
- Misses 29193 54477 +25284
+ Partials 4244 2215 -2029 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
romanett
changed the title
romanett
changed the title
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors session management interfaces and implementations to support asynchronous operations, replacing synchronous methods with ValueTask-based async counterparts and improving concurrency through SemaphoreSlim instead of traditional locking.
- Refactored session-related interfaces (
ISessionManager,IServerInternal) to use async methods for session operations - Replaced
lockstatements withSemaphoreSlimfor better scalability in concurrent scenarios - Enabled async support for FindServers, GetEndpoints, CreateSession, ActivateSession, and CloseSession operations
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Stack/Opc.Ua.Core/Stack/Server/ServerBase.cs | Converted certificate update handler to async and improved exception handling |
| Stack/Opc.Ua.Core/Opc.Ua.Core.csproj | Removed exclusions for async versions of FindServers, GetEndpoints, CreateSession, ActivateSession, and CloseSession |
| Libraries/Opc.Ua.Server/Session/SessionManager.cs | Refactored session manager to use async methods and SemaphoreSlim for synchronization |
| Libraries/Opc.Ua.Server/Session/ISessionManager.cs | Updated interface with async method signatures and added CreateSessionResult class |
| Libraries/Opc.Ua.Server/Server/StandardServer.cs | Implemented async versions of server operations and replaced Lock with SemaphoreSlim |
| Libraries/Opc.Ua.Server/Server/ServerInternalData.cs | Added async CloseSessionAsync method |
| Libraries/Opc.Ua.Server/Server/IServerInternal.cs | Added CloseSessionAsync method to interface |
| Libraries/Opc.Ua.Gds.Server.Common/GlobalDiscoverySampleServer.cs | Replaced lock with SemaphoreSlim for request validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| { | ||
| return [.. m_sessions.Values]; | ||
| } | ||
| return [.. m_sessions.Values]; |
There was a problem hiding this comment.
This method accesses m_sessions without synchronization while other methods use SemaphoreSlim. This could lead to race conditions. Consider adding semaphore protection or documenting thread-safety expectations.
| return [.. m_sessions.Values]; | |
| m_lock.Wait(); | |
| try | |
| { | |
| return [.. m_sessions.Values]; | |
| } | |
| finally | |
| { | |
| m_lock.Release(); | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
m_sessions is a concurrent dictionary internal snapshots are created on access
| if (securityToken is UserNameIdentityToken) | ||
| { | ||
| lock (Lock) | ||
| SemaphoreSlim.Wait(); |
There was a problem hiding this comment.
I think we should have an abstraction here when we expose the lock, something like ILock, then we have the opportunity to change this later, and dont repeat what we have already run into with the ubiqu SyncLock, SyncObject etc. What do you think?
There was a problem hiding this comment.
Would you expose like an AsyncLock or just a thin wrapper around semaphoreslim?
2 participants
Proposed changes
This pull request improvements to session management server. The main changes include refactoring session-related interfaces and implementations to support asynchronous operations, replacing lock-based synchronization with
SemaphoreSlimfor better scalability. These changes enhance the server's ability to handle concurrent requests efficiently and pave the way for modern async workflows.Session Management Refactoring (Async Support)
Refactored
ISessionManagerand its implementation to use async methods for session startup, creation, activation, and closing, replacing synchronous methods withValueTask-based async counterparts. Introduced a newCreateSessionResultclass to encapsulate session creation results.Updated
IServerInternalandServerInternalDatato support async session closing viaCloseSessionAsync, propagating async session management throughout the server stack.Concurrency Improvements
lock/Lock) withSemaphoreSlimfor thread-safe access to shared resources in session management and global discovery server code, improving scalability and responsiveness.Types of changes
Checklist
Further comments