diff --git a/code-reviews/Driver.AbLegacy/findings.md b/code-reviews/Driver.AbLegacy/findings.md index 8759f10..e855923 100644 --- a/code-reviews/Driver.AbLegacy/findings.md +++ b/code-reviews/Driver.AbLegacy/findings.md @@ -194,7 +194,7 @@ shared libplctag `Tag` handle is never touched by two threads at once. | Severity | Medium | | Category | Concurrency & thread safety | | Location | `AbLegacyDriver.cs:411-438`, `AbLegacyDriver.cs:386-409` | -| Status | Open | +| Status | Resolved | **Description:** `EnsureTagRuntimeAsync` and `EnsureParentRuntimeAsync` are check-then-act: `device.Runtimes.TryGetValue(...)` then, after `await @@ -209,7 +209,7 @@ corrupt internal state. `ParentRuntimes` has the identical pattern. `GetOrAdd`, or guard runtime creation under a per-device lock. Ensure the losing runtime of any race is disposed. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `Runtimes` and `ParentRuntimes` changed to `ConcurrentDictionary`; `EnsureTagRuntimeAsync` and `EnsureParentRuntimeAsync` now hold a per-key `GetCreationLock` semaphore around the double-checked create+initialize+store sequence so exactly one runtime is created per key and no race-loser is leaked. ### Driver.AbLegacy-008 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index 3bc5f74..3d94052 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -431,55 +431,84 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover private async Task EnsureParentRuntimeAsync( AbLegacyDriver.DeviceState device, string parentName, CancellationToken ct) { + // Fast path: runtime already cached. if (device.ParentRuntimes.TryGetValue(parentName, out var existing)) return existing; - var runtime = _tagFactory.Create(new AbLegacyTagCreateParams( - Gateway: device.ParsedAddress.Gateway, - Port: device.ParsedAddress.Port, - CipPath: device.ParsedAddress.CipPath, - LibplctagPlcAttribute: device.Profile.LibplctagPlcAttribute, - TagName: parentName, - Timeout: _options.Timeout)); + // Slow path: serialise creation per key so concurrent callers don't each create a + // runtime and one of them gets overwritten + leaked. Only one caller initialises; the + // others find the entry on the second TryGetValue inside the lock. + var creationLock = device.GetCreationLock($"parent:{parentName}"); + await creationLock.WaitAsync(ct).ConfigureAwait(false); try { - await runtime.InitializeAsync(ct).ConfigureAwait(false); + if (device.ParentRuntimes.TryGetValue(parentName, out existing)) return existing; + + var runtime = _tagFactory.Create(new AbLegacyTagCreateParams( + Gateway: device.ParsedAddress.Gateway, + Port: device.ParsedAddress.Port, + CipPath: device.ParsedAddress.CipPath, + LibplctagPlcAttribute: device.Profile.LibplctagPlcAttribute, + TagName: parentName, + Timeout: _options.Timeout)); + try + { + await runtime.InitializeAsync(ct).ConfigureAwait(false); + } + catch + { + runtime.Dispose(); + throw; + } + device.ParentRuntimes[parentName] = runtime; + return runtime; } - catch + finally { - runtime.Dispose(); - throw; + creationLock.Release(); } - device.ParentRuntimes[parentName] = runtime; - return runtime; } private async Task EnsureTagRuntimeAsync( DeviceState device, AbLegacyTagDefinition def, CancellationToken ct) { + // Fast path: runtime already cached. if (device.Runtimes.TryGetValue(def.Name, out var existing)) return existing; - var parsed = AbLegacyAddress.TryParse(def.Address) - ?? throw new InvalidOperationException( - $"AbLegacy tag '{def.Name}' has malformed Address '{def.Address}'."); - - var runtime = _tagFactory.Create(new AbLegacyTagCreateParams( - Gateway: device.ParsedAddress.Gateway, - Port: device.ParsedAddress.Port, - CipPath: device.ParsedAddress.CipPath, - LibplctagPlcAttribute: device.Profile.LibplctagPlcAttribute, - TagName: parsed.ToLibplctagName(), - Timeout: _options.Timeout)); + // Slow path: serialise creation per tag name so concurrent callers for the same tag + // (server read path + poll loop) don't both create a runtime and one gets leaked. + var creationLock = device.GetCreationLock($"tag:{def.Name}"); + await creationLock.WaitAsync(ct).ConfigureAwait(false); try { - await runtime.InitializeAsync(ct).ConfigureAwait(false); + if (device.Runtimes.TryGetValue(def.Name, out existing)) return existing; + + var parsed = AbLegacyAddress.TryParse(def.Address) + ?? throw new InvalidOperationException( + $"AbLegacy tag '{def.Name}' has malformed Address '{def.Address}'."); + + var runtime = _tagFactory.Create(new AbLegacyTagCreateParams( + Gateway: device.ParsedAddress.Gateway, + Port: device.ParsedAddress.Port, + CipPath: device.ParsedAddress.CipPath, + LibplctagPlcAttribute: device.Profile.LibplctagPlcAttribute, + TagName: parsed.ToLibplctagName(), + Timeout: _options.Timeout)); + try + { + await runtime.InitializeAsync(ct).ConfigureAwait(false); + } + catch + { + runtime.Dispose(); + throw; + } + device.Runtimes[def.Name] = runtime; + return runtime; } - catch + finally { - runtime.Dispose(); - throw; + creationLock.Release(); } - device.Runtimes[def.Name] = runtime; - return runtime; } public void Dispose() => DisposeAsync().AsTask().GetAwaiter().GetResult(); @@ -493,7 +522,16 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover public AbLegacyHostAddress ParsedAddress { get; } = parsedAddress; public AbLegacyDeviceOptions Options { get; } = options; public AbLegacyPlcFamilyProfile Profile { get; } = profile; - public Dictionary Runtimes { get; } = + + /// + /// Per-tag cached runtimes. + /// avoids the check-then-act race present on a plain Dictionary: two concurrent + /// EnsureTagRuntimeAsync callers for the same key both miss the lookup on a + /// plain dict and both create + store a runtime, leaking the loser. Access is guarded + /// by a per-key creation semaphore () so exactly one + /// runtime is created per tag name. + /// + public System.Collections.Concurrent.ConcurrentDictionary Runtimes { get; } = new(StringComparer.OrdinalIgnoreCase); /// @@ -501,9 +539,20 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover /// parent address (bit suffix stripped) — e.g. writes to N7:0/3 + N7:0/5 share a /// single parent runtime for N7:0. /// - public Dictionary ParentRuntimes { get; } = + public System.Collections.Concurrent.ConcurrentDictionary ParentRuntimes { get; } = new(StringComparer.OrdinalIgnoreCase); + /// + /// Per-key creation locks for and . + /// A caller holds this before the TryGetValue + Create + InitializeAsync + TryAdd + /// sequence so that a concurrent caller waits rather than creating a duplicate runtime + /// that would be leaked on . + /// + private readonly System.Collections.Concurrent.ConcurrentDictionary _creationLocks = new(StringComparer.OrdinalIgnoreCase); + + public SemaphoreSlim GetCreationLock(string key) => + _creationLocks.GetOrAdd(key, _ => new SemaphoreSlim(1, 1)); + private readonly System.Collections.Concurrent.ConcurrentDictionary _rmwLocks = new(); public SemaphoreSlim GetRmwLock(string parentName) =>