#2656 Fix for - Session is not provided by ClearChangeMasks when a change is notified

[Filippo-Oliva-ABB]

Proposed changes

It fixes this issue by including in the SystemContext (taken as an argument to the OnMonitoredNodeChanged method) the Server session related to the current MonitoredItem that is going to be passed to the QueueValue.

Related Issues

• Fixes Session is not provided by "ClearChangeMasks" when a change is notified #2656

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply. You can also fill these out after creating the PR.

• 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

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

• 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]

All committers have signed the CLA.

[codecov]

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 54.67%. Comparing base (36dbdd3) to head (1c96145).
Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
...braries/Opc.Ua.Server/Diagnostics/MonitoredNode.cs	25.00%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2772      +/-   ##
==========================================
+ Coverage   54.62%   54.67%   +0.05%     
==========================================
  Files         349      349              
  Lines       65697    65703       +6     
  Branches    13441    13443       +2     
==========================================
+ Hits        35886    35923      +37     
+ Misses      25917    25888      -29     
+ Partials     3894     3892       -2     

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

[Filippo-Oliva-ABB]

I added the Session information to the context passed to the QueueValue.

[Filippo-Oliva-ABB]

Hi team,

I’ve noticed that the validation pipeline for my pull request failed due to code coverage requirements. While I understand the importance of comprehensive testing, I encountered a challenge because the existing library does not seem to provide a complete test set for the method I fixed.

To ensure the quality and reliability of my contribution, I would appreciate any guidance or suggestions on how to proceed with creating the necessary unit tests. Alternatively, if there are existing tests that I might have overlooked, please point me in the right direction.

Thank you for your understanding and support.

Best regards

Filippo

[mregen]

Hi @Filippo-Oliva-ABB , no worries, the code coverage is flaky and not required to pass... the problem is that the devops pipeline was not triggered. Thats a known issue with first time PR.

[mregen]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mregen]

great find!

[mregen]

@Filippo-Oliva-ABB unfortunately testing after the merge I ran into a NullArgumentException due to this change, MontoredItem.Session can be null.
Yet undecided if we roll back the change or push a fix. Investigating.

[Filippo-Oliva-ABB]

Hi @mregen, sorry for the exception thrown, I thought a MonitoredItem always had an associated Session. What are the cases where this doesn't happen?

[mregen]

Please feel free to reopen a PR, we need to have another look how to fix the issue.

[Filippo-Oliva-ABB]

Hi @mregen,

I could reopen a PR to add a line before copying the ServerSystemContext with the Session inclusion that checks that the MonitoredNode.Session is not null and, if so, I could skip the enqueuing of the MonitoredItem.
However, after analyzing the “Session” property of the MonitoredNode class, I noticed that it refers to the Session of the related Subscription.

The MonitoredNode.Session property acquires a lock on an internal “lock object,” which does not prevent the Subscription.Session object from changing asynchronously. I could be wrong but, in other words, the MonitoredNode does not appear to be synchronized with the Subscription.

My concern is that there may be a concurrency issue. The double-check on the Session that I propose in my new PR might mitigate this issue somewhat, but it may not fully resolve it.

I hope this clarifies my concerns. Thank you for considering my input.

Best regards

Filippo

[mregen]

Hi @Filippo-Oliva-ABB , it looks like this issue needs more investigation. its on the backlog. Thanks, Martin