Remove callbacks upon initialization failure (#420)

* Add fixes and test for Github Issue #418 * Adjust initialization disposal logic to ensure callbacks are removed upon failure. * Remove integer return value from Shutdown() to align with core library. * We have to retrieve the Status because `plc_tag_create_ex` returns either an error (<0) or a handle according to the core library documentation.
libplctag · Oct 9, 2024 · 149b062 · 149b062
1 parent a4ebff5
commit 149b062
Show file tree

Hide file tree

Showing 7 changed files with 77 additions and 13 deletions.
diff --git a/src/libplctag.NativeImport/NativeMethods.cs b/src/libplctag.NativeImport/NativeMethods.cs
@@ -59,7 +59,7 @@ static NativeMethods()
 
 
         [DllImport(DLL_NAME, EntryPoint = nameof(plc_tag_shutdown), CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]
-        public static extern int plc_tag_shutdown();
+        public static extern void plc_tag_shutdown();
 
 
         [DllImport(DLL_NAME, EntryPoint = nameof(plc_tag_register_callback), CallingConvention = CallingConvention.Cdecl, ExactSpelling = true)]

diff --git a/src/libplctag.NativeImport/plctag.cs b/src/libplctag.NativeImport/plctag.cs
@@ -50,9 +50,9 @@ public static int plc_tag_destroy(Int32 tag)
             return NativeMethods.plc_tag_destroy(tag);
         }
 
-        public static int plc_tag_shutdown()
+        public static void plc_tag_shutdown()
         {
-            return NativeMethods.plc_tag_shutdown();
+            NativeMethods.plc_tag_shutdown();
         }
 
         public static int plc_tag_register_callback(Int32 tag_id, callback_func func)

diff --git a/src/libplctag.Tests/AsyncTests.cs b/src/libplctag.Tests/AsyncTests.cs
@@ -133,7 +133,7 @@ Mock<INative> GetMock()
                     callback = callbackFunc;
                     await Task.Delay(REALISTIC_LATENCY_FOR_CREATE);
                     status = Status.Ok;
-                    callback?.Invoke(tagId, (int)NativeImport.EVENT_CODES.PLCTAG_EVENT_CREATED, (int)NativeImport.STATUS_CODES.PLCTAG_STATUS_OK, IntPtr.Zero);
+                    callback?.Invoke(tagId, (int)NativeImport.EVENT.PLCTAG_EVENT_CREATED, (int)NativeImport.STATUS.PLCTAG_STATUS_OK, IntPtr.Zero);
                 });
 
             // ... as well as when a read call occurs
@@ -142,10 +142,10 @@ Mock<INative> GetMock()
                 .Callback<int, int>(async (tagId, timeout) =>
                 {
                     status = Status.Pending;
-                    callback?.Invoke(tagId, (int)NativeImport.EVENT_CODES.PLCTAG_EVENT_READ_STARTED, (int)NativeImport.STATUS_CODES.PLCTAG_STATUS_OK, IntPtr.Zero);
+                    callback?.Invoke(tagId, (int)NativeImport.EVENT.PLCTAG_EVENT_READ_STARTED, (int)NativeImport.STATUS.PLCTAG_STATUS_OK, IntPtr.Zero);
                     await Task.Delay(REALISTIC_LATENCY_FOR_READ);
                     status = Status.Ok;
-                    callback?.Invoke(tagId, (int)NativeImport.EVENT_CODES.PLCTAG_EVENT_READ_COMPLETED, (int)NativeImport.STATUS_CODES.PLCTAG_STATUS_OK, IntPtr.Zero);
+                    callback?.Invoke(tagId, (int)NativeImport.EVENT.PLCTAG_EVENT_READ_COMPLETED, (int)NativeImport.STATUS.PLCTAG_STATUS_OK, IntPtr.Zero);
                 });
 
             // the status was being tracked, so return it if asked

diff --git a/src/libplctag.Tests/DisposeTests.cs b/src/libplctag.Tests/DisposeTests.cs
@@ -8,6 +8,8 @@
 using System;
 using Xunit;
 using Moq;
+using System.Threading.Tasks;
+using System.Threading;
 
 namespace libplctag.Tests
 {
@@ -29,6 +31,49 @@ public void Destroy_is_called_if_initialized_and_disposed()
             nativeTag.Verify(m => m.plc_tag_destroy(It.IsAny<int>()), Times.Once);
         }
 
