fix(driver-s7): resolve Low code-review findings (Driver.S7-003,005,009,010,013)

- Driver.S7-003: ArgumentNullException.ThrowIfNull on the references
  argument at the top of ReadAsync / WriteAsync (was reaching .Count
  before any null check).
- Driver.S7-005: drop the redundant global::S7.Net.Plc qualifiers in
  ReadOneAsync / WriteOneAsync — using S7.Net already covers Plc.
- Driver.S7-009: PollLoopAsync degrades _health to Degraded after
  sustained failure and backs off exponentially up to PollBackoffCap;
  resets on a healthy tick so an operator can see the loop wedge.
- Driver.S7-010: Dispose runs the synchronous teardown directly with a
  bounded WhenAll Wait drain instead of bridging via DisposeAsync().
- Driver.S7-013: reject unsupported S7DataType values (Int64 / UInt64 /
  Float64 / String / DateTime) at InitializeAsync so half-implemented
  types no longer leak BadNotSupported live nodes into the address space.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-23 07:45:45 -04:00
parent 6575c6e5f6
commit af0f09d07e
3 changed files with 433 additions and 20 deletions

View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 5 |
| Open findings | 0 |
## Checklist coverage
@@ -89,7 +89,7 @@ correct the comment so the lossiness of UInt32 is documented.
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `S7Driver.cs:172`, `S7Driver.cs:255` |
| Status | Open |
| Status | Resolved |
**Description:** ReadAsync and WriteAsync dereference fullReferences.Count /
writes.Count with no null guard. A null argument throws NullReferenceException
@@ -101,7 +101,13 @@ inconsistent with it.
**Recommendation:** Add ArgumentNullException.ThrowIfNull for the list parameters
at the top of ReadAsync and WriteAsync.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — added `ArgumentNullException.ThrowIfNull`
at the top of both `ReadAsync` and `WriteAsync`, placed BEFORE `RequirePlc()` so
a null argument produces a typed `ArgumentNullException` (consistent with
`DiscoverAsync`) rather than either an NRE on `.Count` or the "not initialized"
`InvalidOperationException` from `RequirePlc`. Regression tests
`ReadAsync_with_null_fullReferences_throws_ArgumentNullException` and
`WriteAsync_with_null_writes_throws_ArgumentNullException`.
### Driver.S7-004
@@ -133,7 +139,7 @@ and swallowed poll-loop / shutdown exceptions.
| Severity | Low |
| Category | OtOpcUa conventions |
| Location | `S7Driver.cs:33`, `S7Driver.cs:433` |
| Status | Open |
| Status | Resolved |
**Description:** System.Collections.Concurrent.ConcurrentDictionary is written
out with a fully-qualified namespace at the field declarations instead of a
@@ -145,7 +151,11 @@ S7Driver.cs despite the file-top using S7.Net.
**Recommendation:** Add using System.Collections.Concurrent and drop the
redundant global::S7.Net. qualifiers where using S7.Net already covers them.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `using System.Collections.Concurrent` was
already added by an earlier finding fix; this resolution removes the remaining
`global::S7.Net.Plc` qualifiers from the `ReadOneAsync` and `WriteOneAsync`
signatures, now using the unqualified `Plc` type (the file-top `using S7.Net`
already covers it). House style restored.
### Driver.S7-006
@@ -250,7 +260,7 @@ status, and update _health to Degraded on transport failures.
| Severity | Low |
| Category | Error handling & resilience |
| Location | `S7Driver.cs:392` |
| Status | Open |
| Status | Resolved |
**Description:** The subscription poll loop never reflects sustained polling
failure anywhere an operator can see it. PollLoopAsync swallows every
@@ -266,7 +276,19 @@ Interval indefinitely on a hard failure.
apply a capped backoff after consecutive errors; at minimum log the swallowed
exception (see Driver.S7-004).
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `PollLoopAsync` now tracks
`consecutiveFailures`, calls new `HandlePollFailure` which both logs (with the
failure count) AND degrades `_health` to `Degraded` once
`PollFailureHealthThreshold` (1) consecutive failures have accumulated, and
applies a capped exponential backoff via new `ComputeBackoffDelay` (doubles the
wait each consecutive failure up to a 30 s `PollBackoffCap`). A healthy tick
resets the counter so the cadence snaps back to the configured Interval.
`HandlePollFailure` refuses to downgrade a `Faulted` state (reserved for
permanent config faults like PUT/GET-denied). Regression test
`PollLoop_against_uninitialized_driver_degrades_health` proves the health
surface now reflects sustained failure; `PollLoop_applies_capped_backoff_after_consecutive_failures`
proves shutdown still completes inside the drain window even under a fault
storm.
### Driver.S7-010
@@ -275,7 +297,7 @@ exception (see Driver.S7-004).
| Severity | Low |
| Category | Performance & resource management |
| Location | `S7Driver.cs:504` |
| Status | Open |
| Status | Resolved |
**Description:** Dispose() is implemented as
DisposeAsync().AsTask().GetAwaiter().GetResult() - sync-over-async. Inside the
@@ -288,7 +310,16 @@ blocking wrap is unnecessary risk.
perform the teardown directly (cancel CTSs, close Plc, dispose _gate) without
round-tripping through the async path.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `Dispose()` now performs teardown
directly via a new private `SynchronousTeardown` method that mirrors
`ShutdownAsync` but uses `Task.WhenAll(...).Wait(DrainTimeout)` instead of
`await Task.WhenAll(...).WaitAsync(...)`. Probe + poll Tasks are still drained
with the bounded 5 s timeout (so a wedged loop cannot hang `Dispose` indefinitely),
but the sync path no longer round-trips through `DisposeAsync().AsTask().GetAwaiter().GetResult()`.
`DisposeAsync` keeps its existing implementation for callers that opt into the
async dispose pattern. Regression tests
`Dispose_completes_synchronously_without_sync_over_async_round_trip` and
`Dispose_is_idempotent`.
### Driver.S7-011
@@ -358,7 +389,7 @@ ReadStatusAsync-based probe.
| Severity | Low |
| Category | Code organization & conventions |
| Location | `S7DriverOptions.cs:90`, `S7Driver.cs:300` |
| Status | Open |
| Status | Resolved |
**Description:** S7TagDefinition.StringLength is a public configured/JSON-bound
parameter (default 254) but is dead: S7DataType.String reads and writes both
@@ -376,7 +407,20 @@ StringLength) at InitializeAsync / factory validation with a clear "not yet
supported" error, so a partially-implemented type cannot be configured into a
live address space.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-23 — `InitializeAsync` now runs new
`RejectUnsupportedTagDataTypes`, which throws `NotSupportedException` for any
tag whose `DataType` is in the `UnimplementedDataTypes` set (`Int64`, `UInt64`,
`Float64`, `String`, `DateTime`). The half-implemented types can no longer leak
into the live address space — a site that configures one fails fast at init
rather than seeing a node that returns `BadNotSupported` on every access.
Entries should be removed from `UnimplementedDataTypes` as each type is wired
through; the comment on `RejectUnsupportedTagDataTypes` makes it a single grep
target for that follow-up. `StringLength` remains in `S7TagDefinition` because
removing it would be a breaking change to existing config JSON; once `String`
is implemented it will be consumed without further config changes. Regression
tests `Initialize_rejects_not_yet_implemented_data_type_with_NotSupportedException`
(Theory, 5 types) and `Initialize_accepts_implemented_data_types` (Theory, 7
types) prove the guard is targeted.
### Driver.S7-014