Merge pull request #522 from Inxton/fix-duplicate-identities-false-positives

fix duplicate identities false positives

Author: PTKu

docfx/articles/connectors/IDENTITY.md

@@ -0,0 +1,64 @@
+# Twin Identity System
+
+## Overview
+
+The identity system in AXSharp allows twin objects to be looked up by a unique numeric identity (`ulong`). This is used by components that need to resolve references between twin objects at runtime (e.g. the `[MemberByIdentity]` attribute).
+
+The core class is [`TwinIdentityProvider`](~/api/AXSharp.Connector.Identity.TwinIdentityProvider.yml), which is responsible for assigning, writing, and sorting identity values.
+
+## How identities work
+
+1. During twin construction, each object that implements `ITwinIdentity` is registered via `AddIdentity`.
+2. When the application starts, `ConstructIdentitiesAsync` is called to assign identity values, write them to the PLC, and build a sorted lookup dictionary.
+3. Other parts of the system can then resolve objects by their identity using `GetTwinByIdentity`.
+
+## Constructing identities
+
+```csharp
+await connector.IdentityProvider.ConstructIdentitiesAsync();
+```
+
+`ConstructIdentitiesAsync` performs the following steps:
+
+1. **Assign** -- Each identity tag is assigned a `ulong` value using the provided `identityProvider` function. If no function is supplied, a default provider based on `string.GetHashCode()` of the symbol is used.
+2. **Write** -- The assigned values are written to the PLC via `WriteBatchAsync`. Values are always written fresh, regardless of what was previously stored on the PLC.
+3. **Sort** -- The identities are sorted into a `SortedDictionary<ulong, ITwinIdentity>` using the locally assigned values (`Cyclic`), not values read back from the PLC.
+
+This ensures that stale or inconsistent identity values left on the PLC from prior sessions cannot cause errors.
+
+## The `failOnDuplicate` parameter
+
+```csharp
+await connector.IdentityProvider.ConstructIdentitiesAsync(failOnDuplicate: false);
+```
+
+By default, `ConstructIdentitiesAsync` throws a `DuplicateIdentityException` when two symbols resolve to the same identity value. You can set `failOnDuplicate` to `false` to log a warning and skip the duplicate entry instead.
+
+## Custom identity providers
+
+The default identity provider uses `string.GetHashCode()`, which has two important limitations:
+
+- **Not deterministic across processes** -- In .NET Core/.NET 5+, `string.GetHashCode()` is randomized per process. Identity values will differ between application restarts.
+- **Collision risk** -- `GetHashCode()` returns a 32-bit value. For large PLC programs with many symbols, hash collisions become increasingly likely (birthday paradox).
+
+For production use, supply a custom identity provider that guarantees uniqueness:
+
+```csharp
+await connector.IdentityProvider.ConstructIdentitiesAsync(
+    identityProvider: tag => tag.Cyclic = MyStableHashFunction(tag.Symbol)
+);
+```
+
+A good custom provider should:
+- Produce deterministic values for the same symbol across process restarts.
+- Guarantee uniqueness (or near-uniqueness) across all symbols in the program.
+- Use the full 64-bit `ulong` range to minimize collision probability.
+
+## Troubleshooting
+
+### `DuplicateIdentityException`
+
+This exception is thrown when two different symbols are assigned the same identity value. Common causes:
+
+- **Hash collision** with the default `GetHashCode()`-based provider. Solution: use a custom identity provider with better distribution, or set `failOnDuplicate: false` if identity-based lookups are not critical for your application.
+- **Stale PLC values** (resolved in current version) -- Previously, the identity system would read back values from the PLC after writing. If the PLC returned stale data from a prior session, this could cause spurious duplicate errors. The system now uses the locally assigned values directly, avoiding this issue.

docfx/articles/toc.yml

@@ -31,6 +31,8 @@
     href: ~/articles/connectors/Dummy.md
   - name: WebAPI
     href: ~/articles/connectors/WebAPI.md
+  - name: Identity
+    href: ~/articles/connectors/IDENTITY.md
 - name: Blazor rendering
   href: ~/articles/blazor/README.md
   items:

src/AXSharp.connectors/src/AXSharp.Connector/Identity/TwinIdentityProvider.cs

@@ -206,7 +206,7 @@ internal async Task<IEnumerable<OnlinerULInt>> ReadIdentitiesAsync()
     /// </summary>
     /// <param name="identities">Identities</param>
     /// <param name="identityProvider">Identity creator.</param>
-    /// <returns></returns>
+    /// <returns>Assigned identities.</returns>
     public IEnumerable<OnlinerULInt> AssignIdentities(IEnumerable<OnlinerULInt> identities, Func<OnlinerULInt, ulong> identityProvider = null)
     {
         // If no identity provider is given, use default one based on hash code of the symbol.
@@ -234,45 +234,63 @@ public async Task WriteIdentities(IEnumerable<OnlinerULInt> identitiesToWrite)
     }
 
     /// <summary>
