Auto: twincat-2.2 — handle-based access with caching

Closes #311

src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs

@@ -31,6 +31,26 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
     // than tracking liveness, matching the AbCip / Modbus / FOCAS pattern from #181.
     private readonly ConcurrentDictionary<string, SemaphoreSlim> _bitWriteLocks = new();
 
+    // PR 2.2 — handle cache. Per-tag read/write resolves a symbolic path to an ADS
+    // variable handle once, then issues every subsequent op against the handle. Smaller
+    // AMS payloads (4-byte handle vs N-byte path) + skips name resolution in the runtime.
+    // Lifetime is process-scoped: cleared on reconnect (EnsureConnected path), wiped on
+    // a Symbol-Version-Invalid retry, and disposed on Dispose. PR 2.3 will wire a
+    // proactive Symbol Version invalidation listener so stale handles after an online
+    // change get evicted before the next read fails — until then, operators can call
+    // FlushOptionalCachesAsync to wipe manually.
+    private readonly ConcurrentDictionary<string, uint> _handleCache = new();
+    private bool _wasConnected;
+    private readonly object _connectionStateGate = new();
+
+    // Test-only counter — number of CreateVariableHandleAsync calls actually issued
+    // (i.e. cache misses). Integration tests assert this stays at the unique-symbol
+    // count after a second pass over the same set.
+    internal int HandleCreateCount;
+
+    /// <summary>Test-only — current size of the handle cache.</summary>
+    internal int HandleCacheCount => _handleCache.Count;
+
     public AdsTwinCATClient()
     {
         _client.AdsNotificationEx += OnAdsNotificationEx;
@@ -43,7 +63,24 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
         if (_client.IsConnected) return Task.CompletedTask;
         _client.Timeout = (int)Math.Max(1_000, timeout.TotalMilliseconds);
         var netId = AmsNetId.Parse(address.NetId);
+
+        // PR 2.2 — a fresh AMS session invalidates every cached handle (handle space is
+        // per-session in the ADS device server). Clear before reconnect so any read that
+        // raced with a transient drop never reuses a stale handle from the prior session.
+        // Note: the handles for the prior session are gone with that session — no need to
+        // call DeleteVariableHandleAsync, which would just fail with a transport error.
+        var wasConnected = false;
+        lock (_connectionStateGate)
+        {
+            wasConnected = _wasConnected;
+            _wasConnected = false;
+        }
+        if (wasConnected || !_handleCache.IsEmpty)
+            _handleCache.Clear();
+
         _client.Connect(netId, address.Port);
+
+        lock (_connectionStateGate) _wasConnected = _client.IsConnected;
         return Task.CompletedTask;
     }
 
@@ -59,13 +96,14 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
             var clrType = MapToClrType(type);
             var readType = IsWholeArray(arrayDimensions) ? clrType.MakeArrayType() : clrType;
 
-            var result = await _client.ReadValueAsync(symbolPath, readType, cancellationToken)
+            // PR 2.2 — handle-based read. EnsureHandleAsync resolves through the cache;
+            // SymbolVersionInvalid evicts + retries once with a fresh handle.
+            var (rawValue, errorCode) = await ReadByHandleWithRetryAsync(symbolPath, readType, cancellationToken)
                 .ConfigureAwait(false);
+            if (errorCode != AdsErrorCode.NoError)
+                return (null, TwinCATStatusMapper.MapAdsError((uint)errorCode));
 
-            if (result.ErrorCode != AdsErrorCode.NoError)
-                return (null, TwinCATStatusMapper.MapAdsError((uint)result.ErrorCode));
-
-            var value = result.Value;
+            var value = rawValue;
             if (IsWholeArray(arrayDimensions))
             {
                 value = PostProcessArray(type, value);
@@ -84,6 +122,92 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
         }
     }
 
