fix(twincat): dispose bit-RMW locks on shutdown + guard null parent read (review)

2026-06-17 12:01:55 -04:00
parent a73e20fb28
commit 340c145e87
2 changed files with 34 additions and 1 deletions
@@ -169,6 +169,10 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery
        _devices.Clear();
        _tagsByName.Clear();
        _resolver.Clear(); // drop transient equipment-tag parses so a config change re-parses
+        // Dispose + clear the per-parent-word RMW gates so ReinitializeAsync cycles don't orphan
+        // their SemaphoreSlim instances (each leaks a wait handle once contended).
+        foreach (var sem in _bitRmwLocks.Values) sem.Dispose();
+        _bitRmwLocks.Clear();
        _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null);
    }

@@ -319,9 +323,16 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery
                        {
                            results[i] = new WriteResult(readStatus);
                        }
+                        else if (parentValue is null)
+                        {
+                            // Good status but no value — treating null as 0 would write a zeroed
+                            // word and clear every bit set on the device. Surface a Bad status and
+                            // write nothing rather than corrupt the parent word.
+                            results[i] = new WriteResult(TwinCATStatusMapper.BadCommunicationError);
+                        }
                        else
                        {
-                            var word = Convert.ToUInt32(parentValue ?? 0u);
+                            var word = Convert.ToUInt32(parentValue);
                            var updated = Convert.ToBoolean(w.Value) ? word | (1u << bit) : word & ~(1u << bit);
                            var writeStatus = await client.WriteValueAsync(
                                parentPath, TwinCATDataType.UDInt, null, updated, cancellationToken).ConfigureAwait(false);
@@ -719,6 +730,10 @@ public sealed class TwinCATDriver : IDriver, IReadable, IWritable, ITagDiscovery
        _devices.Clear();
        _tagsByName.Clear();
        _resolver.Clear(); // drop transient equipment-tag parses so a config change re-parses
+        // Dispose + clear the per-parent-word RMW gates (mirrors ShutdownAsync) so the
+        // SemaphoreSlim instances aren't orphaned on disposal.
+        foreach (var sem in _bitRmwLocks.Values) sem.Dispose();
+        _bitRmwLocks.Clear();
        _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null);
    }

@@ -332,6 +332,24 @@ public sealed class TwinCATReadWriteTests
        factory.Clients[0].WriteLog.ShouldBeEmpty();
    }

+    /// <summary>A Good parent read that yields a null value must NOT zero the word — it surfaces
+    ///     a Bad status and writes nothing (treating null as 0 would clear every set bit).</summary>
+    [Fact]
+    public async Task Bit_write_with_null_parent_value_does_not_zero_the_word()
+    {
+        var (drv, factory) = NewDriver(
+            new TwinCATTagDefinition("Flag", "ads://5.23.91.23.1.1:851", "MAIN.Flags.3", TwinCATDataType.Bool));
+        // Do NOT seed Values["MAIN.Flags"] — the fake returns (null, Good) for the parent read.
+        factory.Customise = () => new FakeTwinCATClient();
+        await drv.InitializeAsync("{}", CancellationToken.None);
+
+        var results = await drv.WriteAsync([new WriteRequest("Flag", true)], CancellationToken.None);
+
+        results.Single().StatusCode.ShouldBe(TwinCATStatusMapper.BadCommunicationError);
+        // No parent write attempted — the word is left untouched on the device.
+        factory.Clients[0].WriteLog.ShouldBeEmpty();
+    }
+
    /// <summary>Concurrent bit writes to the same word compose correctly (per-parent lock).</summary>
    [Fact]
    public async Task Concurrent_bit_writes_to_same_word_compose_correctly()