Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Client] Time calculations using DateTime are susceptible to produce erroneous values #2546

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

[Client] Time calculations using DateTime are susceptible to produce erroneous values #2546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b7389e3
using HiResCock info instead of UTCNow
bhnaphade Mar 5, 2024
2368694
more updates
bhnaphade Mar 5, 2024
dac0518
reverted change fixing test failures
bhnaphade Mar 6, 2024
b043334
updated to use environment.ticks for keep alive.
bhnaphade Mar 7, 2024
d4b1c24
more updates
bhnaphade Mar 7, 2024
fc5e21c
removed debug lines
bhnaphade Mar 7, 2024
0ceca80
Merge branch 'master' of https://github.com/OPCFoundation/UA-.NETStan…
bhnaphade Mar 25, 2024
ebcf543
Merge branch 'master' into time-calculation-error-on-system-time-change
bhnaphade Apr 10, 2024
05a97e5
Updated Subscription to use TickCount omstead of UTCNow
bhnaphade Apr 11, 2024
b0f1de8
updated Session to use TickCount
bhnaphade Apr 11, 2024
59437eb
removed unused code
bhnaphade Apr 11, 2024
484691e
Merge branch 'master' into time-calculation-error-on-system-time-change
bhnaphade Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions Libraries/Opc.Ua.Client/Session.cs
View file
Open in desktop
Expand Up @@ -748,7 +748,7 @@
{
get
{
TimeSpan delta = TimeSpan.FromTicks(DateTime.UtcNow.Ticks - Interlocked.Read(ref m_lastKeepAliveTime));
TimeSpan delta = TimeSpan.FromTicks(HiResClock.TickCount - Interlocked.Read(ref m_lastKeepAliveTime));
Copy link
Contributor

Choose a reason for hiding this comment

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

with the move to TickCount which is an int, the m_lastKeepAliveTime can also be an int and the interlocked is not necessary anymore. .NET guarantees atomic operation for the datatype.


// add a guard band to allow for network lag.
return (m_keepAliveInterval + kKeepAliveGuardBand) <= delta.TotalMilliseconds;
Expand Down Expand Up @@ -3689,7 +3689,7 @@
{
int keepAliveInterval = m_keepAliveInterval;

Interlocked.Exchange(ref m_lastKeepAliveTime, DateTime.UtcNow.Ticks);
Interlocked.Exchange(ref m_lastKeepAliveTime, HiResClock.TickCount);

m_serverState = ServerState.Unknown;

Expand Down Expand Up @@ -3959,7 +3959,7 @@
return;
}

Interlocked.Exchange(ref m_lastKeepAliveTime, DateTime.UtcNow.Ticks);
Interlocked.Exchange(ref m_lastKeepAliveTime, HiResClock.TickCount);

Check warning on line 3962 in Libraries/Opc.Ua.Client/Session.cs

View check run for this annotation

Codecov / codecov/patch

Libraries/Opc.Ua.Client/Session.cs#L3962 

Added line #L3962 was not covered by tests

lock (m_outstandingRequests)
{
Expand All @@ -3976,7 +3976,7 @@
}
else
{
Interlocked.Exchange(ref m_lastKeepAliveTime, DateTime.UtcNow.Ticks);
Interlocked.Exchange(ref m_lastKeepAliveTime, HiResClock.TickCount);
}

// save server state.
Expand All @@ -4002,7 +4002,7 @@
/// </summary>
protected virtual bool OnKeepAliveError(ServiceResult result)
{
long delta = DateTime.UtcNow.Ticks - Interlocked.Read(ref m_lastKeepAliveTime);
long delta = HiResClock.TickCount - Interlocked.Read(ref m_lastKeepAliveTime);

Check warning on line 4005 in Libraries/Opc.Ua.Client/Session.cs

View check run for this annotation

Codecov / codecov/patch

Libraries/Opc.Ua.Client/Session.cs#L4005 

Added line #L4005 was not covered by tests

Utils.LogInfo(
"KEEP ALIVE LATE: {0}s, EndpointUrl={1}, RequestCount={2}/{3}",
Copy link
Contributor

Choose a reason for hiding this comment

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

this calculation is now based on millisecond counter, not ticks, and needs to be fixed.

Expand Down
8 changes: 3 additions & 5 deletions Libraries/Opc.Ua.Client/Subscription.cs
View file
Open in desktop
Expand Up @@ -799,8 +799,7 @@ public bool PublishingStopped
{
get
{
TimeSpan timeSinceLastNotification = TimeSpan.FromTicks(DateTime.UtcNow.Ticks - Interlocked.Read(ref m_lastNotificationTime));
if (timeSinceLastNotification.TotalMilliseconds > m_keepAliveInterval + kKeepAliveTimerMargin)
if ((HiResClock.TickCount - m_lastNotificationTime) > (m_keepAliveInterval + kKeepAliveTimerMargin))
{
return true;
}
Expand Down Expand Up @@ -1429,7 +1428,7 @@ public IList<MonitoredItem> DeleteItems()
}

DateTime now = DateTime.UtcNow;
Interlocked.Exchange(ref m_lastNotificationTime, now.Ticks);
Interlocked.Exchange(ref m_lastNotificationTime, HiResClock.TickCount);

// save the string table that came with notification.
message.StringTable = new List<string>(stringTable);
Expand Down Expand Up @@ -1839,8 +1838,7 @@ private void StartKeepAliveTimer()
{
Utils.SilentDispose(m_publishTimer);
m_publishTimer = null;

Interlocked.Exchange(ref m_lastNotificationTime, DateTime.UtcNow.Ticks);
Interlocked.Exchange(ref m_lastNotificationTime, HiResClock.TickCount);
m_keepAliveInterval = (int)(Math.Min(m_currentPublishingInterval * (m_currentKeepAliveCount + 1), Int32.MaxValue));
if (m_keepAliveInterval < kMinKeepAliveTimerInterval)
{
Expand Down