Skip to content

Commit

Permalink
Only remove callback when a tag handle was succesfully created. (#433)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

---------

Signed-off-by: timyhac <[email protected]>
  • Loading branch information
@timyhac
timyhac authored Jan 29, 2025
1 parent 386a09e commit 0fdbc61
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 59 deletions.
25 changes: 0 additions & 25 deletions src/libplctag.Tests/DisposeTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,30 +88,5 @@ public void Can_not_use_if_already_disposed()
Assert.Throws<ObjectDisposedException>(() => 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<INative>();
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<int>()), Times.Once);
}

}
}
84 changes: 50 additions & 34 deletions src/libplctag/Tag.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()
Expand All @@ -79,9 +82,16 @@ internal Tag(INative nativeMethods)
}

/// <summary>
/// True if <see cref="Initialize"/> or <see cref="InitializeAsync"/> has been called.
/// True if <see cref="Initialize"/> or <see cref="InitializeAsync"/> has completed succesfully.
/// </summary>
public bool IsInitialized => _isInitialized;
public bool IsInitialized
{
get
{
ThrowIfAlreadyDisposed();
return _isInitialized;
}
}

/// <summary>
/// <list type="table">
Expand Down Expand Up @@ -604,7 +614,21 @@ public uint? StringTotalLength
set => SetField(ref _stringTotalLength, value);
}


/// <summary>
/// [OPTIONAL | MODBUS-SPECIFIC]
/// The Modbus specification allows devices to queue up to 16 requests at once.
/// </summary>
///
/// <remarks>
/// 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.
/// </remarks>
public uint? MaxRequestsInFlight
{
get => GetField(ref _maxRequestsInFlight);
set => SetField(ref _maxRequestsInFlight, value);
}


public void Dispose()
Expand All @@ -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;
}
Expand Down Expand Up @@ -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
Expand All @@ -666,22 +697,6 @@ public void Initialize()
_isInitialized = true;
}

/// <summary>
/// [OPTIONAL | MODBUS-SPECIFIC]
/// The Modbus specification allows devices to queue up to 16 requests at once.
/// </summary>
///
/// <remarks>
/// 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.
/// </remarks>
public uint? MaxRequestsInFlight
{
get => GetField(ref _maxRequestsInFlight);
set => SetField(ref _maxRequestsInFlight, value);
}

/// <summary>
/// Creates the underlying data structures and references required before tag operations.
/// </summary>
Expand All @@ -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();
Expand All @@ -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
Expand All @@ -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);
}

Expand Down Expand Up @@ -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<TaskCompletionSource<Status>> createTasks = new ConcurrentStack<TaskCompletionSource<Status>>();
Expand Down

0 comments on commit 0fdbc61

Please sign in to comment.