From 0fdbc61016c5d5aca71fc36ae585f3dd168581ca Mon Sep 17 00:00:00 2001 From: timyhac Date: Wed, 29 Jan 2025 18:39:54 +1100 Subject: [PATCH] Only remove callback when a tag handle was succesfully created. (#433) * Only remove callback when a tag handle was succesfully created. * Add asserts to catch errors in `Dispose()` during development * Adjust phrasing and location of Dispose() comments Signed-off-by: timyhac --------- Signed-off-by: timyhac --- src/libplctag.Tests/DisposeTests.cs | 25 --------- src/libplctag/Tag.cs | 84 +++++++++++++++++------------ 2 files changed, 50 insertions(+), 59 deletions(-) diff --git a/src/libplctag.Tests/DisposeTests.cs b/src/libplctag.Tests/DisposeTests.cs index ea2f245..32532d2 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..3c65a6b 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; @@ -63,14 +64,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 +82,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 +614,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 +637,17 @@ public void Dispose() return; if (_isInitialized) - RemoveEventsAndRemoveCallback(); + { + // 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); - var result = (Status)_native.plc_tag_destroy(nativeTagHandle); - ThrowIfStatusNotOk(result); + // However, we cannot recover if they do fail, so ignore except during development + Debug.Assert(removeCallbackResult == Status.Ok); + Debug.Assert(destroyResult == Status.Ok); + + RemoveEvents(); + } _isDisposed = true; } @@ -654,7 +685,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 +697,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 +721,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 +743,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 +760,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 +1223,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>();