+        [Fact]
+        public async Task GithubIssue418()
+        {
+            // Arrange
+            const int tagId = 11;
+
+            NativeImport.plctag.callback_func_ex callback = null;
+            Status? status = null;
+
+            var nativeTag = new Mock<INative>();
+
+            nativeTag
+                .Setup(m => m.plc_tag_create_ex(It.IsAny<string>(), It.IsAny<NativeImport.plctag.callback_func_ex>(), It.IsAny<IntPtr>(), 0))
+                .Callback<string, NativeImport.plctag.callback_func_ex, IntPtr, int>(async (attributeString, callbackFunc, userData, timeout) =>
+                {
+                    status = Status.Pending;
+                    callback = callbackFunc;
+                    status = Status.ErrorNotFound;
+                    callback?.Invoke(tagId, (int)NativeImport.EVENT.PLCTAG_EVENT_CREATED, (int)NativeImport.STATUS.PLCTAG_ERR_NOT_FOUND, IntPtr.Zero);
+                });
+
+            // the status was being tracked, so return it if asked
+            nativeTag
+                .Setup(m => m.plc_tag_status(It.IsAny<int>()))
+                .Returns(() => (int)status.Value);
+
+            // Act
+            using(var tag = new Tag(nativeTag.Object))
+            {
+                try
+                {
+                    await tag.InitializeAsync();
+                }
+                catch (Exception e) when (e.Message == "ErrorNotFound") // we are expecting this exception
+                {
+                }
+            }
+
+            // Assert
+            LibPlcTag.Shutdown();
+
+        }
+
         [Fact]
         public void Can_not_use_if_already_disposed()
         {

diff --git a/src/libplctag/INative.cs b/src/libplctag/INative.cs
@@ -56,7 +56,7 @@ interface INative
         int plc_tag_set_uint32(int tag, int offset, uint val);
         int plc_tag_set_uint64(int tag, int offset, ulong val);
         int plc_tag_set_uint8(int tag, int offset, byte val);
-        int plc_tag_shutdown();
+        void plc_tag_shutdown();
         int plc_tag_status(int tag);
         int plc_tag_unlock(int tag);
         int plc_tag_unregister_callback(int tag_id);

diff --git a/src/libplctag/Native.cs b/src/libplctag/Native.cs
@@ -18,7 +18,7 @@ class Native : INative
         public Int32 plc_tag_create(string lpString, int timeout)                                                       => plctag.plc_tag_create(lpString, timeout);
         public Int32 plc_tag_create_ex(string lpString, callback_func_ex func, IntPtr userdata, int timeout)            => plctag.plc_tag_create_ex(lpString, func, userdata, timeout);
         public int plc_tag_destroy(Int32 tag)                                                                           => plctag.plc_tag_destroy(tag);
-        public int plc_tag_shutdown()                                                                                   => plctag.plc_tag_shutdown();
+        public void plc_tag_shutdown()                                                                                  => plctag.plc_tag_shutdown();
         public int plc_tag_register_callback(Int32 tag_id, callback_func func)                                          => plctag.plc_tag_register_callback(tag_id, func);
         public int plc_tag_unregister_callback(Int32 tag_id)                                                            => plctag.plc_tag_unregister_callback(tag_id);
         public int plc_tag_register_logger(log_callback_func func)                                                      => plctag.plc_tag_register_logger(func);

diff --git a/src/libplctag/Tag.cs b/src/libplctag/Tag.cs
@@ -595,11 +595,10 @@ public void Dispose()
                 return;
 
             if (_isInitialized)
-            {
                 RemoveEventsAndRemoveCallback();
-                var result = (Status)_native.plc_tag_destroy(nativeTagHandle);
-                ThrowIfStatusNotOk(result);
-            }
+
+            var result = (Status)_native.plc_tag_destroy(nativeTagHandle);
+            ThrowIfStatusNotOk(result);
 
             _isDisposed = true;
         }
@@ -636,9 +635,14 @@ public void Initialize()
 
             var result = _native.plc_tag_create_ex(attributeString, coreLibCallbackFuncExDelegate, IntPtr.Zero, millisecondTimeout);
             if (result < 0)
+            {
+                RemoveEventsAndRemoveCallback();
                 throw new LibPlcTagException((Status)result);
+            }
             else
+            {
                 nativeTagHandle = result;
+            }
 
 
             _isInitialized = true;
@@ -701,15 +705,30 @@ public async Task InitializeAsync(CancellationToken token = default)
                     var attributeString = GetAttributeString();
 
                     var result = _native.plc_tag_create_ex(attributeString, coreLibCallbackFuncExDelegate, IntPtr.Zero, TIMEOUT_VALUE_THAT_INDICATES_ASYNC_OPERATION);
+
+                    // Something went wrong locally
                     if (result < 0)
+                    {
+                        RemoveEventsAndRemoveCallback();
                         throw new LibPlcTagException((Status)result);
+                    }
                     else
+                    {
                         nativeTagHandle = result;
+                    }
 
+                    // Await while Pending
                     if(GetStatus() == Status.Pending)
+                    {
                         await createTask.Task.ConfigureAwait(false);
+                    }
 
-                    ThrowIfStatusNotOk(createTask.Task.Result);
+                    // On error, tear down and throw
+                    if(createTask.Task.Result != Status.Ok)
+                    {
+                        RemoveEventsAndRemoveCallback();
+                        throw new LibPlcTagException(createTask.Task.Result);
+                    }
 
                     _isInitialized = true;
                 }