Skip to content

Conversation

@romanett
Copy link
Contributor

@romanett romanett commented Sep 15, 2025

Proposed changes

  • Extend IAsyncNodeManager with new methods (CreateAddressSpaceAsync, DeleteAddressSpaceAsync, DeleteReferenceAsync, GetManagerHandleAsync, GetNodeMetadataAsync, GetPermissionMetadataAsync, IsNodeInViewAsync, SessionClosingAsync)
  • Use SemaphoreSlim to lock async startup and shutdown of MasterNodeManager
  • Use Async Methods in the MasterNodeManager
  • Expose IAsyncNodeManagers from NodeManagers & NamespaceManagers property

Related Issues

  • Fixes #

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.

TODO

In the next PR the Management methods for MonitoredItems will be updated for async compatiblity.
Once the IAsyncNodeManager interface is complete it will be possible to register a NodeManager that only implements the IAsyncNodeManager and not the INodeManager Interface.

At a later point in time when CustomNodeManager3 (IAsyncNodeManager) is implemented, only IAsyncNodeManager could be exposed from the MasterNodeManager (all existing INodeManagers would be exposed via the AsyncNodeManagerAdapter).

…AsyncNodeManager

Refactor MasterNodeManager to allow access to IAsyncNodeManager from NodeManagers & NamespaceManagers Properties
Copy link
Contributor

Copilot AI left a 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 extends the IAsyncNodeManager interface with new async methods and modernizes the MasterNodeManager to use asynchronous operations throughout. The changes create better async compatibility across the node management system while maintaining backward compatibility through adapter patterns.

Key changes:

  • Extended IAsyncNodeManager with core service methods (CreateAddressSpaceAsync, DeleteAddressSpaceAsync, GetNodeMetadataAsync, etc.)
  • Replaced synchronous operations in MasterNodeManager with async equivalents using ValueTask
  • Changed from Lock to SemaphoreSlim for async-compatible thread safety during startup/shutdown

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
MasterNodeManager.cs Core implementation converting sync operations to async, replacing lock with SemaphoreSlim, updating data structures to store tuple pairs
INodeManager.cs Extended IAsyncNodeManager interface with new async method signatures
AsyncNodeManagerAdapter.cs Added implementation for new async methods, maintaining backward compatibility
StandardServer.cs Updated to call async startup/shutdown methods with GetAwaiter().GetResult()
ServerInternalData.cs Updated session closing to use async method
ContinuationPoint.cs Changed Manager property type to IAsyncNodeManager
Test files Updated test assertions to handle new tuple data structure in NamespaceManagers

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@codecov
Copy link

codecov bot commented Sep 15, 2025

Codecov Report

❌ Patch coverage is 73.96061% with 119 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.59%. Comparing base (912563e) to head (d077ebb).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...ies/Opc.Ua.Server/NodeManager/MasterNodeManager.cs 79.51% 74 Missing and 2 partials ⚠️
...c.Ua.Server/NodeManager/AsyncNodeManagerAdapter.cs 46.25% 28 Missing and 15 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3204      +/-   ##
==========================================
+ Coverage   57.46%   57.59%   +0.13%     
==========================================
  Files         355      355              
  Lines       78541    78549       +8     
  Branches    13855    13849       -6     
==========================================
+ Hits        45133    45240     +107     
+ Misses      29172    29069     -103     
- Partials     4236     4240       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@romanett romanett changed the title [Server] Implement Service Members of IASyncNodeManager in MasterNodeManager [Server] Implement Service Members of IAsyncNodeManager in MasterNodeManager Sep 15, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@marcschier marcschier merged commit 4654e3e into OPCFoundation:master Sep 21, 2025
100 of 101 checks passed
@romanett romanett deleted the feat/asyncNodeManager branch September 21, 2025 17:33
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.

2 participants