+    /// <summary>
+    ///     Resolve <paramref name="symbolPath"/> to a cached ADS variable handle (or create one
+    ///     on first use) and dispatch a <see cref="AdsClient.ReadAnyAsync(uint, Type, CancellationToken)"/>.
+    ///     On <see cref="AdsErrorCode.DeviceSymbolVersionInvalid"/> evicts the cached handle
+    ///     + retries once with a freshly-created handle — covers the online-change race where
+    ///     the symbol survives but its descriptor moves.
+    /// </summary>
+    private async Task<(object? value, AdsErrorCode errorCode)> ReadByHandleWithRetryAsync(
+        string symbolPath, Type readType, CancellationToken cancellationToken)
+    {
+        var handle = await EnsureHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false);
+        var result = await _client.ReadAnyAsync(handle, readType, cancellationToken).ConfigureAwait(false);
+        if (result.ErrorCode == AdsErrorCode.DeviceSymbolVersionInvalid)
+        {
+            EvictHandle(symbolPath);
+            handle = await EnsureHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false);
+            result = await _client.ReadAnyAsync(handle, readType, cancellationToken).ConfigureAwait(false);
+        }
+        return (result.Value, result.ErrorCode);
+    }
+
+    /// <summary>
+    ///     Mirror of <see cref="ReadByHandleWithRetryAsync"/> for writes. Returns the final
+    ///     <see cref="AdsErrorCode"/>; the caller maps that to an OPC UA status.
+    /// </summary>
+    private async Task<AdsErrorCode> WriteByHandleWithRetryAsync(
+        string symbolPath, object value, CancellationToken cancellationToken)
+    {
+        var handle = await EnsureHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false);
+        var result = await _client.WriteAnyAsync(handle, value, cancellationToken).ConfigureAwait(false);
+        if (result.ErrorCode == AdsErrorCode.DeviceSymbolVersionInvalid)
+        {
+            EvictHandle(symbolPath);
+            handle = await EnsureHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false);
+            result = await _client.WriteAnyAsync(handle, value, cancellationToken).ConfigureAwait(false);
+        }
+        return result.ErrorCode;
+    }
+
+    /// <summary>
+    ///     Lookup-or-create the cached ADS handle for <paramref name="symbolPath"/>. The
+    ///     <see cref="ConcurrentDictionary{TKey, TValue}"/> guarantees publication safety,
+    ///     but two concurrent callers on a cold key may both call
+    ///     <see cref="AdsClient.CreateVariableHandleAsync(string, CancellationToken)"/>.
+    ///     The loser's handle leaks for the lifetime of the process — acceptable cost
+    ///     given how narrow the race window is, and matched by the libplctag / S7 driver
+    ///     handle-cache patterns.
+    /// </summary>
+    internal async ValueTask<uint> EnsureHandleAsync(string symbolPath, CancellationToken cancellationToken)
+    {
+        if (_handleCache.TryGetValue(symbolPath, out var existing))
+            return existing;
+
+        Interlocked.Increment(ref HandleCreateCount);
+        var result = await _client.CreateVariableHandleAsync(symbolPath, cancellationToken).ConfigureAwait(false);
+        if (result.ErrorCode != AdsErrorCode.NoError)
+            throw new AdsErrorException(
+                $"CreateVariableHandleAsync failed for '{symbolPath}'", result.ErrorCode);
+
+        // GetOrAdd on a hit returns the winning handle; a loser-side DeleteVariableHandle here
+        // would race against an in-flight read using that handle elsewhere in this method, so
+        // we accept the small leak (one-time, per cold key) instead.
+        return _handleCache.GetOrAdd(symbolPath, result.Handle);
+    }
+
+    /// <summary>
+    ///     Evict a single cached handle. Best-effort delete on the wire — the runtime may
+    ///     already have invalidated the handle (Symbol-Version-Invalid path), so we swallow
+    ///     transport / ADS errors here.
+    /// </summary>
+    private void EvictHandle(string symbolPath)
+    {
+        if (!_handleCache.TryRemove(symbolPath, out var handle)) return;
+        try
+        {
+            // Fire-and-forget delete — the cache key is gone, the wire-side cleanup is
+            // strictly courtesy. If the device server is in a state where the handle is
+            // already dead, the delete will fail and we don't care.
+            _ = _client.DeleteVariableHandleAsync(handle, CancellationToken.None);
+        }
+        catch
+        {
+            // Best-effort.
+        }
+    }
+
     private static bool IsWholeArray(int[]? arrayDimensions) =>
         arrayDimensions is { Length: > 0 } && arrayDimensions.All(d => d > 0);
 
