Added a new setting MessageTransportMode which enables the user to decide if DataEfficiency or TimeCriticality is important

[sxleixer]

Proposed changes

I and my colleagues encountered a perfomance issue lately using the default OPC UA Stack.
We were issuing a method call and waited in parallel for a node value to change. Which signalled the end of the running method.
Calling the method alone took us 17ms in median, but when waiting for the node value changes to occurr we suddenly were waiting for up to 200ms. However, we were rather expecting about 45ms.

Debugging into the OPC UA stack I realized, when I set the NoDelay property of the Socket to true that these 200ms shrunk down to 47ms in median, totally meeting our expectations.

In this PR I propose a new setting in the application configuration. I call it MessageTransportMode and the user can select between two modes DataEfficient (NoDelay = false) and TimeCritical (NoDelay = true). These settings are added into the ClientConfiguration and ServerConfiguration sections of the ApplicationConfiguration and passed throgh the whole stack.

Related Issues

• Fixes #

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.

Further comments

None

[CLAassistant]

All committers have signed the CLA.

[romanett]

/azp run

[azure-pipelines]

Pull request contains merge conflicts.

[codecov]

Codecov Report

Attention: Patch coverage is 71.79487% with 11 lines in your changes missing coverage. Please review.

Project coverage is 55.45%. Comparing base (c87eca9) to head (f597c7d).
Report is 17 commits behind head on master.

Files with missing lines	Patch %	Lines
...ack/Opc.Ua.Core/Stack/Client/RegistrationClient.cs	0.00%	6 Missing ⚠️
...a.Configuration/ApplicationConfigurationBuilder.cs	0.00%	2 Missing ⚠️
...ack/Opc.Ua.Core/Schema/ApplicationConfiguration.cs	50.00%	1 Missing ⚠️
...k/Opc.Ua.Core/Stack/Tcp/UaSCBinaryClientChannel.cs	66.66%	1 Missing ⚠️
....Core/Stack/Transport/TransportListenerSettings.cs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2882      +/-   ##
==========================================
+ Coverage   55.37%   55.45%   +0.07%     
==========================================
  Files         352      352              
  Lines       67602    67614      +12     
  Branches    13849    13851       +2     
==========================================
+ Hits        37437    37497      +60     
+ Misses      26059    26015      -44     
+ Partials     4106     4102       -4     

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

Copilot reviewed 6 out of 17 changed files in this pull request and generated no comments.

Files not reviewed (11)

• Stack/Opc.Ua.Core/Schema/ApplicationConfiguration.xsd: Language not supported
• Libraries/Opc.Ua.Client/Session/Session.cs: Evaluated as low risk
• Libraries/Opc.Ua.Configuration/ApplicationConfigurationBuilder.cs: Evaluated as low risk
• Libraries/Opc.Ua.Configuration/IApplicationConfigurationBuilder.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Schema/ApplicationConfiguration.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Client/RegistrationClient.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Client/SessionChannel.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Client/UaChannelBase.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Tcp/TcpMessageSocket.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Tcp/TcpServerChannel.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Tcp/TcpTransportListener.cs: Evaluated as low risk

Comments suppressed due to low confidence (2)

Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryClientChannel.cs:75

• [nitpick] The variable name m_transportMode should be reviewed for consistency with the naming conventions used in the rest of the codebase.

m_transportMode = transportMode;

Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryClientChannel.cs:42

• Ensure that there are test cases covering the new behavior introduced by MessageTransportMode.

MessageTransportMode transportMode = MessageTransportMode.DataEfficient)

[mregen]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

Copilot reviewed 6 out of 17 changed files in this pull request and generated no comments.

Files not reviewed (11)

• Stack/Opc.Ua.Core/Schema/ApplicationConfiguration.xsd: Language not supported
• Stack/Opc.Ua.Core/Stack/Client/SessionChannel.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryTransportChannel.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Tcp/UaSCBinaryClientChannel.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Tcp/TcpMessageSocket.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Client/UaChannelBase.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Tcp/TcpTransportListener.cs: Evaluated as low risk
• Libraries/Opc.Ua.Client/Session/Session.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Tcp/TcpServerChannel.cs: Evaluated as low risk
• Stack/Opc.Ua.Core/Stack/Transport/IMessageSocket.cs: Evaluated as low risk
• Libraries/Opc.Ua.Configuration/IApplicationConfigurationBuilder.cs: Evaluated as low risk

Comments suppressed due to low confidence (1)

Stack/Opc.Ua.Core/Stack/Transport/TransportChannelSettings.cs:131

• Ensure that the new behavior introduced by the MessageTransportMode setting is adequately covered by tests.

public MessageTransportMode TransportMode

[sxleixer]

I'm kindly requesting a feedback for this PR, since we need it in Production.

[marcschier]

First off, why not always set NoDelay? Also, why are you always connecting for every method call? Can you not keep the session around to save yourself the connect cost?

[sxleixer]

First off, why not always set NoDelay? Also, why are you always connecting for every method call? Can you not keep the session around to save yourself the connect cost?

Thank you for your response.

Firstly, regarding the use of NoDelay, it is important to consider that setting it to true can actually reduce throughput and potentially disrupt existing use cases. The TCP protocol incurs overhead for each packet sent, so pooling requests by NoDelay = false can enhance throughput performance. In my opinion it is more safe to keep the default behavior and give the user the possibility to change the behavior accordingly.

Secondly, I honestly have no clue where you came up with the information that we supposedly do not keep the session open. If it is for the latency - our PLC has a cycle time of 5ms and it is introducing this latency. The session is open and kept alive at all times.