diff --git a/docs/drivers/AbCip.md b/docs/drivers/AbCip.md index 6d7b7193..de1157d2 100644 --- a/docs/drivers/AbCip.md +++ b/docs/drivers/AbCip.md @@ -181,8 +181,33 @@ The driver has two interchangeable wire transports, selected by one `DriverConfi an Unsupported refusal (final review I4): the tag's handle is kept (no re-create, no reconnect), driver health is unchanged (no `Degraded`, nothing fed to the comms-loss watchdog), and the Warning is logged **once per tag** per driver instance, not every poll. The same applies to the libplctag-path width check. -- Malformed tag names put the device into backoff, as under libplctag (#570). STRING capacity includes the - alignment pad (84, not 82) (#570). + The same holds when the controller refuses the **existence read itself** with a type error (CIP + `0xFF`/`0x2107`): initialization completes with the status instead of throwing, so the runtime is not + disposed and re-created (nor, for a sole lease holder, re-Forward-Opened) on every poll, and the device's + connect backoff is not opened (#570). Micro800 `BYTE`/`WORD`/`LWORD` (`0xD1`/`0xD2`/`0xD4`) are checked as + integers of their width: a `REAL`/`LREAL` or a narrower integer authored over an `LWORD` is + `BadTypeMismatch`, not decoded garbage (#570). +- **A malformed tag name is a per-tag configuration error** (#570). A `TagPath` that does not parse, or a + name the CIP symbolic encoder refuses (non-ASCII, an empty member such as `A..B`, a member longer than + 255 characters, an unterminated or non-numeric subscript), reads and writes **`BadConfigurationError`** + with one Warning per tag. Nothing reaches the device, so it never opens the device's connect backoff and + never degrades driver health — healthy tags on the same device keep reading. (libplctag still reports its + own `ErrorBadParam` for a name it rejects.) A malformed **probe** tag does not feed the backoff either. +- **STRING capacity:** the built-in Logix `STRING` is 88 bytes on the wire — `LEN` + `DATA[82]` + 2 bytes of + alignment padding — so a write holds at most **82** characters (it used to allow 84) and a read never + returns the padding as characters. The write still sends the whole 88-byte structure, padding zeroed, as + libplctag does. A user string type of any other size keeps the buffer-derived capacity (`size − 4`): the + read alone cannot tell its `DATA` length from up to 3 bytes of padding, so a write may accept up to 3 + characters more than the type holds (#570). +- **Releasing a tag never blocks** (#579). Disposing a native tag runtime — the driver does it under the + tag's lock on an evict, a reinitialize or a shutdown — releases its connection lease in the background + (`ConnectionLease.ReleaseInBackground`, the same shape the native AbLegacy runtime uses). It used to wait + up to 2 s for the last lease's Forward Close. Nothing is lost by not waiting: the pool records the close + synchronously, so a re-create the next instant still does not open a second session until that Forward + Close + UnRegisterSession have finished, and the close is bounded by the connection's own budgets (the + open budget for an open still in flight, 1 s for the Forward Close), so a connection is always released. + Offline soak: `NativeAbCipLeaseReleaseTests` (25 create/release cycles, no leaked connection); a soak + against `ab_server` has not been run. - **A refused operation is not a comms fault.** Any operation a runtime refuses as Unsupported (in AbCip, e.g. a native write whose type cannot be determined) reaches the client as `BadNotSupported`, keeps the tag handle, and leaves driver health unchanged; only real transport failures evict and degrade. diff --git a/docs/drivers/AbLegacy.md b/docs/drivers/AbLegacy.md index 20c251c7..dc243c4e 100644 --- a/docs/drivers/AbLegacy.md +++ b/docs/drivers/AbLegacy.md @@ -150,6 +150,9 @@ Native routing: SLC500/MicroLogix/PLC-5 are direct; `logixpccc` uses Unconnected - **ST (string) byte order is unverified** (no capture); string values may be byte-swapped. - Unlike AbCip, the native AbLegacy runtime has **no element-type check**, so a mis-authored same-width type reads as reinterpreted data (as on libplctag), not `BadTypeMismatch`. + - Releasing a native tag (evict, reinitialize, shutdown) never blocks on the connection close: the lease + is released in the background (`ConnectionLease.ReleaseInBackground`), the same shape as native AbCip + (#579). A re-create cannot overlap the close, because the pool holds the successor connection behind it. ### Wire evidence (native PCCC) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipPath.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipPath.cs index 058176aa..644f8f80 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipPath.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipPath.cs @@ -54,11 +54,21 @@ public static class CipPath { var bracket = part.IndexOf('['); var name = bracket < 0 ? part : part[..bracket]; + // An ANSI extended symbol segment carries a one-byte length of ASCII: a non-ASCII name would be + // sent as '?' substitutes and an over-long one would wrap its length byte — both silently + // address a different symbol, so refuse them here (Gitea #570). + if (name.Length is 0 or > byte.MaxValue || !Ascii.IsValid(name)) + throw new ArgumentException( + $"'{tagName}' has a member name that is empty, longer than {byte.MaxValue} characters, or not ASCII.", + nameof(tagName)); var a = Encoding.ASCII.GetBytes(name); w.U8(0x91).U8((byte)a.Length).Bytes(a).PadToEven(); if (bracket >= 0) { - foreach (var idxText in part[(bracket + 1)..part.IndexOf(']')].Split(',', StringSplitOptions.TrimEntries)) + var close = part.IndexOf(']'); + if (close < bracket) + throw new ArgumentException($"'{tagName}' has an unterminated subscript.", nameof(tagName)); + foreach (var idxText in part[(bracket + 1)..close].Split(',', StringSplitOptions.TrimEntries)) { var idx = uint.Parse(idxText, CultureInfo.InvariantCulture); if (idx < 0x100) w.U8(0x28).U8((byte)idx); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/DeviceSessionPool.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/DeviceSessionPool.cs index e2a2db66..1572caa3 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/DeviceSessionPool.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/DeviceSessionPool.cs @@ -158,4 +158,29 @@ public sealed class ConnectionLease : IAsyncDisposable public ValueTask DisposeAsync() => Interlocked.Exchange(ref _disposed, 1) == 0 ? _pool.ReleaseAsync(_key, _entry) : ValueTask.CompletedTask; + + /// + /// Releases this lease without waiting for the teardown — the one release shape both native AB tag + /// runtimes use from their synchronous Dispose, which the drivers call under a per-tag lock + /// (Gitea #579). Idempotent, like . + /// + /// + /// Nothing is lost by not waiting. The release's bookkeeping — the ref-count drop and, for the + /// last lease, the entry's removal and the registration of its close in the pool's closing chain — runs + /// synchronously, before this returns. So an acquire made the very next instant builds a connection that + /// does not open until this one's Forward Close + UnRegisterSession have finished (the guarantee the old + /// blocking 2 s wait in the AbCip runtime existed for), and the close itself is bounded by + /// 's own budgets (the open budget for an in-flight open, 1 s for the Forward + /// Close), so it always completes and the socket is always released. + /// The returned task is the teardown; it never faults (the pool logs its own close failures), so + /// a caller may drop it. + /// + /// The teardown, for tests and callers that want to observe it. + public Task ReleaseInBackground() => Observe(DisposeAsync()); + + private static async Task Observe(ValueTask release) + { + try { await release.ConfigureAwait(false); } + catch (Exception) { /* ReleaseAsync logs and swallows its own failures; never fault the observer */ } + } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index f8f03953..c88bfba6 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -207,6 +207,24 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, _logger.LogWarning(message, args); } + // Tags whose malformed-name Warning has already been logged (Gitea #570) — polled forever, warned once. + private readonly System.Collections.Concurrent.ConcurrentDictionary _configErrorWarned = + new(StringComparer.OrdinalIgnoreCase); + + /// + /// Logs the Warning for a tag whose own configuration cannot become a request + /// (), the first time reports + /// one on this instance. + /// + private void WarnConfigurationErrorOnce(string tagName, string reference, string device, AbCipTagConfigurationException ex) + { + if (_configErrorWarned.TryAdd(tagName, 0)) + _logger.LogWarning( + "AbCip tag {Tag} on device {Device} is misconfigured: {Reason}; reporting BadConfigurationError " + + "until it is corrected — the device and its other tags are unaffected (logged once per tag)", + reference, device, ex.Message); + } + /// Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8). private static readonly TimeSpan PollBackoffCap = TimeSpan.FromSeconds(30); @@ -576,7 +594,9 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, try { probeRuntime?.Dispose(); } catch { } probeRuntime = null; state.ProbeInitialized = false; - backoffVerdict = false; // 05/STAB-8: keep the data-path window open + // 05/STAB-8: keep the data-path window open — unless the probe tag's own name is malformed + // (Gitea #570): that never reached the device, so it is no evidence either way. + backoffVerdict = ex is AbCipTagConfigurationException ? null : false; } // ConnectionBackoff is caller-serialized, and the data path now records under the @@ -892,6 +912,14 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadNotSupported, null, now); return ReadOutcome.None; } + catch (AbCipTagConfigurationException ace) + { + // Gitea #570: this tag's name/path cannot become a request — nothing reached the device, so + // no evict (nothing was cached), no health signal, no backoff; siblings keep reading. + results[fb.OriginalIndex] = new DataValueSnapshot(null, AbCipStatusMapper.BadConfigurationError, null, now); + WarnConfigurationErrorOnce(def.Name, reference, def.DeviceHostAddress, ace); + return ReadOutcome.None; + } catch (AbTransportException ate) when (ate.Status == AbStatus.TypeMismatch) { // Final review I4: deterministic, keep the handle, no health signal, warn once per tag. @@ -989,6 +1017,13 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, StampGroupStatus(group, results, now, AbCipStatusMapper.BadNotSupported); return ReadOutcome.None; } + catch (AbCipTagConfigurationException ace) + { + // Gitea #570: the parent's name cannot become a request — per-group config error, device untouched. + StampGroupStatus(group, results, now, AbCipStatusMapper.BadConfigurationError); + WarnConfigurationErrorOnce(parent.Name, group.ParentName, parent.DeviceHostAddress, ace); + return ReadOutcome.None; + } catch (AbTransportException ate) when (ate.Status == AbStatus.TypeMismatch) { StampGroupStatus(group, results, now, AbCipStatusMapper.BadTypeMismatch); @@ -1150,6 +1185,12 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, "AbCip write refused (not supported) for tag {Tag} on device {Device}", w.FullReference, def.DeviceHostAddress); } + catch (AbCipTagConfigurationException ace) + { + // Gitea #570: a config error on this tag — no evict, no health signal, no backoff. + results[i] = new WriteResult(AbCipStatusMapper.BadConfigurationError); + WarnConfigurationErrorOnce(def.Name, w.FullReference, def.DeviceHostAddress, ace); + } catch (AbTransportException ate) when (ate.Status == AbStatus.TypeMismatch) { // Final review I4: the controller's type disagrees with the authored one (native type gate, or @@ -1266,7 +1307,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, if (device.Runtimes.TryGetValue(def.Name, out var existing)) return existing; var parsed = AbCipTagPath.TryParse(def.TagPath) - ?? throw new InvalidOperationException( + ?? throw new AbCipTagConfigurationException( $"AbCip tag '{def.Name}' has malformed TagPath '{def.TagPath}'."); await device.ConnectGate.WaitAsync(ct).ConfigureAwait(false); @@ -1295,6 +1336,24 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, { await runtime.InitializeAsync(ct).ConfigureAwait(false); } + catch (AbTransportException ate) when (ate.Status == AbStatus.TypeMismatch) + { + // Gitea #570: the controller ANSWERED with a type refusal — the tag exists and the session + // works. Keep the handle (disposing it re-creates it, and for a sole lease holder + // re-Forward-Opens, on every poll) and let the caller report BadTypeMismatch. The native + // runtime completes its initialization instead of throwing this; the cache keeps any runtime + // that does throw it from churning. + device.Backoff.RecordSuccess(); + if (!device.Runtimes.TryAdd(def.Name, runtime)) runtime.Dispose(); + throw; + } + catch (AbCipTagConfigurationException) + { + // Gitea #570: the tag's own name cannot become a request — no I/O happened, so it says nothing + // about the device: leave the backoff window exactly as it was. + runtime.Dispose(); + throw; + } catch (Exception ex) { runtime.Dispose(); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs index 7b026d30..3bee959a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs @@ -45,6 +45,7 @@ public static class AbCipStatusMapper public const uint BadCommunicationError = 0x80050000u; public const uint BadTimeout = 0x800A0000u; public const uint BadTypeMismatch = 0x80740000u; + public const uint BadConfigurationError = 0x80890000u; /// Map a CIP general-status byte to an OPC UA StatusCode. /// The CIP general-status byte value. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTagConfigurationException.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTagConfigurationException.cs new file mode 100644 index 00000000..cf54c013 --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipTagConfigurationException.cs @@ -0,0 +1,18 @@ +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +/// +/// A tag's own configuration cannot be turned into a request at all — a malformed tag path, or a +/// symbolic name the native CIP encoder refuses (Gitea #570). Raised before any I/O, so it says nothing +/// about the device: the driver reports it per tag as BadConfigurationError and never counts it +/// against the device's connect backoff or its health, so healthy tags on the same device keep reading. +/// +internal sealed class AbCipTagConfigurationException : Exception +{ + /// Creates the exception. + /// What is wrong with the tag's configuration. + /// The encoder's own exception, if any. + public AbCipTagConfigurationException(string message, Exception? innerException = null) + : base(message, innerException) + { + } +} diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs index 60910133..d6b5c585 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs @@ -20,12 +20,18 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime { - /// Upper bound on waiting for the lease release (and its Forward Close) in . - private static readonly TimeSpan DisposeWait = TimeSpan.FromSeconds(2); - /// Bytes of a Logix STRING's length word. private const int StringLengthBytes = 4; + /// + /// Read size of the built-in Logix STRING: LEN (4) + DATA SINT[82] + 2 bytes of + /// alignment padding to the structure's 4-byte boundary. + /// + private const int BuiltInStringBytes = 88; + + /// Characters the built-in Logix STRING's DATA member holds. + private const int BuiltInStringCapacity = 82; + private readonly AbCipTagCreateParams _p; private readonly DeviceSessionPool? _pool; private readonly ushort _elements; @@ -90,9 +96,16 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime /// /// - /// Performs one read to prove the tag exists: libplctag's InitializeAsync errors for an + /// Performs one read to prove the tag exists: libplctag's InitializeAsync errors for an /// unknown tag, and the driver's 05/STAB-8 backoff classification depends on create throwing - /// . + /// . + /// A name the encoder refuses throws before any + /// lease (Gitea #570): it is this tag's configuration, not the device, and must not black out the + /// device's other tags. A read the controller answers with a deterministic refusal + /// ( / ) proves the tag exists, + /// so it completes the initialization with that status rather than throwing: a throw here makes the + /// driver dispose the runtime and re-create it — and, for a sole lease holder, re-Forward-Open — on + /// every poll (the mistype churn of #570). The driver's own read reports the refusal. /// public async Task InitializeAsync(CancellationToken cancellationToken) { @@ -100,9 +113,19 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime if (_path is null) { // Before any lease: a malformed name costs no connection. - var (name, bit) = CipPath.SplitBitSelector(_p.TagName); - var path = CipPath.EncodeSymbolic(name); - _implicitBit = bit; + (string Name, int? Bit) split; + byte[] path; + try + { + split = CipPath.SplitBitSelector(_p.TagName); + path = CipPath.EncodeSymbolic(split.Name); + } + catch (Exception ex) when (ex is ArgumentException or FormatException or OverflowException) + { + throw new AbCipTagConfigurationException( + $"AbCip tag name '{_p.TagName}' cannot be encoded as a CIP symbolic path: {ex.Message}", ex); + } + _implicitBit = split.Bit; _path = path; } if (_client is null) @@ -112,7 +135,14 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime // The lease's shared scheduler — never a private one (B1: MSP coalescing across tags). _client = new LogixTagClient(lease.Scheduler, lease.Channel); } - await ReadAsync(cancellationToken).ConfigureAwait(false); + try + { + await ReadAsync(cancellationToken).ConfigureAwait(false); + } + catch (AbTransportException ex) when (ex.Status is AbStatus.TypeMismatch or AbStatus.Unsupported) + { + // The controller answered — the tag exists. _status already holds the refusal. + } } /// @@ -339,29 +369,32 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime } /// - /// Releases the connection lease, waiting at most 2 s so an immediate re-init does not open a - /// second session to the device while this one's Forward Close is still in flight (review - /// finding 22). Never disposes the shared scheduler; the pool does that on the last release. + /// Releases the connection lease without blocking (Gitea #579) — the driver calls this under the + /// tag's lock, which used to be held for up to 2 s while the last release's Forward Close ran. An + /// immediate re-init still cannot open a second session while that close is in flight (review + /// finding 22): the pool registers the close synchronously and holds any successor connection + /// behind it — see . Never disposes the shared + /// scheduler; the pool does that on the last release. Same release shape as the native AbLegacy runtime. /// public void Dispose() { if (_disposed) return; _disposed = true; - try - { - if (_lease is { } lease) - lease.DisposeAsync().AsTask().Wait(DisposeWait); - else if (_privateScheduler is { } scheduler) - scheduler.DisposeAsync().AsTask().Wait(DisposeWait); - } - catch (AggregateException) - { - // A faulted close is the pool's problem to log; Dispose must not throw. - } + TeardownTask = _lease?.ReleaseInBackground() ?? DisposePrivateScheduler(_privateScheduler); _lease = null; _privateScheduler = null; } + /// The background teardown started, for tests; null before Dispose. + internal Task? TeardownTask { get; private set; } + + private static async Task DisposePrivateScheduler(RequestScheduler? scheduler) + { + if (scheduler is null) return; + try { await scheduler.DisposeAsync().ConfigureAwait(false); } + catch (Exception) { /* ForTest only; Dispose must not throw */ } + } + private byte[] Path() => _path ?? throw new InvalidOperationException("InitializeAsync has not been called."); @@ -383,7 +416,12 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime private bool IsTypeMismatch(ushort returned) { if (AtomicCode(_p.AuthoredType) is not { } expected) return false; - if (LogixTypeCode.ByteWidth(returned) < 0 || returned == expected) return false; + if (returned == expected) return false; + // Micro800 BYTE/WORD/LWORD (0xD1/0xD2/0xD4) are outside LogixTypeCode's width table but are + // integers of a known width: they are checked like any integer, so a REAL / LREAL / narrower + // integer authored over an LWORD is a mismatch rather than garbage decoded as Good (Gitea #570). + // Any other code without a known width (DATE, DATE_AND_TIME, structures) stays exempt. + if (LogixTypeCode.ByteWidth(returned) < 0 && IntegerWidth(returned) is null) return false; if (_p.AuthoredType == AbCipDataType.Bool) { if (returned == LogixTypeCode.BoolArray) return false; @@ -458,27 +496,40 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime return _data.AsSpan(offset, length); } + /// + /// Characters a STRING of holds. The built-in 88-byte STRING holds + /// 82 — its last 2 bytes are alignment padding, not DATA (Gitea #570). Any other size is a user + /// string type whose DATA length the read alone cannot separate from its padding (up to 3 bytes), + /// so it keeps the buffer-derived size − 4. + /// + private static int StringCapacity(int structureBytes) => structureBytes == BuiltInStringBytes + ? BuiltInStringCapacity + : structureBytes - StringLengthBytes; + /// Logix STRING: u32 LEN then ASCII DATA, LEN clamped to what the buffer holds. private string DecodeString(int offset) { var len = BinaryPrimitives.ReadUInt32LittleEndian(Span(offset, StringLengthBytes)); var available = _data.Length - offset - StringLengthBytes; + // A scalar built-in STRING never yields its alignment padding as characters. + if (offset == 0 && _data.Length == BuiltInStringBytes) available = BuiltInStringCapacity; var count = (int)Math.Min(len, (uint)available); return Encoding.ASCII.GetString(_data, offset + StringLengthBytes, count); } /// - /// u32 len + DATA padded to the capacity learned from the last read (data length − 4), so the - /// write carries the whole structure as the controller sized it. + /// u32 len + DATA, zero-padded to the size learned from the last read, so the write carries the + /// whole structure as the controller sized it (padding included, as libplctag sends it). The text + /// must fit the DATA capacity, which excludes the padding (). /// private byte[] EncodeStringPayload(string text) { - var capacity = _data.Length - StringLengthBytes; + var capacity = StringCapacity(_data.Length); var bytes = Encoding.ASCII.GetBytes(text); if (capacity < 0 || bytes.Length > capacity) throw new AbTransportException(AbStatus.TooLarge, - $"STRING write to '{_p.TagName}': {bytes.Length} character(s) exceeds the {Math.Max(capacity, 0)}-byte capacity."); - var payload = new byte[StringLengthBytes + capacity]; + $"STRING write to '{_p.TagName}': {bytes.Length} character(s) exceeds the {Math.Max(capacity, 0)}-character capacity."); + var payload = new byte[_data.Length]; BinaryPrimitives.WriteUInt32LittleEndian(payload, (uint)bytes.Length); bytes.CopyTo(payload, StringLengthBytes); return payload; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/NativeAbLegacyTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/NativeAbLegacyTagRuntime.cs index b11d3a82..e091e1f8 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/NativeAbLegacyTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/NativeAbLegacyTagRuntime.cs @@ -381,12 +381,11 @@ internal sealed class NativeAbLegacyTagRuntime : IAbLegacyTagRuntime } } - /// Releases the lease on the shared device connection (the last release closes it). - public void Dispose() - { - if (_lease is null) return; - // Closing does I/O (Forward Close / UnRegisterSession); never block the caller on it. - _ = _lease.DisposeAsync().AsTask().ContinueWith( - static t => { _ = t.Exception; }, TaskContinuationOptions.OnlyOnFaulted); - } + /// + /// Releases the lease on the shared device connection (the last release closes it) without waiting: + /// closing does I/O (Forward Close / UnRegisterSession) and the driver disposes under a per-tag lock. + /// The same release shape as the native AbCip runtime — see + /// for why a re-create the next instant still cannot overlap the close (Gitea #579). + /// + public void Dispose() => _lease?.ReleaseInBackground(); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/CipPathTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/CipPathTests.cs index 8b3b5415..1928cd0b 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/CipPathTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/CipPathTests.cs @@ -33,6 +33,23 @@ public sealed class CipPathTests public void Symbolic_rejects_bit_selector() => Should.Throw(() => CipPath.EncodeSymbolic("Tag.5")); + /// Gitea #570: names that used to be encoded silently wrong (a non-ASCII name as '?', an over-long + /// one with a wrapped length byte, an empty member as a zero-length segment) or that threw an unrelated + /// exception (an unterminated subscript) are refused as invalid arguments before any I/O. + [Theory] + [InlineData("Tëmp")] + [InlineData("A..B")] + [InlineData("Arr[1")] + public void Symbolic_rejects_names_it_cannot_encode_faithfully(string name) => + Should.Throw(() => CipPath.EncodeSymbolic(name)); + + [Fact] + public void Symbolic_rejects_a_member_longer_than_its_length_byte() + { + Should.Throw(() => CipPath.EncodeSymbolic(new string('A', 256))); + CipPath.EncodeSymbolic(new string('A', 255))[1].ShouldBe((byte)255); // the boundary still encodes + } + [Fact] public void Splits_bit_selector() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/DeviceSessionPoolTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/DeviceSessionPoolTests.cs index bf14ebf9..618ccb4c 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/DeviceSessionPoolTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/DeviceSessionPoolTests.cs @@ -267,6 +267,43 @@ public sealed class DeviceSessionPoolTests public void Dispose() { } } + /// + /// Gitea #579: returns before the last release's Forward + /// Close completes, yet a successor acquired that same instant still cannot open until the close is over — + /// the closing chain is registered synchronously, inside the call. + /// + [Fact] + public async Task Release_in_background_returns_before_the_close_and_a_successor_still_waits_for_it() + { + await using var target = new FakeCipTarget(); + var closeHeld = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + target.ForwardCloseGate = () => closeHeld.Task; + var time = new ManualTimeProvider(); + var pool = new DeviceSessionPool(time); + var key = new DeviceKey("127.0.0.1", target.Port, "1,0", "ControlLogix"); + + var a = pool.Acquire(key, Options()); + await a.Scheduler.SendAsync(ReadTag, TestCt); + var teardown = a.ReleaseInBackground(); + pool.EntryCount.ShouldBe(0); // bookkeeping done before it returned + await FakeCipTarget.Until(() => target.ForwardCloses == 1, Guard, "the Forward Close"); + teardown.IsCompleted.ShouldBeFalse(); // ...but the close is still pending + + time.Advance(TimeSpan.FromSeconds(5)); // the throttle is no obstacle + await using var b = pool.Acquire(key, Options()); + var bSend = b.Scheduler.SendAsync(ReadTag, TestCt); + await Task.Delay(300, TestCt); + target.ForwardOpens.ShouldBe(1); // held behind A's close + bSend.IsCompleted.ShouldBeFalse(); + + closeHeld.SetResult(); + await teardown.WaitAsync(Guard, TestCt); + (await bSend.WaitAsync(Guard, TestCt)).GeneralStatus.ShouldBe((byte)0); + target.ForwardOpens.ShouldBe(2); + await a.ReleaseInBackground().WaitAsync(Guard, TestCt); // idempotent: b's reference is untouched + pool.EntryCount.ShouldBe(1); + } + [Fact] public void Shared_is_one_process_wide_instance() => DeviceSessionPool.Shared.ShouldBeSameAs(DeviceSessionPool.Shared); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Fakes/FakeEipServer.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Fakes/FakeEipServer.cs index 79bf33bf..b6866fcd 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Fakes/FakeEipServer.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Fakes/FakeEipServer.cs @@ -29,6 +29,7 @@ internal sealed class FakeEipServer : IAsyncDisposable private readonly ConcurrentBag _clients = []; private readonly ConcurrentBag _handlerErrors = []; private int _connections; + private int _open; private int _outstanding; private int _maxOutstanding; @@ -45,6 +46,9 @@ internal sealed class FakeEipServer : IAsyncDisposable public int Connections => Volatile.Read(ref _connections); + /// TCP connections accepted and not yet closed by the client (Gitea #579 leak checks). + public int OpenConnections => Volatile.Read(ref _open); + /// Largest number of non-session requests ever received but not yet answered. public int MaxOutstanding => Volatile.Read(ref _maxOutstanding); @@ -108,12 +112,19 @@ internal sealed class FakeEipServer : IAsyncDisposable try { c = await _listener.AcceptTcpClientAsync(_cts.Token); } catch { return; } Interlocked.Increment(ref _connections); + Interlocked.Increment(ref _open); _clients.Add(c); _ = Serve(c); } } private async Task Serve(TcpClient c) + { + try { await ServeCore(c); } + finally { Interlocked.Decrement(ref _open); } + } + + private async Task ServeCore(TcpClient c) { using var _ = c; var s = c.GetStream(); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipMalformedTagNameTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipMalformedTagNameTests.cs new file mode 100644 index 00000000..ec977215 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipMalformedTagNameTests.cs @@ -0,0 +1,135 @@ +using Microsoft.Extensions.Logging; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes; +using CapturingLogger = ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests.AbCipLoggingTests.CapturingLogger; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Gitea #570: a tag whose own name cannot become a request is a configuration error on THAT tag. It +/// reports BadConfigurationError, never opens the device's connect-backoff window and never +/// degrades driver health, so healthy tags on the same device keep reading. Before the fix the native +/// encoder's raw exception was counted as a failed Forward Open: the window opened and every cold sibling +/// was throttled to BadCommunicationError — one typo blacked out the device. +/// +[Trait("Category", "Unit")] +public sealed class AbCipMalformedTagNameTests +{ + private const string Host = "ab://10.0.0.5/1,0"; + private const int Polls = 3; + +#pragma warning disable OTOPCUA0001 // the driver's own status handling is under test, not resilience wrapping + [Fact] + public async Task A_name_the_encoder_refuses_is_BadConfigurationError_and_does_not_throttle_a_cold_sibling() + { + var factory = new FakeAbCipTagFactory + { + Customise = p => p.TagName == "Bad" + ? new FakeAbCipTag(p) + { + ThrowOnInitialize = true, + Exception = new AbCipTagConfigurationException("'Bad' cannot be encoded"), + } + : new FakeAbCipTag(p) { Value = 7 }, + }; + var log = new CapturingLogger(); + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Host)], + RawTags = AbCipRawTags.From( + new AbCipTagDefinition("Bad", Host, "Bad", AbCipDataType.DInt, Writable: true), + new AbCipTagDefinition("Good", Host, "Good", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "drv-570", factory, logger: log); + await drv.InitializeAsync("{}", CancellationToken.None); + var health = drv.GetHealth(); + + for (var n = 0; n < Polls; n++) + (await drv.ReadAsync(["Bad"], CancellationToken.None)).Single().StatusCode + .ShouldBe(AbCipStatusMapper.BadConfigurationError); + (await drv.WriteAsync([new WriteRequest("Bad", 1)], CancellationToken.None)).Single().StatusCode + .ShouldBe(AbCipStatusMapper.BadConfigurationError); + + // The sibling is still COLD here, so it goes through the backoff check — a window opened by the + // malformed tag would throttle it to BadCommunicationError without ever creating it. + var good = (await drv.ReadAsync(["Good"], CancellationToken.None)).Single(); + good.StatusCode.ShouldBe(AbCipStatusMapper.Good); + good.Value.ShouldBe(7); + factory.Tags["Good"].InitializeCount.ShouldBe(1); + + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + drv.GetHealth().State.ShouldBe(health.State); + log.Entries.Count(e => e.Level == LogLevel.Warning && e.Message.Contains("misconfigured")).ShouldBe(1); + } + + [Fact] + public async Task A_malformed_TagPath_is_BadConfigurationError_without_degrading_health() + { + var factory = new FakeAbCipTagFactory { Customise = p => new FakeAbCipTag(p) { Value = 7 } }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Host)], + RawTags = AbCipRawTags.From( + new AbCipTagDefinition("Bad", Host, "1nvalid[", AbCipDataType.DInt, Writable: true), + new AbCipTagDefinition("Good", Host, "Good", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "drv-570-path", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + var results = await drv.ReadAsync(["Bad", "Good"], CancellationToken.None); + results[0].StatusCode.ShouldBe(AbCipStatusMapper.BadConfigurationError); + results[1].StatusCode.ShouldBe(AbCipStatusMapper.Good); // positive control: the batch did read + (await drv.WriteAsync([new WriteRequest("Bad", 1)], CancellationToken.None)).Single().StatusCode + .ShouldBe(AbCipStatusMapper.BadConfigurationError); + + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + factory.Tags.ContainsKey("1nvalid[").ShouldBeFalse(); // never created + } + + /// + /// End to end over the REAL native runtime + a private pool + a fake CIP target: a non-ASCII name (which + /// accepts — char.IsLetter — but the CIP symbolic encoder refuses) is + /// BadConfigurationError, the sibling on the same device reads Good, and the device sees ONE Forward Open. + /// + [Fact] + public async Task Over_the_native_runtime_a_non_ASCII_name_leaves_the_device_and_its_siblings_alone() + { + await using var target = new FakeCipTarget(); + // Read Tag Fragmented (0x52) → DINT 42. + target.ConnectedHandler = req => Task.FromResult( + req[0] == 0x52 ? [0xD2, 0x00, 0x00, 0x00, 0xC4, 0x00, 0x2A, 0x00, 0x00, 0x00] : target.ServiceReply(req)); + var pool = new DeviceSessionPool(); + var host = $"ab://127.0.0.1:{target.Port}/1,0"; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(host, AbCipPlcFamily.ControlLogix)], + RawTags = AbCipRawTags.From( + new AbCipTagDefinition("Bad", host, "Tëmp", AbCipDataType.DInt), + new AbCipTagDefinition("Good", host, "Good", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + Timeout = TimeSpan.FromSeconds(5), + }, "drv-570-native", new NativeAbCipTagFactory(pool)); + await drv.InitializeAsync("{}", CancellationToken.None); + try + { + for (var n = 0; n < Polls; n++) + (await drv.ReadAsync(["Bad"], CancellationToken.None)).Single().StatusCode + .ShouldBe(AbCipStatusMapper.BadConfigurationError); + target.ForwardOpens.ShouldBe(0); // nothing reached the device + + var good = (await drv.ReadAsync(["Good"], CancellationToken.None)).Single(); + good.StatusCode.ShouldBe(AbCipStatusMapper.Good); + good.Value.ShouldBe(42); + target.ForwardOpens.ShouldBe(1); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + finally + { + await drv.ShutdownAsync(CancellationToken.None); + } + } +#pragma warning restore OTOPCUA0001 +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTypeMismatchTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTypeMismatchTests.cs index d44485c9..a9dab1e5 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTypeMismatchTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipTypeMismatchTests.cs @@ -155,5 +155,92 @@ public sealed class AbCipTypeMismatchTests await drv.ShutdownAsync(CancellationToken.None); } } + + /// + /// Gitea #570 mistype churn — the path the I4 fix above did not reach. A controller that refuses the + /// EXISTENCE read itself with a type error (CIP 0xFF / 0x2107) made InitializeAsync throw, so the + /// driver disposed the runtime (the sole lease: Forward Close), opened the device backoff window, and + /// re-created the runtime (Forward Open) on the next poll — forever, throttled. One create and one + /// Forward Open across N polls, every poll BadTypeMismatch, health unchanged. + /// + [Fact] + public async Task A_native_type_refusal_on_the_existence_read_causes_no_runtime_or_connection_churn() + { + await using var target = new FakeCipTarget(); + // Read Tag Fragmented (0x52) → general status 0xFF, extended 0x2107 (type mismatch). + target.ConnectedHandler = req => Task.FromResult( + req[0] == 0x52 ? [0xD2, 0x00, 0xFF, 0x01, 0x07, 0x21] : target.ServiceReply(req)); + var pool = new DeviceSessionPool(); + var host = $"ab://127.0.0.1:{target.Port}/1,0"; + var creates = 0; + var native = new NativeAbCipTagFactory(pool); + var counting = new CountingFactory(p => { Interlocked.Increment(ref creates); return native.Create(p); }); + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(host, AbCipPlcFamily.ControlLogix)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("T", host, "T", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + Timeout = TimeSpan.FromSeconds(5), + }, "drv-570-churn", counting); + await drv.InitializeAsync("{}", CancellationToken.None); + var health = drv.GetHealth(); + try + { + for (var n = 0; n < Polls; n++) + (await drv.ReadAsync(["T"], CancellationToken.None)).Single().StatusCode + .ShouldBe(AbCipStatusMapper.BadTypeMismatch); + + Volatile.Read(ref creates).ShouldBe(1); + target.ForwardOpens.ShouldBe(1); + target.ForwardCloses.ShouldBe(0); + target.ConnectedRequests.ShouldBeGreaterThanOrEqualTo(Polls); // positive control: polls hit the wire + drv.GetHealth().ShouldBe(health); + } + finally + { + await drv.ShutdownAsync(CancellationToken.None); + } + } + + /// + /// The driver-side half of the same guarantee, independent of the native runtime: a runtime whose + /// InitializeAsync throws a type mismatch is kept, not disposed and re-created per poll. + /// + [Fact] + public async Task A_runtime_that_throws_a_type_mismatch_from_initialize_is_kept_not_recreated() + { + var creates = 0; + var mismatch = new AbTransportException(AbStatus.TypeMismatch, "REAL on the controller, DINT authored"); + var factory = new FakeAbCipTagFactory + { + Customise = p => + { + Interlocked.Increment(ref creates); + return new FakeAbCipTag(p) { ThrowOnInitialize = true, ThrowOnRead = true, Exception = mismatch }; + }, + }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Host)], + RawTags = AbCipRawTags.From(new AbCipTagDefinition("T", Host, "T", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "drv-570-fake", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + var health = drv.GetHealth(); + + for (var n = 0; n < Polls; n++) + (await drv.ReadAsync(["T"], CancellationToken.None)).Single().StatusCode + .ShouldBe(AbCipStatusMapper.BadTypeMismatch); + + Volatile.Read(ref creates).ShouldBe(1); + factory.Tags["T"].Disposed.ShouldBeFalse(); + factory.Tags["T"].ReadCount.ShouldBe(Polls - 1); // positive control: later polls reached the kept runtime + drv.GetHealth().ShouldBe(health); + } + + private sealed class CountingFactory(Func create) : IAbCipTagFactory + { + public IAbCipTagRuntime Create(AbCipTagCreateParams createParams) => create(createParams); + } #pragma warning restore OTOPCUA0001 } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipLeaseReleaseTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipLeaseReleaseTests.cs new file mode 100644 index 00000000..1203432a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipLeaseReleaseTests.cs @@ -0,0 +1,139 @@ +using System.Diagnostics; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Gitea #579: the native runtime's Dispose — which the driver calls under the tag's lock — used to +/// block for up to 2 s waiting for the last lease's Forward Close. It now releases in the background +/// (, the same shape as the native AbLegacy runtime), and +/// these tests pin both halves of why that is safe: the caller is not held, and a re-create still cannot +/// overlap the close or leak a connection. +/// +[Trait("Category", "Unit")] +public sealed class NativeAbCipLeaseReleaseTests +{ + private static readonly TimeSpan Guard = TimeSpan.FromSeconds(15); + private static CancellationToken Ct => TestContext.Current.CancellationToken; + + private static AbCipTagCreateParams Params(int port) => new( + Gateway: "127.0.0.1", + Port: port, + CipPath: "1,0", + LibplctagPlcAttribute: "controllogix", + TagName: "T", + Timeout: TimeSpan.FromSeconds(5), + AllowPacking: false, + ElementCount: 1, + IsArray: false, + AuthoredType: AbCipDataType.DInt, + UseConnectedMessaging: true); + + /// A target whose Read Tag Fragmented (0x52) answers DINT 42. + private static FakeCipTarget NewTarget() + { + var target = new FakeCipTarget(); + target.ConnectedHandler = req => Task.FromResult( + req[0] == 0x52 ? [0xD2, 0x00, 0x00, 0x00, 0xC4, 0x00, 0x2A, 0x00, 0x00, 0x00] : target.ServiceReply(req)); + return target; + } + + [Fact] + public async Task Release_under_the_tag_lock_returns_promptly_while_the_Forward_Close_is_still_pending() + { + await using var target = NewTarget(); + var closeHeld = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + target.ForwardCloseGate = () => closeHeld.Task; + var pool = new DeviceSessionPool(); + var factory = new NativeAbCipTagFactory(pool); + + var first = (NativeAbCipTagRuntime)factory.Create(Params(target.Port)); + await first.InitializeAsync(Ct); + target.ForwardOpens.ShouldBe(1); + + // The driver disposes under the per-tag lock; hold one and time the release inside it. + using var tagLock = new SemaphoreSlim(1, 1); + await tagLock.WaitAsync(Ct); + var sw = Stopwatch.StartNew(); + first.Dispose(); + sw.Stop(); + tagLock.Release(); + + // The old Dispose waited up to 2 s for exactly this close; it must not wait at all now. + sw.Elapsed.ShouldBeLessThan(TimeSpan.FromMilliseconds(500)); + await FakeCipTarget.Until(() => target.ForwardCloses == 1, Guard, "the Forward Close"); + (first.TeardownTask is null).ShouldBeFalse(); + first.TeardownTask!.IsCompleted.ShouldBeFalse(); // the close really is still pending + + // An immediate re-create (what the driver does after an evict) must NOT open a second session while + // the first one's Forward Close is in flight (review finding 22) — the pool holds it behind the close. + var second = (NativeAbCipTagRuntime)factory.Create(Params(target.Port)); + var init = second.InitializeAsync(Ct); + await Task.Delay(300, Ct); + target.ForwardOpens.ShouldBe(1); + init.IsCompleted.ShouldBeFalse(); + + closeHeld.SetResult(); + await first.TeardownTask.WaitAsync(Guard, Ct); + await init.WaitAsync(Guard, Ct); + target.ForwardOpens.ShouldBe(2); + second.GetStatus().ShouldBe((int)AbStatus.Ok); + second.DecodeValue(AbCipDataType.DInt, null).ShouldBe(42); + + second.Dispose(); + await second.TeardownTask!.WaitAsync(Guard, Ct); + target.ForwardCloses.ShouldBe(2); + } + + /// + /// The offline soak: N create → read → release cycles, each release fire-and-forget and each create + /// immediately after it — the evict/re-create rhythm at its tightest. Every connection opened is closed + /// (one Forward Open and one Forward Close per cycle, one TCP connection per cycle), no TCP connection is + /// left open on the target, and the pool ends empty. A clock that runs fast lets the 1 s Forward Open + /// throttle (press safety) pass every cycle without the test taking N seconds; the throttle is still + /// exercised. + /// + [Fact] + public async Task Repeated_create_and_release_cycles_leak_no_connection() + { + const int cycles = 25; + await using var target = NewTarget(); + var pool = new DeviceSessionPool(new FastForwardClock()); + var factory = new NativeAbCipTagFactory(pool); + + NativeAbCipTagRuntime? last = null; + for (var i = 0; i < cycles; i++) + { + var rt = (NativeAbCipTagRuntime)factory.Create(Params(target.Port)); + await rt.InitializeAsync(Ct).WaitAsync(Guard, Ct); + rt.DecodeValue(AbCipDataType.DInt, null).ShouldBe(42); + rt.Dispose(); // not awaited — the next cycle starts at once + last = rt; + } + + await last!.TeardownTask!.WaitAsync(Guard, Ct); + await FakeCipTarget.Until(() => target.Server.OpenConnections == 0, Guard, "every TCP connection to close"); + target.ForwardOpens.ShouldBe(cycles); + target.ForwardCloses.ShouldBe(cycles); + target.Server.Connections.ShouldBe(cycles); + pool.EntryCount.ShouldBe(0); + } + + /// + /// Real timers, but a monotonic clock that jumps 10 s on every read — so an interval measured between + /// two reads has always "elapsed". The Forward Open throttle reads elapsed time this way; the only other + /// reader, the keep-alive idle check, runs on a real-time timer and at worst sends a harmless keep-alive. + /// + private sealed class FastForwardClock : TimeProvider + { + private long _skew; + + public override long GetTimestamp() => + System.GetTimestamp() + Interlocked.Add(ref _skew, 10 * System.TimestampFrequency); + + public override long TimestampFrequency => System.TimestampFrequency; + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs index 2e6e6b5d..27a14895 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs @@ -327,6 +327,12 @@ public sealed class NativeAbCipTagRuntimeTests [InlineData(AbCipDataType.Int, "C400", "2A000000")] // different width [InlineData(AbCipDataType.Real, "D300", "2A000000")] // integer ↔ real, same width [InlineData(AbCipDataType.UDInt, "CA00", "0000803F")] // real ↔ integer, same width + // Gitea #570: the Micro800 bit-string codes are integers of a known width, not "unknown, exempt". + [InlineData(AbCipDataType.Real, "D400", "0000803F00000000")] // REAL ← Micro800 LWORD + [InlineData(AbCipDataType.LReal, "D400", "000000000000F03F")] // LREAL ← Micro800 LWORD (same width) + [InlineData(AbCipDataType.DInt, "D400", "2A00000000000000")] // DINT ← Micro800 LWORD (narrower) + [InlineData(AbCipDataType.Real, "D200", "2A00")] // REAL ← Micro800 WORD + [InlineData(AbCipDataType.Bool, "D100", "01")] // BOOL ← Micro800 BYTE public async Task Different_widths_and_integer_real_pairs_still_mismatch( AbCipDataType authored, string typeHex, string dataHex) { @@ -334,17 +340,93 @@ public sealed class NativeAbCipTagRuntimeTests rt.GetStatus().ShouldBe((int)AbStatus.TypeMismatch); } - [Fact] - public async Task A_malformed_tag_name_never_throws_from_Create_and_surfaces_on_Initialize_without_IO() + /// + /// Gitea #570: a name the encoder refuses is a CONFIGURATION error on this tag + /// (, which the driver reports as BadConfigurationError + /// without touching the device's backoff), raised before any I/O — not a raw encoder exception the + /// driver would count as a failed connect. + /// + [Theory] + [InlineData("Arr[abc]")] // non-numeric subscript + [InlineData("Arr[1")] // unterminated subscript + [InlineData("Arr[99999999999]")] // subscript past uint + [InlineData("Tëmp")] // not ASCII — used to go out as "T?mp" + [InlineData("A..B")] // empty member + public async Task A_malformed_tag_name_never_throws_from_Create_and_is_a_configuration_error_on_Initialize_without_IO( + string name) { var ch = new FakeCipChannel(); - var p = Params("Arr[abc]", AbCipDataType.DInt); + var p = Params(name, AbCipDataType.DInt); using var created = new NativeAbCipTagFactory().Create(p); // must not throw using var rt = NativeAbCipTagRuntime.ForTest(ch, p); - await Should.ThrowAsync(() => rt.InitializeAsync(CancellationToken.None)); + await Should.ThrowAsync(() => rt.InitializeAsync(CancellationToken.None)); ch.SentCount.ShouldBe(0); } + [Fact] + public async Task An_over_long_member_name_is_a_configuration_error_not_a_wrapped_length_byte() + { + var ch = new FakeCipChannel(); + using var rt = NativeAbCipTagRuntime.ForTest(ch, Params(new string('A', 256), AbCipDataType.DInt)); + await Should.ThrowAsync(() => rt.InitializeAsync(CancellationToken.None)); + ch.SentCount.ShouldBe(0); + } + + /// + /// Gitea #570 mistype churn: a controller that answers the existence read with a type refusal (CIP + /// 0xFF / 0x2107) has proved the tag exists. Initialize completes with the status instead of throwing + /// — a throw makes the driver dispose and re-create the runtime on every poll — and the next read + /// reports the refusal as before. + /// + [Fact] + public async Task A_type_refusal_on_the_existence_read_completes_initialize_with_the_status() + { + var ch = new FakeCipChannel { Respond = _ => Task.FromResult(Convert.FromHexString("D200FF010721")) }; + using var rt = NativeAbCipTagRuntime.ForTest(ch, Params("T", AbCipDataType.DInt)); + + await rt.InitializeAsync(CancellationToken.None); + rt.GetStatus().ShouldBe((int)AbStatus.TypeMismatch); + + (await Should.ThrowAsync(() => rt.ReadAsync(CancellationToken.None))) + .Status.ShouldBe(AbStatus.TypeMismatch); + ch.SentCount.ShouldBe(2); // positive control: both reads reached the wire + } + + /// + /// Gitea #570: the built-in Logix STRING is 88 bytes on the wire, but 2 of them are alignment padding — + /// its DATA holds 82 characters, not 84. 83 characters must be refused, 82 accepted, and the write + /// still carries the whole 88-byte structure (padding zeroed) as libplctag sends it. + /// + [Fact] + public async Task A_built_in_STRING_holds_82_characters_not_the_84_its_padding_would_allow() + { + var ch = Replying("A002CE0F", "00000000" + new string('0', 2 * 84)); + using var rt = await ReadAsync(ch, Params("S", AbCipDataType.String)); + ch.Respond = req => Task.FromResult(FakeCipChannel.OkReply(req)); + var sentBefore = ch.SentCount; + + rt.EncodeValue(AbCipDataType.String, null, new string('x', 83)); + (await Should.ThrowAsync(() => rt.WriteAsync(CancellationToken.None))) + .Status.ShouldBe(AbStatus.TooLarge); + ch.SentCount.ShouldBe(sentBefore); + + var full = new string('x', 82); + rt.EncodeValue(AbCipDataType.String, null, full); + await rt.WriteAsync(CancellationToken.None); + var expected = "4D02" + "91015300" + "A002" + "CE0F" + "0100" + + "52000000" + Convert.ToHexString(Encoding.ASCII.GetBytes(full)) + "0000"; + ch.Sent[^1].ShouldBe(Convert.FromHexString(expected)); + } + + [Fact] + public async Task A_built_in_STRING_never_decodes_its_alignment_padding_as_characters() + { + // LEN claims 84; DATA is 82 'A's then the 2 pad bytes (here non-zero, as garbage would be). + var data = "54000000" + Convert.ToHexString(Encoding.ASCII.GetBytes(new string('A', 82))) + "4242"; + using var rt = await ReadAsync(Replying("A002CE0F", data), Params("S", AbCipDataType.String)); + rt.DecodeValue(AbCipDataType.String, null).ShouldBe(new string('A', 82)); + } + [Fact] public async Task A_refused_write_stores_the_status_and_rethrows() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs index 40320025..8d5185fe 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs @@ -630,4 +630,58 @@ public sealed class NativeAbLegacyTagRuntimeTests runtime.DecodeValue(AbLegacyDataType.Int, null).ShouldBe(4321); server.HandlerErrors.ShouldBeEmpty(); } + + /// + /// Gitea #579 offline soak: N create → read → release cycles over a pooled connection, each release + /// fire-and-forget (, the shape the native AbCip runtime + /// now shares) and each create immediately after it. Every TCP connection opened is closed, one per + /// cycle, and the pool ends empty. A fast-running monotonic clock lets the 1 s open throttle pass each + /// cycle without the test taking N seconds; the throttle is still exercised. + /// + [Fact] + public async Task Repeated_create_and_release_cycles_leak_no_connection() + { + const int cycles = 25; + byte[] fixedId = [0x07, 0x3D, 0xF3, 0x45, 0x43, 0x50, 0x21]; + await using var server = new FakeEipServer { ZeroReplyContext = true }; + server.Handler = req => + { + if (FakeEipServer.Command(req) != Ab.Transport.Eip.EncapCommand.SendRRData) return null; + var cip = FakeEipServer.RequestCip(req); + byte[] reply = [0xCB, 0x00, 0x00, 0x00, .. fixedId, 0x4F, 0x00, cip[15], cip[16], 0xE1, 0x10]; + return FakeEipServer.RRReply(req, reply); + }; + var pool = new DeviceSessionPool(new FastForwardClock()); + var p = new AbLegacyTagCreateParams("127.0.0.1", server.Port, "1,0", "slc500", "N7:0", TimeSpan.FromSeconds(5)); + var ct = TestContext.Current.CancellationToken; + + for (var i = 0; i < cycles; i++) + { + var runtime = new NativeAbLegacyTagRuntime(p, pool); + await runtime.InitializeAsync(ct); + await runtime.ReadAsync(ct); + runtime.GetStatus().ShouldBe(0); + runtime.DecodeValue(AbLegacyDataType.Int, null).ShouldBe(4321); + runtime.Dispose(); // not awaited — the next cycle starts at once + } + + var giveUp = DateTime.UtcNow + TimeSpan.FromSeconds(15); + while ((server.OpenConnections != 0 || pool.EntryCount != 0) && DateTime.UtcNow < giveUp) + await Task.Delay(5, ct); + server.OpenConnections.ShouldBe(0); + server.Connections.ShouldBe(cycles); + pool.EntryCount.ShouldBe(0); + server.HandlerErrors.ShouldBeEmpty(); + } + + /// Real timers; a monotonic clock that jumps 10 s per read, so the open throttle never waits. + private sealed class FastForwardClock : TimeProvider + { + private long _skew; + + public override long GetTimestamp() => + System.GetTimestamp() + Interlocked.Add(ref _skew, 10 * System.TimestampFrequency); + + public override long TimestampFrequency => System.TimestampFrequency; + } }