From f7d6bd12b97df2a4766f4d8ed9d32b1d43324f96 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 10:44:16 -0400 Subject: [PATCH] fix(driver-twincat): resolve Medium code-review finding (Driver.TwinCAT-010) Replace yield break with cancellationToken.ThrowIfCancellationRequested() in BrowseSymbolsAsync so a cancelled browse propagates as OperationCanceledException instead of silently completing with a partial symbol set. Co-Authored-By: Claude Sonnet 4.6 --- code-reviews/Driver.TwinCAT/findings.md | 2 +- .../ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/code-reviews/Driver.TwinCAT/findings.md b/code-reviews/Driver.TwinCAT/findings.md index 04de94f..3d4beee 100644 --- a/code-reviews/Driver.TwinCAT/findings.md +++ b/code-reviews/Driver.TwinCAT/findings.md @@ -272,7 +272,7 @@ symbol set with no indication it was truncated. The `SymbolLoaderFactory.Create` `yield break` so a cancelled browse surfaces as cancellation, not as a successful but partial discovery. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — replaced `yield break` with `cancellationToken.ThrowIfCancellationRequested()` in the `foreach` loop so a cancelled browse propagates as `OperationCanceledException`, matching the `DiscoverAsync` expectation. ### Driver.TwinCAT-011 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs index cc5bf4c..af86e4d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/AdsTwinCATClient.cs @@ -244,7 +244,12 @@ internal sealed class AdsTwinCATClient : ITwinCATClient foreach (ISymbol symbol in loader.Symbols) { - if (cancellationToken.IsCancellationRequested) yield break; + // ThrowIfCancellationRequested — not yield break — so a cancelled browse propagates + // as OperationCanceledException rather than a silent clean completion. DiscoverAsync + // has an explicit catch(OperationCanceledException){ throw; } to surface this + // distinctly from a genuine browse failure; a yield break would let a partial + // symbol set appear as a fully successful discovery (Driver.TwinCAT-010). + cancellationToken.ThrowIfCancellationRequested(); var mapped = MapSymbolTypeName(symbol.DataType?.Name); var readOnly = !IsSymbolWritable(symbol); yield return new TwinCATDiscoveredSymbol(symbol.InstancePath, mapped, readOnly);