diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagOrphanRelease.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagOrphanRelease.cs index d68d3cb2..11b756a9 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagOrphanRelease.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagOrphanRelease.cs @@ -23,7 +23,8 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip; /// The wrapper exposes no handle, so it is read by reflection from the two private fields of the pinned /// 1.5.2 wrapper (Tag._tag, NativeTagWrapper.nativeTagHandle). If a future wrapper renames them the /// release cannot run; the tag is then rooted for the life of the process instead, which keeps its delegate -/// alive (no crash, a bounded leak) — and LibplctagOrphanReleaseTests fails, so the bump is noticed. +/// alive. That is no crash but an UNBOUNDED leak — one native tag (holding its libplctag session open) per failed +/// create, and the data path re-creates a NotFound tag on every read — so LibplctagOrphanReleaseTests fails, so the bump is noticed. /// internal static class LibplctagOrphanRelease { @@ -38,7 +39,10 @@ internal static class LibplctagOrphanRelease private static int _released; /// True when the pinned wrapper's private fields were found, so a failed tag can be destroyed. - internal static bool CanReadHandles => HandleField is not null; + internal static bool CanReadHandles => HandleField is not null && !ForceRootingForTest; + + /// Test seam (review r1 M1): take the rooting fallback as if the wrapper's fields had been renamed. + internal static bool ForceRootingForTest { get; set; } /// Orphaned native tags destroyed so far (process-wide; diagnostics and tests). internal static int Released => Volatile.Read(ref _released); @@ -50,6 +54,12 @@ internal static class LibplctagOrphanRelease internal static int HandleOf(Tag tag) => HandleField is not null && WrapperField!.GetValue(tag) is { } wrapper ? (int)HandleField.GetValue(wrapper)! : 0; + /// Clears the wrapper's handle after a destroy, so a repeat release is a no-op (review r1 M2). + private static void ClearHandle(Tag tag) + { + if (HandleField is not null && WrapperField!.GetValue(tag) is { } wrapper) HandleField.SetValue(wrapper, 0); + } + /// /// Runs a libplctag operation that may initialise (InitializeAsync, or a first /// Read/Write, which initialise on demand). If it fails and leaves the tag uninitialised, the native tag it @@ -103,6 +113,7 @@ internal static class LibplctagOrphanRelease var status = (Status)plctag.plc_tag_status(handle); plctag.plc_tag_unregister_callback(handle); plctag.plc_tag_destroy(handle); + ClearHandle(tag); Interlocked.Increment(ref _released); return status; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs index 6f509db9..d1c4436b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs @@ -20,6 +20,9 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime /// private Status? _releasedStatus; + /// The native handle a failed initialisation destroyed (0 if none); the wrapper's own field is cleared. + private int _releasedHandle; + /// Initializes a new instance of the LibplctagTagRuntime class. /// The tag creation parameters. public LibplctagTagRuntime(AbCipTagCreateParams p) @@ -70,16 +73,26 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime { await LibplctagStatusTranslation.Wrap(op).ConfigureAwait(false); _releasedStatus = null; + _releasedHandle = 0; } catch { - if (LibplctagOrphanRelease.ReleaseIfOrphaned(_tag) is { } status) _releasedStatus = status; + var handle = LibplctagOrphanRelease.HandleOf(_tag); + if (LibplctagOrphanRelease.ReleaseIfOrphaned(_tag) is { } status) + { + _releasedStatus = status; + _releasedHandle = handle; + } throw; } } - /// The native libplctag handle this runtime's tag holds or last held (0 if none was created). Tests only. - internal int NativeHandle => LibplctagOrphanRelease.HandleOf(_tag); + /// The native libplctag handle this runtime's tag holds, or the one a failed initialisation destroyed + /// (0 if none was created). Tests only. + internal int NativeHandle => _releasedHandle != 0 ? _releasedHandle : LibplctagOrphanRelease.HandleOf(_tag); + + /// The handle the libplctag wrapper itself still holds (0 once a release cleared it). Tests only. + internal int WrapperHandle => LibplctagOrphanRelease.HandleOf(_tag); /// public int GetStatus() => diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs index e99b097c..4067d0b2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs @@ -19,6 +19,9 @@ internal sealed class LibplctagLegacyTagRuntime : IAbLegacyTagRuntime /// private Status? _releasedStatus; + /// The native handle a failed initialisation destroyed (0 if none); the wrapper's own field is cleared. + private int _releasedHandle; + /// Initializes a new instance of the class. /// The parameters for tag creation. public LibplctagLegacyTagRuntime(AbLegacyTagCreateParams p) @@ -66,16 +69,26 @@ internal sealed class LibplctagLegacyTagRuntime : IAbLegacyTagRuntime { await LibplctagLegacyStatusTranslation.Wrap(op).ConfigureAwait(false); _releasedStatus = null; + _releasedHandle = 0; } catch { - if (LibplctagOrphanRelease.ReleaseIfOrphaned(_tag) is { } status) _releasedStatus = status; + var handle = LibplctagOrphanRelease.HandleOf(_tag); + if (LibplctagOrphanRelease.ReleaseIfOrphaned(_tag) is { } status) + { + _releasedStatus = status; + _releasedHandle = handle; + } throw; } } - /// The native libplctag handle this runtime's tag holds or last held (0 if none was created). Tests only. - internal int NativeHandle => LibplctagOrphanRelease.HandleOf(_tag); + /// The native libplctag handle this runtime's tag holds, or the one a failed initialisation destroyed + /// (0 if none was created). Tests only. + internal int NativeHandle => _releasedHandle != 0 ? _releasedHandle : LibplctagOrphanRelease.HandleOf(_tag); + + /// The handle the libplctag wrapper itself still holds (0 once a release cleared it). Tests only. + internal int WrapperHandle => LibplctagOrphanRelease.HandleOf(_tag); /// public int GetStatus() => diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagOrphanRelease.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagOrphanRelease.cs index 642ba5f3..ffaf5274 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagOrphanRelease.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagOrphanRelease.cs @@ -23,7 +23,8 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy; /// The wrapper exposes no handle, so it is read by reflection from the two private fields of the pinned /// 1.5.2 wrapper (Tag._tag, NativeTagWrapper.nativeTagHandle). If a future wrapper renames them the /// release cannot run; the tag is then rooted for the life of the process instead, which keeps its delegate -/// alive (no crash, a bounded leak) — and LibplctagLegacyOrphanReleaseTests fails, so the bump is noticed. +/// alive. That is no crash but an UNBOUNDED leak — one native tag (holding its libplctag session open) per failed +/// create, and the data path re-creates a NotFound tag on every read — so LibplctagLegacyOrphanReleaseTests fails, so the bump is noticed. /// internal static class LibplctagOrphanRelease { @@ -38,7 +39,10 @@ internal static class LibplctagOrphanRelease private static int _released; /// True when the pinned wrapper's private fields were found, so a failed tag can be destroyed. - internal static bool CanReadHandles => HandleField is not null; + internal static bool CanReadHandles => HandleField is not null && !ForceRootingForTest; + + /// Test seam (review r1 M1): take the rooting fallback as if the wrapper's fields had been renamed. + internal static bool ForceRootingForTest { get; set; } /// Orphaned native tags destroyed so far (process-wide; diagnostics and tests). internal static int Released => Volatile.Read(ref _released); @@ -50,6 +54,12 @@ internal static class LibplctagOrphanRelease internal static int HandleOf(Tag tag) => HandleField is not null && WrapperField!.GetValue(tag) is { } wrapper ? (int)HandleField.GetValue(wrapper)! : 0; + /// Clears the wrapper's handle after a destroy, so a repeat release is a no-op (review r1 M2). + private static void ClearHandle(Tag tag) + { + if (HandleField is not null && WrapperField!.GetValue(tag) is { } wrapper) HandleField.SetValue(wrapper, 0); + } + /// /// Runs a libplctag operation that may initialise (InitializeAsync, or a first /// Read/Write, which initialise on demand). If it fails and leaves the tag uninitialised, the native tag it @@ -103,6 +113,7 @@ internal static class LibplctagOrphanRelease var status = (Status)plctag.plc_tag_status(handle); plctag.plc_tag_unregister_callback(handle); plctag.plc_tag_destroy(handle); + ClearHandle(tag); Interlocked.Increment(ref _released); return status; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/LibplctagOrphanReleaseTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/LibplctagOrphanReleaseTests.cs index 52e51759..55dd52ad 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/LibplctagOrphanReleaseTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/LibplctagOrphanReleaseTests.cs @@ -14,8 +14,20 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; /// AbCip.IntegrationTests/LibplctagOrphanReleaseLiveTests; this offline one is the refused-connection /// variant, whose native tag otherwise keeps reconnecting in the background for the life of the process. /// +[CollectionDefinition(Name, DisableParallelization = true)] +public sealed class LibplctagOrphanReleaseCollection +{ + /// The release counters and the rooting seam are process-wide: nothing else may run alongside. + public const string Name = "libplctag orphan release"; +} + +[Collection(LibplctagOrphanReleaseCollection.Name)] public sealed class LibplctagOrphanReleaseTests { + private static AbCipTagCreateParams Refused(string tagName = "TestDINT") => new( + Gateway: "127.0.0.1", Port: 1, CipPath: "1,0", LibplctagPlcAttribute: "controllogix", + TagName: tagName, Timeout: TimeSpan.FromMilliseconds(300)); + [Fact] public void The_pinned_libplctag_wrapper_still_exposes_the_native_handle() { @@ -39,6 +51,8 @@ public sealed class LibplctagOrphanReleaseTests NativeTagExists(handle).ShouldBeFalse("the failed tag's native handle must be destroyed, not orphaned"); // The status the tag held is kept, not replaced by the destroyed handle's NotFound. rt.GetStatus().ShouldNotBe((int)AbStatus.NotFound); + // Review r1 M2: the wrapper no longer holds the destroyed id, so a repeat release cannot touch it. + rt.WrapperHandle.ShouldBe(0); } finally { @@ -46,6 +60,33 @@ public sealed class LibplctagOrphanReleaseTests } } + /// + /// Review r1 M1: when the handle cannot be read (a renamed wrapper field), the failed tag is ROOTED — its callback + /// delegate can then never be collected — and nothing is destroyed. + /// + [Fact] + public async Task Without_a_readable_handle_the_failed_tag_is_rooted_instead() + { + var rooted = LibplctagOrphanRelease.RootedCount; + var released = LibplctagOrphanRelease.Released; + var rt = new LibplctagTagRuntime(Refused()); + LibplctagOrphanRelease.ForceRootingForTest = true; + try + { + await Should.ThrowAsync(() => rt.InitializeAsync(CancellationToken.None)); + LibplctagOrphanRelease.RootedCount.ShouldBe(rooted + 1); + LibplctagOrphanRelease.Released.ShouldBe(released); + } + finally + { + LibplctagOrphanRelease.ForceRootingForTest = false; + // Clean up the native tag this test deliberately leaked (its callback was unregistered by the timeout). + var handle = rt.NativeHandle; + if (handle > 0) plctag.plc_tag_destroy(handle); + rt.Dispose(); + } + } + /// plc_tag_lock answers PLCTAG_ERR_NOT_FOUND for a handle the library no longer holds. internal static bool NativeTagExists(int handle) { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/LibplctagLegacyOrphanReleaseTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/LibplctagLegacyOrphanReleaseTests.cs index 412d95f8..8b34d284 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/LibplctagLegacyOrphanReleaseTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/LibplctagLegacyOrphanReleaseTests.cs @@ -31,6 +31,7 @@ public sealed class LibplctagLegacyOrphanReleaseTests if (exists) plctag.plc_tag_unlock(handle); exists.ShouldBeFalse("the failed tag's native handle must be destroyed, not orphaned"); rt.GetStatus().ShouldNotBe((int)AbStatus.NotFound); + rt.WrapperHandle.ShouldBe(0); // review r1 M2: a repeat release cannot touch the destroyed id } finally {