@@ -125,11 +249,12 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
         try
         {
             var converted = ConvertForWrite(type, value);
-            var result = await _client.WriteValueAsync(symbolPath, converted, cancellationToken)
+            // PR 2.2 — handle-based write with SymbolVersionInvalid evict-and-retry.
+            var errorCode = await WriteByHandleWithRetryAsync(symbolPath, converted, cancellationToken)
                 .ConfigureAwait(false);
-            return result.ErrorCode == AdsErrorCode.NoError
+            return errorCode == AdsErrorCode.NoError
                 ? TwinCATStatusMapper.Good
-                : TwinCATStatusMapper.MapAdsError((uint)result.ErrorCode);
+                : TwinCATStatusMapper.MapAdsError((uint)errorCode);
         }
         catch (AdsErrorException ex)
         {
@@ -159,19 +284,21 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
         await rmwLock.WaitAsync(cancellationToken).ConfigureAwait(false);
         try
         {
-            var read = await _client.ReadValueAsync(parentPath, typeof(uint), cancellationToken)
+            // PR 2.2 — RMW round-trip flows through the same handle cache so that the
+            // parent word's resolved handle is reused on subsequent bit writes too.
+            var (rawCurrent, readErr) = await ReadByHandleWithRetryAsync(parentPath, typeof(uint), cancellationToken)
                 .ConfigureAwait(false);
-            if (read.ErrorCode != AdsErrorCode.NoError)
-                return TwinCATStatusMapper.MapAdsError((uint)read.ErrorCode);
+            if (readErr != AdsErrorCode.NoError)
+                return TwinCATStatusMapper.MapAdsError((uint)readErr);
 
-            var current = Convert.ToUInt32(read.Value ?? 0u);
+            var current = Convert.ToUInt32(rawCurrent ?? 0u);
             var updated = ApplyBit(current, bit, setBit);
 
-            var write = await _client.WriteValueAsync(parentPath, updated, cancellationToken)
+            var writeErr = await WriteByHandleWithRetryAsync(parentPath, updated, cancellationToken)
                 .ConfigureAwait(false);
-            return write.ErrorCode == AdsErrorCode.NoError
+            return writeErr == AdsErrorCode.NoError
                 ? TwinCATStatusMapper.Good
-                : TwinCATStatusMapper.MapAdsError((uint)write.ErrorCode);
+                : TwinCATStatusMapper.MapAdsError((uint)writeErr);
         }
         catch (AdsErrorException ex)
         {
@@ -354,6 +481,12 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
     {
         if (reads.Count == 0) return Array.Empty<(object?, uint)>();
 
+        // PR 2.2 deviation: bulk path stays on symbolic Sum-command (SumInstancePathAnyTypeRead /
+        // SumWriteBySymbolPath). Beckhoff also exposes SumHandleRead / SumWriteByHandle, but
+        // wiring the cached handles into them changes the request layout substantially +
+        // would either need to reuse handles created on the per-tag path (tying lifetimes)
+        // or maintain a parallel handle batch — neither pulls weight against PR 2.1's already
+        // 10-20× win. Tracked as a follow-up for the Phase-2 perf sweep.
         // Build the (path, AnyTypeSpecifier) request envelope. SumInstancePathAnyTypeRead
         // batches all paths into a single ADS Sum-read round-trip (IndexGroup 0xF080 = read
         // multiple items by symbol name with ANY-type marshalling).
@@ -458,9 +591,53 @@ internal sealed class AdsTwinCATClient : ITwinCATClient
     {
         _client.AdsNotificationEx -= OnAdsNotificationEx;
         _notifications.Clear();
+
+        // PR 2.2 — release every cached handle on the wire as a good citizen. Best-effort
+        // and bounded to a short window so a hung router doesn't block process shutdown:
+        // each delete is fire-and-forget, errors swallowed. The session itself is about to
+        // tear down anyway, so the device server will reclaim everything regardless.
+        foreach (var kv in _handleCache)
+        {
+            try
+            {
+                _ = _client.DeleteVariableHandleAsync(kv.Value, CancellationToken.None);
+            }
+            catch
+            {
+                // Per-entry failures are expected on a closing connection.
+            }
+        }
+        _handleCache.Clear();
+
         _client.Dispose();
     }
 
+    /// <summary>
+    ///     PR 2.2 — flush all process-scoped optional caches (handle cache today). A
+    ///     proactive Symbol Version invalidation listener arrives in PR 2.3 — until then,
+    ///     operators / 2.3-aware callers can wipe the cache manually after a known online
+    ///     change.
+    /// </summary>
+    public Task FlushOptionalCachesAsync()
+    {
+        // Best-effort delete on the wire — a held handle won't survive a redeploy anyway,
+        // but cleaning up matches the Dispose convention.
+        var snapshot = _handleCache.ToArray();
+        _handleCache.Clear();
+        foreach (var kv in snapshot)
+        {
+            try
+            {
+                _ = _client.DeleteVariableHandleAsync(kv.Value, CancellationToken.None);
+            }
+            catch
+            {
+                // Best-effort.
+            }
+        }
+        return Task.CompletedTask;
+    }
+
     private sealed class NotificationRegistration(
         string symbolPath,
         TwinCATDataType type,

src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ITwinCATClient.cs

@@ -114,6 +114,16 @@ public interface ITwinCATClient : IDisposable
     ///     decide whether to drill in via their own walker.
     /// </summary>
     IAsyncEnumerable<TwinCATDiscoveredSymbol> BrowseSymbolsAsync(CancellationToken cancellationToken);
+
+    /// <summary>
+    ///     PR 2.2 — wipe process-scoped optional caches (today: the ADS variable-handle
+    ///     cache backing per-tag reads / writes). Surfaces so operators + the future PR
+    ///     2.3 Symbol-Version invalidation listener can flush stale handles after a known
+    ///     online change without forcing a full reconnect. Safe to call mid-traffic — in-flight
+    ///     reads continue to use the handles they already hold; the next read for a symbol
+    ///     will re-resolve. Best-effort wire-side delete; failures are swallowed.
+    /// </summary>
+    Task FlushOptionalCachesAsync();
 }
 
 /// <summary>Opaque handle for a registered ADS notification. <see cref="IDisposable.Dispose"/> tears it down.</summary>

src/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.csproj

@@ -26,6 +26,9 @@
 
     <ItemGroup>
         <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests"/>
+        <!-- PR 2.2 — integration tier needs visibility into AdsTwinCATClient + its
+             HandleCreateCount / HandleCacheCount counters to assert the live cache. -->
+        <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.IntegrationTests"/>
     </ItemGroup>
 
 </Project>