-    ///     Refreshes and sorts identities.
+    ///    Constructs identities by assigning them locally, writing to PLC, and sorting by the assigned values.
+    ///    Identity values are always written fresh to the PLC regardless of any previously stored values.
+    ///    This ensures that stale or inconsistent identity values from prior sessions do not cause duplicate identity errors.
     /// </summary>
-    public async Task ConstructIdentitiesAsync(Func<OnlinerULInt, ulong> identityProvider = null)
+    /// <param name="identityProvider">
+    ///    Function that assigns and returns an identity value for each <see cref="OnlinerULInt"/> tag.
+    ///    When <c>null</c>, a default provider based on <see cref="string.GetHashCode()"/> of the symbol is used.
+    ///    Note: <see cref="string.GetHashCode()"/> is not deterministic across process restarts in .NET Core/.NET 5+,
+    ///    so identity values will differ between sessions. For stable identities, supply a custom provider.
+    /// </param>
+    /// <param name="failOnDuplicate">
+    ///    When <c>true</c>, throws <see cref="DuplicateIdentityException"/> if two symbols resolve to the same identity value.
+    ///    When <c>false</c>, logs a warning and skips the duplicate entry.
+    /// </param>
+    public async Task ConstructIdentitiesAsync(Func<OnlinerULInt, ulong> identityProvider = null, bool failOnDuplicate = true)
     {
-        await WriteIdentities(AssignIdentities(_identitiesTags, identityProvider));        
-        await SortIdentitiesAsync();
+        await WriteIdentities(AssignIdentities(_identitiesTags, identityProvider));
+        await SortIdentitiesAsync(failOnDuplicate);
     }
 
     /// <summary>
-    ///     Sorts identities.
+    ///     Sorts identities using the locally assigned Cyclic values, without reading back from PLC.
     /// </summary>
-    internal async Task SortIdentitiesAsync()
+    internal async Task SortIdentitiesAsync(bool failOnDuplicate = true)
     {
-        await Task.Run(async () =>
+        _connector?.Logger.Information("Sorting identities from assigned values...");
+        _sortedIdentities.Clear();
+        foreach (var identity in _identities)
         {
-            _connector?.Logger.Information("Sorting identities...");
-            if (_connector != null)
+            var key = identity.Key.Cyclic;
+            if (!_sortedIdentities.ContainsKey(key) && key != 0)
             {
-                await _connector?.ReadBatchAsync(_identities.Select(p => p.Key), eAccessPriority.High);
+                _sortedIdentities.Add(key, identity.Value);
             }
-            _sortedIdentities.Clear();
-            foreach (var identity in _identities)
+            else
             {
-                var key = identity.Key.LastValue;
-                if (!_sortedIdentities.ContainsKey(key))
-                {
-                    _sortedIdentities.Add(key, identity.Value);
-                }
-                else
+                if (failOnDuplicate)
                 {
                     throw new DuplicateIdentityException("There is a duplicate identity: " +
-                                                         $"{identity.Value.Symbol} : {key}." +
+                                                         $"'{identity.Value.Symbol} : {key}.'" +
+                                                         $" and '{_sortedIdentities[key].Symbol} : {key}.'" +
+                                                         $" share the same identity value." +
                                                          $"The algorithm for assigning identities needs to be adjusted." +
                                                          $"Use an algorithm that guarantees unique identities and is less prone to collisions.");
                 }
+                else
+                {
+                    _connector?.Logger.Warning($"Duplicate identity detected: '{identity.Value.Symbol} : {key}' and '{_sortedIdentities[key].Symbol} : {key}' share the same identity value. " +
+                                               $"This entry will be ignored."+
+                                               $"The algorithm for assigning identities needs to be adjusted." +
+                                               $"Use an algorithm that guarantees unique identities and is less prone to collisions." +
+                                               $"Ignoring this warning may lead to unexpected behavior in those parts of the system that rely on unique identities.");
+                }
             }
+        }
 
-            _connector?.Logger.Information("Sorting identities done.");
-        });
+        _connector?.Logger.Information("Sorting identities done.");
     }
 }
 

src/AXSharp.connectors/tests/AXSharp.ConnectorTests/AXSharp.ConnectorTests/Identity/TwinIdentityProviderTests.cs

@@ -220,16 +220,19 @@ public async void CanCallSortIdentities()
 
             var identityVar_2 = Substitute.For<OnlinerULInt, IOnline<ulong>>();
             identityVar_2.LastValue.Returns(2ul);
+            identityVar_2.Cyclic.Returns(2ul);
             var obj2 = Substitute.For<ITwinIdentity>();
             obj2.Identity.Returns(identityVar_2);
 
             var identityVar_1 = Substitute.For<OnlinerULInt, IOnline<ulong>>();
             var obj1 = Substitute.For<ITwinIdentity>();
             obj1.Identity.Returns(identityVar_1);
             identityVar_1.LastValue.Returns(1ul);
+            identityVar_1.Cyclic.Returns(1ul);
 
             var identityVar_3 = Substitute.For<OnlinerULInt, IOnline<ulong>>();
             identityVar_3.LastValue.Returns(3ul);
+            identityVar_3.Cyclic.Returns(3ul);
             var obj3 = Substitute.For<ITwinIdentity>();
             obj3.Identity.Returns(identityVar_3);