From 752814bd2359806c42e27a6e46be340fd35de3c5 Mon Sep 17 00:00:00 2001 From: timyhac Date: Sat, 25 Jan 2025 22:11:10 +1100 Subject: [PATCH 1/3] Only remove callback when a tag handle was succesfully created. --- src/libplctag.Tests/DisposeTests.cs | 25 --------- src/libplctag/Tag.cs | 79 ++++++++++++++++------------- 2 files changed, 44 insertions(+), 60 deletions(-) diff --git a/src/libplctag.Tests/DisposeTests.cs b/src/libplctag.Tests/DisposeTests.cs index 40a497f..809173d 100644 --- a/src/libplctag.Tests/DisposeTests.cs +++ b/src/libplctag.Tests/DisposeTests.cs @@ -88,30 +88,5 @@ public void Can_not_use_if_already_disposed() Assert.Throws(() => tag.GetStatus()); } - [Fact(Skip = "The finalizer is no longer called because the Mock is holding a reference to a callback defined on the Tag. This would not happen outside of unit tests.")] - public void Finalizer_calls_destroy() - { - - // See https://www.inversionofcontrol.co.uk/unit-testing-finalizers-in-csharp/ - - - // Arrange - var nativeTag = new Mock(); - Action dispose = () => - { - // This will go out of scope after dispose() is executed, so the garbage collector will be able to call the finalizer - var tag = new Tag(nativeTag.Object); - tag.Initialize(); - }; - - // Act - dispose(); - GC.Collect(0, GCCollectionMode.Forced); - GC.WaitForPendingFinalizers(); - - // Assert - nativeTag.Verify(m => m.plc_tag_destroy(It.IsAny()), Times.Once); - } - } } diff --git a/src/libplctag/Tag.cs b/src/libplctag/Tag.cs index b31e7b0..ec7749c 100644 --- a/src/libplctag/Tag.cs +++ b/src/libplctag/Tag.cs @@ -63,14 +63,16 @@ public sealed class Tag : IDisposable private uint? _stringPadBytes; private uint? _stringTotalLength; - public Tag() + public Tag() : this(new Native()) { - _native = new Native(); } internal Tag(INative nativeMethods) { _native = nativeMethods; + + // Need to keep a reference to the delegate in memory so it doesn't get garbage collected + coreLibCallbackFuncExDelegate = new callback_func_ex(coreLibEventCallback); } ~Tag() @@ -79,9 +81,16 @@ internal Tag(INative nativeMethods) } /// - /// True if or has been called. + /// True if or has completed succesfully. /// - public bool IsInitialized => _isInitialized; + public bool IsInitialized + { + get + { + ThrowIfAlreadyDisposed(); + return _isInitialized; + } + } /// /// @@ -604,7 +613,21 @@ public uint? StringTotalLength set => SetField(ref _stringTotalLength, value); } - + /// + /// [OPTIONAL | MODBUS-SPECIFIC] + /// The Modbus specification allows devices to queue up to 16 requests at once. + /// + /// + /// + /// The default is 1 and the maximum is 16. + /// This allows the host to send multiple requests without waiting for the device to respond. + /// Not all devices support up to 16 requests in flight. + /// + public uint? MaxRequestsInFlight + { + get => GetField(ref _maxRequestsInFlight); + set => SetField(ref _maxRequestsInFlight, value); + } public void Dispose() @@ -613,10 +636,11 @@ public void Dispose() return; if (_isInitialized) - RemoveEventsAndRemoveCallback(); - - var result = (Status)_native.plc_tag_destroy(nativeTagHandle); - ThrowIfStatusNotOk(result); + { + RemoveCallback(); + _native.plc_tag_destroy(nativeTagHandle); + RemoveEvents(); + } _isDisposed = true; } @@ -654,7 +678,7 @@ public void Initialize() var result = _native.plc_tag_create_ex(attributeString, coreLibCallbackFuncExDelegate, IntPtr.Zero, millisecondTimeout); if (result < 0) { - RemoveEventsAndRemoveCallback(); + RemoveEvents(); throw new LibPlcTagException((Status)result); } else @@ -666,22 +690,6 @@ public void Initialize() _isInitialized = true; } - /// - /// [OPTIONAL | MODBUS-SPECIFIC] - /// The Modbus specification allows devices to queue up to 16 requests at once. - /// - /// - /// - /// The default is 1 and the maximum is 16. - /// This allows the host to send multiple requests without waiting for the device to respond. - /// Not all devices support up to 16 requests in flight. - /// - public uint? MaxRequestsInFlight - { - get => GetField(ref _maxRequestsInFlight); - set => SetField(ref _maxRequestsInFlight, value); - } - /// /// Creates the underlying data structures and references required before tag operations. /// @@ -706,7 +714,8 @@ public async Task InitializeAsync(CancellationToken token = default) if (createTasks.TryPop(out var createTask)) { Abort(); - RemoveEventsAndRemoveCallback(); + RemoveCallback(); + RemoveEvents(); if (token.IsCancellationRequested) createTask.SetCanceled(); @@ -727,7 +736,7 @@ public async Task InitializeAsync(CancellationToken token = default) // Something went wrong locally if (result < 0) { - RemoveEventsAndRemoveCallback(); + RemoveEvents(); throw new LibPlcTagException((Status)result); } else @@ -744,7 +753,8 @@ public async Task InitializeAsync(CancellationToken token = default) // On error, tear down and throw if(createTask.Task.Result != Status.Ok) { - RemoveEventsAndRemoveCallback(); + RemoveCallback(); + RemoveEvents(); throw new LibPlcTagException(createTask.Task.Result); } @@ -1206,22 +1216,21 @@ void SetUpEvents() WriteCompleted += WriteTaskCompleter; Created += CreatedTaskCompleter; - // Need to keep a reference to the delegate in memory so it doesn't get garbage collected - coreLibCallbackFuncExDelegate = new callback_func_ex(coreLibEventCallback); } - void RemoveEventsAndRemoveCallback() + void RemoveEvents() { // Used to finalize the read/write task completion sources ReadCompleted -= ReadTaskCompleter; WriteCompleted -= WriteTaskCompleter; Created -= CreatedTaskCompleter; + } - var callbackRemovalResult = (Status)_native.plc_tag_unregister_callback(nativeTagHandle); - ThrowIfStatusNotOk(callbackRemovalResult); - + Status RemoveCallback() + { + return (Status)_native.plc_tag_unregister_callback(nativeTagHandle); } private readonly ConcurrentStack> createTasks = new ConcurrentStack>(); From 403a54fc08788043808f7388ba484e3b0e5b3326 Mon Sep 17 00:00:00 2001 From: timyhac Date: Sun, 26 Jan 2025 15:02:49 +1100 Subject: [PATCH 2/3] Add asserts to catch errors in `Dispose()` during development --- src/libplctag/Tag.cs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/libplctag/Tag.cs b/src/libplctag/Tag.cs index ec7749c..1db42e2 100644 --- a/src/libplctag/Tag.cs +++ b/src/libplctag/Tag.cs @@ -8,6 +8,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Runtime.CompilerServices; using System.Text; @@ -637,8 +638,14 @@ public void Dispose() if (_isInitialized) { - RemoveCallback(); - _native.plc_tag_destroy(nativeTagHandle); + var removeCallbackResult = RemoveCallback(); + var destroyResult = (Status)_native.plc_tag_destroy(nativeTagHandle); + + // These should always succeed unless bugs exist in the core library or this wrapper + // If these calls fail, we cannot recover so ignore except during development + Debug.Assert(removeCallbackResult == Status.Ok); + Debug.Assert(destroyResult == Status.Ok); + RemoveEvents(); } From 104aada0de6e5b8ed1a39a7b62030576fad62b8d Mon Sep 17 00:00:00 2001 From: timyhac Date: Tue, 28 Jan 2025 06:41:37 +1100 Subject: [PATCH 3/3] Adjust phrasing and location of Dispose() comments Signed-off-by: timyhac --- src/libplctag/Tag.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libplctag/Tag.cs b/src/libplctag/Tag.cs index 1db42e2..3c65a6b 100644 --- a/src/libplctag/Tag.cs +++ b/src/libplctag/Tag.cs @@ -638,11 +638,11 @@ public void Dispose() if (_isInitialized) { + // These should always succeed unless bugs exist in this wrapper or the core library var removeCallbackResult = RemoveCallback(); var destroyResult = (Status)_native.plc_tag_destroy(nativeTagHandle); - // These should always succeed unless bugs exist in the core library or this wrapper - // If these calls fail, we cannot recover so ignore except during development + // However, we cannot recover if they do fail, so ignore except during development Debug.Assert(removeCallbackResult == Status.Ok); Debug.Assert(destroyResult == Status.Ok);