dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(driver-abcip): resolve Medium code-review finding (Driver.AbCip-010)

Browse Source 
Add `EvictRuntime` helper that removes + disposes a stale
`ConcurrentDictionary` entry. Call it from `ReadSingleAsync`,
`ReadGroupAsync`, and `WriteAsync` on non-zero libplctag status and
transport exceptions so the next call for the same tag re-creates a
fresh handle — mirroring the probe loop's recreate-on-failure pattern.
Value-conversion exceptions (NotSupportedException, FormatException,
InvalidCastException, OverflowException) are not transport faults and
do not evict the handle.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 09:24:45 -04:00
parent a17de80cdb
commit 72728a5d45
2 changed files with 40 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
code-reviews/Driver.AbCip/findings.md
View File

@@ -168,13 +168,13 @@
| Severity | Medium |
| Category | Error handling & resilience |
| Location | `AbCipDriver.cs:621-648`, `AbCipDriver.cs:346-391` |
| Status | Open |
| Status | Resolved |
**Description:** Once `EnsureTagRuntimeAsync` successfully creates and initializes a `LibplctagTagRuntime`, that runtime is cached for the lifetime of the device and never re-created on failure. If the underlying native tag enters a permanently-bad state (connection dropped, controller rebooted, tag handle invalidated by a PLC program download), every subsequent `ReadAsync`/`WriteAsync` reuses the same dead handle and returns errors forever. The probe loop does tear down and recreate its runtime after a failure, but the read/write path has no equivalent recovery; only a full `ReinitializeAsync` (itself broken, see Driver.AbCip-001) clears the cache. The normal data path should self-heal from a transient handle fault without operator-driven reinitialize.
**Recommendation:** On a non-zero libplctag status or transport exception in `ReadSingleAsync`/`ReadGroupAsync`/`WriteAsync`, evict the offending runtime from `device.Runtimes` (and dispose it) so the next call re-creates and re-initializes it. Mirror the probe loop recreate-on-failure behavior.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — added `EvictRuntime(device, tagName)` helper that calls `ConcurrentDictionary.TryRemove` + disposes the evicted instance; called from `ReadSingleAsync`, `ReadGroupAsync`, and `WriteAsync` on both non-zero libplctag status and transport exceptions (type/value-conversion exceptions are not transport faults and do not evict). The next read/write for the affected tag re-runs `EnsureTagRuntimeAsync`, which creates and initializes a fresh handle, mirroring the probe loop's recreate-on-failure behaviour.
### Driver.AbCip-011

42
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs
View File

@@ -448,6 +448,11 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
var status = runtime.GetStatus();
if (status != 0)
{
// Evict the stale handle so the next call re-creates it (Driver.AbCip-010).
// A non-zero status can mean the controller dropped the connection or the tag
// handle became permanently invalid (e.g. after a PLC download). Evicting
// mirrors the probe loop's recreate-on-failure behaviour.
EvictRuntime(device, def.Name);
results[fb.OriginalIndex] = new DataValueSnapshot(null,
AbCipStatusMapper.MapLibplctagStatus(status), null, now);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead,
@@ -467,6 +472,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
}
catch (Exception ex)
{
// Transport exception — evict so the next read creates a fresh handle.
EvictRuntime(device, def.Name);
results[fb.OriginalIndex] = new DataValueSnapshot(null,
AbCipStatusMapper.BadCommunicationError, null, now);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message);
@@ -499,6 +506,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
var status = runtime.GetStatus();
if (status != 0)
{
EvictRuntime(device, parent.Name); // Driver.AbCip-010
var mapped = AbCipStatusMapper.MapLibplctagStatus(status);
StampGroupStatus(group, results, now, mapped);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead,
@@ -519,6 +527,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
}
catch (Exception ex)
{
EvictRuntime(device, parent.Name); // Driver.AbCip-010
StampGroupStatus(group, results, now, AbCipStatusMapper.BadCommunicationError);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message);
}
@@ -589,10 +598,16 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
await runtime.WriteAsync(cancellationToken).ConfigureAwait(false);
var status = runtime.GetStatus();
results[i] = new WriteResult(status == 0
? AbCipStatusMapper.Good
: AbCipStatusMapper.MapLibplctagStatus(status));
if (status == 0) _health = new DriverHealth(DriverState.Healthy, now, null);
if (status != 0)
{
EvictRuntime(device, def.Name); // Driver.AbCip-010
results[i] = new WriteResult(AbCipStatusMapper.MapLibplctagStatus(status));
}
else
{
results[i] = new WriteResult(AbCipStatusMapper.Good);
_health = new DriverHealth(DriverState.Healthy, now, null);
}
}
catch (OperationCanceledException)
{
@@ -600,11 +615,13 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
}
catch (NotSupportedException nse)
{
// Type/protocol error — not a transport fault; don't evict the handle.
results[i] = new WriteResult(AbCipStatusMapper.BadNotSupported);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, nse.Message);
}
catch (FormatException fe)
{
// Value conversion error — not a transport fault; don't evict.
results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, fe.Message);
}
@@ -620,6 +637,8 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
}
catch (Exception ex)
{
// Transport / wire error — evict so the next write creates a fresh handle.
EvictRuntime(device, def.Name); // Driver.AbCip-010
results[i] = new WriteResult(AbCipStatusMapper.BadCommunicationError);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message);
}
@@ -738,6 +757,21 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
return device.Runtimes[def.Name];
}
/// <summary>
/// Evict the runtime for <paramref name="tagName"/> from the device's cache and dispose
/// it so the next read/write call re-creates and re-initializes a fresh handle.
/// Called from <see cref="ReadSingleAsync"/>, <see cref="ReadGroupAsync"/>, and
/// <see cref="WriteAsync"/> after a non-zero libplctag status or transport exception —
/// mirroring the probe loop's recreate-on-failure behaviour (Driver.AbCip-010).
/// </summary>
private static void EvictRuntime(DeviceState device, string tagName)
{
if (device.Runtimes.TryRemove(tagName, out var stale))
{
try { stale.Dispose(); } catch { }
}
}
public DriverHealth GetHealth() => _health;
/// <summary>