dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(driver-cli-common): resolve Medium code-review finding (Driver.Cli.Common-003)

Browse Source 
ConfigureLogging is now idempotent via a _loggingConfigured guard field so
repeated calls from subclasses do not abandon and leak the previous logger.
The previous Log.Logger is disposed before overwriting to release its
console-sink resources cleanly.

A new protected static FlushLogging() helper calls Log.CloseAndFlush() so
commands can guarantee buffered output is flushed in their finally blocks
before the process exits — important for the long-running subscribe verb.

XML doc updated to reflect call-once semantics and document FlushLogging().

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 08:38:09 -04:00
parent 1433a1cf30
commit 7ff356bddc
2 changed files with 27 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
code-reviews/Driver.Cli.Common/findings.md
View File

@@ -105,7 +105,7 @@ on the masked code (`code & 0xFFFF0000`).
| Severity | Medium |
| Category | Concurrency & thread safety |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:51-59` |
| Status | Open |
| Status | Resolved |
**Description:** `ConfigureLogging` assigns the process-global `Serilog.Log.Logger`
without disposing the previously assigned logger and the library never calls
@@ -121,7 +121,7 @@ command `ExecuteAsync`, or via a `protected` disposal helper on this base). Trea
`ConfigureLogging` as call-once / idempotent and document that. At minimum capture and
dispose the previous logger if reconfiguration is genuinely intended.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — `ConfigureLogging` is now idempotent (guarded by `_loggingConfigured` field) and disposes the previous `Log.Logger` before overwriting; a new `protected static FlushLogging()` helper calls `Log.CloseAndFlush()` for commands to call in their `finally` blocks; XML doc updated accordingly.
### Driver.Cli.Common-004

30
src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs
View File

@@ -7,8 +7,8 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Cli.Common;
/// <summary>
/// Shared base for every driver test-client command (Modbus / AB CIP / AB Legacy / S7 /
/// TwinCAT). Carries the options that are meaningful regardless of protocol — verbose
/// logging + the standard timeout — plus helpers every command implementation wants:
/// TwinCAT / FOCAS). Carries the options that are meaningful regardless of protocol —
/// verbose logging + the standard timeout — plus helpers every command implementation wants:
/// Serilog configuration + cancellation-token capture.
/// </summary>
/// <remarks>
@@ -44,17 +44,37 @@ public abstract class DriverCommandBase : ICommand
public abstract ValueTask ExecuteAsync(IConsole console);
/// <summary>
/// Configures the process-global Serilog logger. Commands call this at the top of
/// <see cref="ExecuteAsync"/> so driver-internal <c>Log.Logger</c> writes land on the
/// same sink as the CLI's operator-facing output.
/// Configures the process-global Serilog logger. Intended to be called exactly once,
/// at the top of <see cref="ExecuteAsync"/>, so driver-internal <c>Log.Logger</c>
/// writes land on the same sink as the CLI's operator-facing output.
/// If the logger has already been configured this call is a no-op (idempotent).
/// Call <see cref="FlushLogging"/> in a <c>finally</c> block to ensure buffered output
/// is flushed before the process exits.
/// </summary>
protected void ConfigureLogging()
{
if (_loggingConfigured) return;
_loggingConfigured = true;
// Dispose the previous global logger (e.g. Serilog's silent bootstrap logger) so
// its resources are released cleanly before we overwrite Log.Logger.
var previous = Log.Logger;
var config = new LoggerConfiguration();
if (Verbose)
config.MinimumLevel.Debug().WriteTo.Console();
else
config.MinimumLevel.Warning().WriteTo.Console();
Log.Logger = config.CreateLogger();
(previous as IDisposable)?.Dispose();
}
/// <summary>
/// Flushes and closes the Serilog logger configured by <see cref="ConfigureLogging"/>.
/// Call this in a <c>finally</c> block inside <see cref="ExecuteAsync"/> to prevent
/// buffered log output from being lost on process exit, particularly for long-running
/// commands such as <c>subscribe</c>.
/// </summary>
protected static void FlushLogging() => Log.CloseAndFlush();
private bool _loggingConfigured;
}