From 2ffd237ddde0ccea26bb37533efaf28486888f43 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 3 Jun 2026 17:27:58 -0500 Subject: [PATCH] cleanup after DI graph rewrite --- .../UI/MainForm.cs | 16 ++ .../UI/SensorGadget.cs | 15 ++ .../UI/SystemTray.cs | 15 ++ .../Utilities/Logger.cs | 105 ++++++---- .../Services/Logger.cs | 91 +++++--- .../Services/PasswordHasher.cs | 5 + .../Services/RemoteWebServer.cs | 14 +- .../Services/SensorIconRenderer.cs | 10 + .../Services/TrayIconService.cs | 12 ++ .../ViewModels/MainWindowViewModel.cs | 15 +- .../ViewModels/PlotTrackingService.cs | 9 +- LibreHardwareMonitorLib/Hardware/Computer.cs | 194 +++++++++++------- .../Hardware/Cpu/GenericCpu.cs | 47 +++-- .../Hardware/Cpu/IntelCpu.cs | 14 +- .../Hardware/HardwareStartupTrace.cs | 10 +- .../Hardware/IHardwareDiscoveryTask.cs | 8 + .../Hardware/Memory/MemoryGroup.cs | 41 ++-- .../Hardware/SettingsParsing.cs | 37 ++++ docs/code-review-followup.md | 111 ++++++++++ 19 files changed, 553 insertions(+), 216 deletions(-) create mode 100644 LibreHardwareMonitorLib/Hardware/SettingsParsing.cs create mode 100644 docs/code-review-followup.md diff --git a/LibreHardwareMonitor.Windows.Forms/UI/MainForm.cs b/LibreHardwareMonitor.Windows.Forms/UI/MainForm.cs index a4aff93..a8dcd0e 100644 --- a/LibreHardwareMonitor.Windows.Forms/UI/MainForm.cs +++ b/LibreHardwareMonitor.Windows.Forms/UI/MainForm.cs @@ -56,6 +56,7 @@ public sealed partial class MainForm : Form private readonly SystemTray _systemTray; private readonly UnitManager _unitManager; private readonly UpdateVisitor _updateVisitor = new(); + private readonly System.Threading.SynchronizationContext _uiSyncContext; private int _delayCount; private Form _plotForm; @@ -164,6 +165,9 @@ public sealed partial class MainForm : Form perSessionFileRotationMenuItem.Checked = _logger.FileRotationMethod == LoggerFileRotation.PerSession; dailyFileRotationMenuItem.Checked = _logger.FileRotationMethod == LoggerFileRotation.Daily; + // Captured on the UI thread; HardwareAdded/HardwareRemoved can now fire on a background discovery thread + // (when a defer env var is set), so they re-dispatch here before touching the tree. + _uiSyncContext = System.Threading.SynchronizationContext.Current; _computer.HardwareAdded += HardwareAdded; _computer.HardwareRemoved += HardwareRemoved; @@ -824,12 +828,24 @@ public sealed partial class MainForm : Form private void HardwareAdded(IHardware hardware) { + if (_uiSyncContext != null && System.Threading.SynchronizationContext.Current != _uiSyncContext) + { + _uiSyncContext.Post(_ => HardwareAdded(hardware), null); + return; + } + SubHardwareAdded(hardware, _root); PlotSelectionChanged(this, null); } private void HardwareRemoved(IHardware hardware) { + if (_uiSyncContext != null && System.Threading.SynchronizationContext.Current != _uiSyncContext) + { + _uiSyncContext.Post(_ => HardwareRemoved(hardware), null); + return; + } + List nodesToRemove = new(); foreach (Node node in _root.Nodes) { diff --git a/LibreHardwareMonitor.Windows.Forms/UI/SensorGadget.cs b/LibreHardwareMonitor.Windows.Forms/UI/SensorGadget.cs index beba3ef..3162906 100644 --- a/LibreHardwareMonitor.Windows.Forms/UI/SensorGadget.cs +++ b/LibreHardwareMonitor.Windows.Forms/UI/SensorGadget.cs @@ -62,11 +62,14 @@ public class SensorGadget : Gadget private StringFormat _alignRightStringFormat; private Color _fontColor; private Color _backgroundColor; + private readonly System.Threading.SynchronizationContext _uiSyncContext; public SensorGadget(IComputer computer, PersistentSettings settings, UnitManager unitManager) { _unitManager = unitManager; _settings = settings; + // Captured on the UI thread so the now-possibly-background HardwareAdded/HardwareRemoved re-dispatch here. + _uiSyncContext = System.Threading.SynchronizationContext.Current; computer.HardwareAdded += HardwareAdded; computer.HardwareRemoved += HardwareRemoved; @@ -385,6 +388,12 @@ public class SensorGadget : Gadget private void HardwareRemoved(IHardware hardware) { + if (_uiSyncContext != null && System.Threading.SynchronizationContext.Current != _uiSyncContext) + { + _uiSyncContext.Post(_ => HardwareRemoved(hardware), null); + return; + } + hardware.SensorAdded -= SensorAdded; hardware.SensorRemoved -= SensorRemoved; @@ -397,6 +406,12 @@ public class SensorGadget : Gadget private void HardwareAdded(IHardware hardware) { + if (_uiSyncContext != null && System.Threading.SynchronizationContext.Current != _uiSyncContext) + { + _uiSyncContext.Post(_ => HardwareAdded(hardware), null); + return; + } + foreach (ISensor sensor in hardware.Sensors) SensorAdded(sensor); diff --git a/LibreHardwareMonitor.Windows.Forms/UI/SystemTray.cs b/LibreHardwareMonitor.Windows.Forms/UI/SystemTray.cs index 10ec3d8..580fcf3 100644 --- a/LibreHardwareMonitor.Windows.Forms/UI/SystemTray.cs +++ b/LibreHardwareMonitor.Windows.Forms/UI/SystemTray.cs @@ -20,12 +20,15 @@ public class SystemTray : IDisposable private readonly List _sensorList = new List(); private bool _mainIconEnabled; private readonly NotifyIconAdv _mainIcon; + private readonly System.Threading.SynchronizationContext _uiSyncContext; public SystemTray(IComputer computer, PersistentSettings settings, UnitManager unitManager) { _computer = computer; _settings = settings; _unitManager = unitManager; + // Captured on the UI thread so the now-possibly-background HardwareAdded/HardwareRemoved re-dispatch here. + _uiSyncContext = System.Threading.SynchronizationContext.Current; computer.HardwareAdded += HardwareAdded; computer.HardwareRemoved += HardwareRemoved; @@ -56,6 +59,12 @@ public class SystemTray : IDisposable private void HardwareRemoved(IHardware hardware) { + if (_uiSyncContext != null && System.Threading.SynchronizationContext.Current != _uiSyncContext) + { + _uiSyncContext.Post(_ => HardwareRemoved(hardware), null); + return; + } + hardware.SensorAdded -= SensorAdded; hardware.SensorRemoved -= SensorRemoved; @@ -68,6 +77,12 @@ public class SystemTray : IDisposable private void HardwareAdded(IHardware hardware) { + if (_uiSyncContext != null && System.Threading.SynchronizationContext.Current != _uiSyncContext) + { + _uiSyncContext.Post(_ => HardwareAdded(hardware), null); + return; + } + foreach (ISensor sensor in hardware.Sensors) SensorAdded(sensor); diff --git a/LibreHardwareMonitor.Windows.Forms/Utilities/Logger.cs b/LibreHardwareMonitor.Windows.Forms/Utilities/Logger.cs index 7d53f8a..84f8e0f 100644 --- a/LibreHardwareMonitor.Windows.Forms/Utilities/Logger.cs +++ b/LibreHardwareMonitor.Windows.Forms/Utilities/Logger.cs @@ -24,6 +24,10 @@ public class Logger private string[] _identifiers; private ISensor[] _sensors; private DateTime _lastLoggedTime = DateTime.MinValue; + // Guards the _sensors/_identifiers pair: when a defer env var is set, Computer.HardwareAdded can now fire on a + // background thread, so SensorAdded/SensorRemoved can run concurrently with the timer thread's rotation. Mirrors + // the WinUI Logger fix. + private readonly object _sync = new object(); public LoggerFileRotation FileRotationMethod = LoggerFileRotation.PerSession; @@ -60,25 +64,31 @@ public class Logger private void SensorAdded(ISensor sensor) { - if (_sensors == null) - return; - - for (int i = 0; i < _sensors.Length; i++) + lock (_sync) { - if (sensor.Identifier.ToString() == _identifiers[i]) - _sensors[i] = sensor; + if (_sensors == null) + return; + + for (int i = 0; i < _sensors.Length; i++) + { + if (sensor.Identifier.ToString() == _identifiers[i]) + _sensors[i] = sensor; + } } } private void SensorRemoved(ISensor sensor) { - if (_sensors == null) - return; - - for (int i = 0; i < _sensors.Length; i++) + lock (_sync) { - if (sensor == _sensors[i]) - _sensors[i] = null; + if (_sensors == null) + return; + + for (int i = 0; i < _sensors.Length; i++) + { + if (sensor == _sensors[i]) + _sensors[i] = null; + } } } @@ -93,6 +103,7 @@ public class Logger if (!File.Exists(_fileName)) return false; + string[] identifiers; try { string line; @@ -102,28 +113,31 @@ public class Logger if (string.IsNullOrEmpty(line)) return false; - _identifiers = line.Split(',').Skip(1).ToArray(); + identifiers = line.Split(',').Skip(1).ToArray(); } catch { - _identifiers = null; return false; } - if (_identifiers.Length == 0) - { - _identifiers = null; + if (identifiers.Length == 0) return false; - } - _sensors = new ISensor[_identifiers.Length]; + ISensor[] sensors = new ISensor[identifiers.Length]; SensorVisitor visitor = new SensorVisitor(sensor => { - for (int i = 0; i < _identifiers.Length; i++) - if (sensor.Identifier.ToString() == _identifiers[i]) - _sensors[i] = sensor; + for (int i = 0; i < identifiers.Length; i++) + if (sensor.Identifier.ToString() == identifiers[i]) + sensors[i] = sensor; }); visitor.VisitComputer(_computer); + + lock (_sync) + { + _identifiers = identifiers; + _sensors = sensors; + } + return true; } @@ -135,28 +149,34 @@ public class Logger list.Add(sensor); }); visitor.VisitComputer(_computer); - _sensors = list.ToArray(); - _identifiers = _sensors.Select(s => s.Identifier.ToString()).ToArray(); + ISensor[] sensors = list.ToArray(); + string[] identifiers = sensors.Select(s => s.Identifier.ToString()).ToArray(); + + lock (_sync) + { + _sensors = sensors; + _identifiers = identifiers; + } using (StreamWriter writer = new StreamWriter(_fileName, false)) { writer.Write(","); - for (int i = 0; i < _sensors.Length; i++) + for (int i = 0; i < sensors.Length; i++) { - writer.Write(_sensors[i].Identifier); - if (i < _sensors.Length - 1) + writer.Write(sensors[i].Identifier); + if (i < sensors.Length - 1) writer.Write(","); else writer.WriteLine(); } writer.Write("Time,"); - for (int i = 0; i < _sensors.Length; i++) + for (int i = 0; i < sensors.Length; i++) { writer.Write('"'); - writer.Write(_sensors[i].Name); + writer.Write(sensors[i].Name); writer.Write('"'); - if (i < _sensors.Length - 1) + if (i < sensors.Length - 1) writer.Write(","); else writer.WriteLine(); @@ -199,24 +219,31 @@ public class Logger break; } + ISensor[] sensors; + lock (_sync) + sensors = _sensors; + try { using (StreamWriter writer = new StreamWriter(new FileStream(_fileName, FileMode.Append, FileAccess.Write, FileShare.ReadWrite))) { writer.Write(now.ToString("G", CultureInfo.InvariantCulture)); writer.Write(","); - for (int i = 0; i < _sensors.Length; i++) + if (sensors != null) { - if (_sensors[i] != null) + for (int i = 0; i < sensors.Length; i++) { - float? value = _sensors[i].Value; - if (value.HasValue) - writer.Write(value.Value.ToString("R", CultureInfo.InvariantCulture)); + if (sensors[i] != null) + { + float? value = sensors[i].Value; + if (value.HasValue) + writer.Write(value.Value.ToString("R", CultureInfo.InvariantCulture)); + } + if (i < sensors.Length - 1) + writer.Write(","); + else + writer.WriteLine(); } - if (i < _sensors.Length - 1) - writer.Write(","); - else - writer.WriteLine(); } } } diff --git a/LibreHardwareMonitor.Windows.WinUI/Services/Logger.cs b/LibreHardwareMonitor.Windows.WinUI/Services/Logger.cs index 06a915a..d5757c0 100644 --- a/LibreHardwareMonitor.Windows.WinUI/Services/Logger.cs +++ b/LibreHardwareMonitor.Windows.WinUI/Services/Logger.cs @@ -18,6 +18,10 @@ public sealed class Logger : ILogger private readonly IComputer _computer; private readonly TimeProvider _timeProvider; private readonly string _baseDirectory; + // Guards the _sensors/_identifiers pair: the timer thread reassigns them on rotation while background + // hardware-discovery threads read/write them via SensorAdded/SensorRemoved (Computer.HardwareAdded now fires + // off the caller thread once any deferred-detection flag is active). + private readonly object _sync = new(); private DateTime _day = DateTime.MinValue; private string _fileName = ""; private string[]? _identifiers; @@ -76,21 +80,28 @@ public sealed class Logger : ILogger break; } + // Snapshot the sensor array under the lock so a concurrent SensorAdded/SensorRemoved, or a rotation that swaps + // in a different-length array, on a discovery thread cannot tear the read below. Element reads stay safe: a + // reference write to a slot is atomic, so each read yields either a sensor or null. + ISensor?[]? sensors; + lock (_sync) + sensors = _sensors; + try { using StreamWriter writer = new(new FileStream(_fileName, FileMode.Append, FileAccess.Write, FileShare.ReadWrite)); writer.Write(now.ToString("G", CultureInfo.InvariantCulture)); writer.Write(","); - if (_sensors != null) + if (sensors != null) { - for (int i = 0; i < _sensors.Length; i++) + for (int i = 0; i < sensors.Length; i++) { - float? value = _sensors[i]?.Value; + float? value = sensors[i]?.Value; if (value.HasValue) writer.Write(value.Value.ToString("R", CultureInfo.InvariantCulture)); - writer.Write(i < _sensors.Length - 1 ? "," : Environment.NewLine); + writer.Write(i < sensors.Length - 1 ? "," : Environment.NewLine); } } } @@ -112,12 +123,21 @@ public sealed class Logger : ILogger SensorVisitor visitor = new(sensor => sensors.Add(sensor)); visitor.VisitComputer(_computer); - _sensors = sensors.Cast().ToArray(); - _identifiers = sensors.Select(sensor => sensor.Identifier.ToString()).ToArray(); + ISensor?[] sensorArray = sensors.Cast().ToArray(); + string[] identifiers = sensors.Select(sensor => sensor.Identifier.ToString()).ToArray(); + + // Publish the paired arrays together under the lock so a concurrent SensorAdded/SensorRemoved never observes + // mismatched lengths. VisitComputer and the file write run outside the lock to avoid coupling with Computer's + // own locks. + lock (_sync) + { + _sensors = sensorArray; + _identifiers = identifiers; + } using StreamWriter writer = new(_fileName, false); writer.Write(","); - writer.WriteLine(string.Join(",", _identifiers)); + writer.WriteLine(string.Join(",", identifiers)); writer.Write("Time,"); writer.WriteLine(string.Join(",", sensors.Select(sensor => $"\"{sensor.Name}\""))); } @@ -127,6 +147,7 @@ public sealed class Logger : ILogger if (!File.Exists(_fileName)) return false; + string[] identifiers; try { using StreamReader reader = new(_fileName); @@ -134,30 +155,35 @@ public sealed class Logger : ILogger if (string.IsNullOrEmpty(line)) return false; - _identifiers = line.Split(',').Skip(1).ToArray(); + identifiers = line.Split(',').Skip(1).ToArray(); } catch { - _identifiers = null; return false; } - if (_identifiers.Length == 0) - { - _identifiers = null; + if (identifiers.Length == 0) return false; - } - _sensors = new ISensor?[_identifiers.Length]; + // Build into local arrays, then publish the pair together under the lock, so a discovery thread reading + // _sensors/_identifiers never observes a half-populated or length-mismatched pair. + ISensor?[] sensors = new ISensor?[identifiers.Length]; SensorVisitor visitor = new(sensor => { - for (int i = 0; i < _identifiers.Length; i++) + for (int i = 0; i < identifiers.Length; i++) { - if (sensor.Identifier.ToString() == _identifiers[i]) - _sensors[i] = sensor; + if (sensor.Identifier.ToString() == identifiers[i]) + sensors[i] = sensor; } }); visitor.VisitComputer(_computer); + + lock (_sync) + { + _identifiers = identifiers; + _sensors = sensors; + } + return true; } @@ -187,25 +213,32 @@ public sealed class Logger : ILogger private void SensorAdded(ISensor sensor) { - if (_sensors == null || _identifiers == null) - return; - - for (int i = 0; i < _sensors.Length; i++) + lock (_sync) { - if (sensor.Identifier.ToString() == _identifiers[i]) - _sensors[i] = sensor; + if (_sensors == null || _identifiers == null) + return; + + // _sensors and _identifiers are always published together with equal lengths, so indexing both is safe. + for (int i = 0; i < _sensors.Length; i++) + { + if (sensor.Identifier.ToString() == _identifiers[i]) + _sensors[i] = sensor; + } } } private void SensorRemoved(ISensor sensor) { - if (_sensors == null) - return; - - for (int i = 0; i < _sensors.Length; i++) + lock (_sync) { - if (sensor == _sensors[i]) - _sensors[i] = null; + if (_sensors == null) + return; + + for (int i = 0; i < _sensors.Length; i++) + { + if (sensor == _sensors[i]) + _sensors[i] = null; + } } } } diff --git a/LibreHardwareMonitor.Windows.WinUI/Services/PasswordHasher.cs b/LibreHardwareMonitor.Windows.WinUI/Services/PasswordHasher.cs index 58e9eb9..b493c0a 100644 --- a/LibreHardwareMonitor.Windows.WinUI/Services/PasswordHasher.cs +++ b/LibreHardwareMonitor.Windows.WinUI/Services/PasswordHasher.cs @@ -81,6 +81,11 @@ internal static class PasswordHasher return false; } + // Reject a missing salt or hash segment: an empty expected hash would make FixedTimeEquals(empty, empty) return + // true and authenticate any password. Hash() never produces this, so it only arises from a corrupted value. + if (salt.Length == 0 || expected.Length == 0) + return false; + byte[] actual = Rfc2898DeriveBytes.Pbkdf2(password, salt, iterations, HashAlgorithmName.SHA256, expected.Length); return CryptographicOperations.FixedTimeEquals(actual, expected); } diff --git a/LibreHardwareMonitor.Windows.WinUI/Services/RemoteWebServer.cs b/LibreHardwareMonitor.Windows.WinUI/Services/RemoteWebServer.cs index 3cbd154..3b9d71f 100644 --- a/LibreHardwareMonitor.Windows.WinUI/Services/RemoteWebServer.cs +++ b/LibreHardwareMonitor.Windows.WinUI/Services/RemoteWebServer.cs @@ -408,6 +408,13 @@ public sealed class RemoteWebServer : IRemoteWebServer sensor.Control.SetSoftware(float.Parse(value, CultureInfo.InvariantCulture)); } + // Reused across requests: building a JsonSerializerOptions per request defeats System.Text.Json's per-options + // metadata cache, which matters because data.json is polled frequently. + private static readonly JsonSerializerOptions JsonOptions = new() + { + NumberHandling = JsonNumberHandling.AllowNamedFloatingPointLiterals + }; + private async Task SendJsonAsync(HttpListenerResponse response, HttpListenerRequest request) { Dictionary json = new() @@ -425,12 +432,7 @@ public sealed class RemoteWebServer : IRemoteWebServer SensorTreeItemViewModel? root = _rootProvider(); json["Children"] = root == null ? Array.Empty() : new List { GenerateJsonForNode(root, ref nodeIndex) }; - JsonSerializerOptions options = new() - { - NumberHandling = JsonNumberHandling.AllowNamedFloatingPointLiterals - }; - - byte[] buffer = Encoding.UTF8.GetBytes(JsonSerializer.Serialize(json, options)); + byte[] buffer = Encoding.UTF8.GetBytes(JsonSerializer.Serialize(json, JsonOptions)); bool acceptGzip = request.Headers["Accept-Encoding"]?.IndexOf("gzip", StringComparison.OrdinalIgnoreCase) >= 0; WriteCommonHeaders(response); diff --git a/LibreHardwareMonitor.Windows.WinUI/Services/SensorIconRenderer.cs b/LibreHardwareMonitor.Windows.WinUI/Services/SensorIconRenderer.cs index d34aa72..d1d770f 100644 --- a/LibreHardwareMonitor.Windows.WinUI/Services/SensorIconRenderer.cs +++ b/LibreHardwareMonitor.Windows.WinUI/Services/SensorIconRenderer.cs @@ -94,6 +94,16 @@ internal sealed class SensorIconRenderer } } + /// + /// Returns a key identifying the pixels would produce (the drawn text and the background + /// color). When it is unchanged the previously created icon can be reused instead of re-rendering a new GDI icon. + /// + public string GetRenderKey(ISensor sensor) + { + WinUIColor color = GetSensorTrayColor(sensor); + return $"{GetSensorIconText(sensor)}|{color.A:X2}{color.R:X2}{color.G:X2}{color.B:X2}"; + } + private WinUIColor GetSensorTrayColor(ISensor sensor) { WinUIColor defaultColor = sensor.SensorType is SensorType.Load or SensorType.Control or SensorType.Level diff --git a/LibreHardwareMonitor.Windows.WinUI/Services/TrayIconService.cs b/LibreHardwareMonitor.Windows.WinUI/Services/TrayIconService.cs index 53c8a71..6760e33 100644 --- a/LibreHardwareMonitor.Windows.WinUI/Services/TrayIconService.cs +++ b/LibreHardwareMonitor.Windows.WinUI/Services/TrayIconService.cs @@ -158,8 +158,17 @@ public sealed class TrayIconService : IDisposable private void Update(SensorTrayIcon icon) { + string renderKey = _iconRenderer.GetRenderKey(icon.Sensor); + if (icon.IconHandle != IntPtr.Zero && renderKey == icon.LastRenderKey) + { + // The icon pixels are unchanged; refresh only the (cheap) tooltip and skip the GDI re-render + handle churn. + ModifyNotifyIcon(icon.Id, icon.IconHandle, GetSensorToolTip(icon.Sensor)); + return; + } + IntPtr previousIcon = icon.IconHandle; icon.IconHandle = _iconRenderer.CreateIcon(icon.Sensor); + icon.LastRenderKey = renderKey; ModifyNotifyIcon(icon.Id, icon.IconHandle, GetSensorToolTip(icon.Sensor)); if (previousIcon != IntPtr.Zero) DestroyIcon(previousIcon); @@ -373,5 +382,8 @@ public sealed class TrayIconService : IDisposable public IntPtr IconHandle { get; set; } public ISensor Sensor { get; set; } = sensor; + + /// Key of the pixels currently rendered into , used to skip redundant re-renders. + public string? LastRenderKey { get; set; } } } diff --git a/LibreHardwareMonitor.Windows.WinUI/ViewModels/MainWindowViewModel.cs b/LibreHardwareMonitor.Windows.WinUI/ViewModels/MainWindowViewModel.cs index 8d7f2f4..7d92acc 100644 --- a/LibreHardwareMonitor.Windows.WinUI/ViewModels/MainWindowViewModel.cs +++ b/LibreHardwareMonitor.Windows.WinUI/ViewModels/MainWindowViewModel.cs @@ -112,6 +112,8 @@ public sealed class MainWindowViewModel : ViewModelBase, IDisposable private bool _isStarted; private bool _isStarting; private int _rootUpdateQueued; + private int _statusHardwareCount; + private int _statusSensorCount; private string _statusText = ""; private TemperatureUnit _temperatureUnit; private int _updateIntervalIndex; @@ -1041,14 +1043,21 @@ public sealed class MainWindowViewModel : ViewModelBase, IDisposable MeasureStartup("MainWindowViewModel.ConfigureRoot", () => root.Configure(TemperatureUnit, ShowHiddenSensors, ShowValueColumn, ShowMinColumn, ShowMaxColumn), GetRootDetail); MeasureStartup("MainWindowViewModel.RootItems.Add", () => RootItems.Add(root), GetRootDetail); MeasureStartup("MainWindowViewModel.ApplySensorValuesTimeWindow", ApplySensorValuesTimeWindow, GetRootDetail); + RefreshStatusCounts(); + } + + // Hardware/sensor counts change only when the tree is rebuilt, so cache them here (called from UpdateRoot) instead + // of re-walking the whole tree and re-aggregating Computer.Hardware on every per-tick UpdateStatus(). + private void RefreshStatusCounts() + { + _statusHardwareCount = _hardwareMonitor.Computer.Hardware.Count; + _statusSensorCount = RootItems.FirstOrDefault()?.EnumerateSensors().Count() ?? 0; } private void UpdateStatus() { - int hardwareCount = _hardwareMonitor.Computer.Hardware.Count; - int sensorCount = RootItems.FirstOrDefault()?.EnumerateSensors().Count() ?? 0; string webServerStatus = RunWebServer ? $", web server {WebServerUrl}" : ""; - StatusText = $"{hardwareCount} hardware devices, {sensorCount} sensors, update every {FormatInterval(UpdateInterval)}{webServerStatus}"; + StatusText = $"{_statusHardwareCount} hardware devices, {_statusSensorCount} sensors, update every {FormatInterval(UpdateInterval)}{webServerStatus}"; } private void RestartWebServerIfRunning() diff --git a/LibreHardwareMonitor.Windows.WinUI/ViewModels/PlotTrackingService.cs b/LibreHardwareMonitor.Windows.WinUI/ViewModels/PlotTrackingService.cs index 5345e9a..3e3839e 100644 --- a/LibreHardwareMonitor.Windows.WinUI/ViewModels/PlotTrackingService.cs +++ b/LibreHardwareMonitor.Windows.WinUI/ViewModels/PlotTrackingService.cs @@ -78,17 +78,20 @@ internal sealed class PlotTrackingService series.Color = sensorItem.PenColor.Value; } + // The pipeline below sorts and de-duplicates, so there is no need to OrderBy the whole (potentially 24h) + // history every tick; track the latest timestamp in the same pass instead of assuming the list is ordered. List points = []; - foreach (SensorValue sensorValue in sensor.Values.OrderBy(value => value.Time)) + DateTime? latestHistoryTimestamp = null; + foreach (SensorValue sensorValue in sensor.Values) { double? displayedValue = SensorFormatter.GetPlotValue(sensor, sensorValue.Value, temperatureUnit); if (displayedValue is not { } pointValue || !double.IsFinite(pointValue)) continue; points.Add(new PlotPointViewModel(sensorValue.Time, pointValue)); + if (!latestHistoryTimestamp.HasValue || sensorValue.Time > latestHistoryTimestamp.Value) + latestHistoryTimestamp = sensorValue.Time; } - - DateTime? latestHistoryTimestamp = points.Count > 0 ? points[^1].Timestamp : null; foreach (PlotPointViewModel existingPoint in series.Points) { if (now - existingPoint.Timestamp > MaximumSyntheticPlotPointRetention) diff --git a/LibreHardwareMonitorLib/Hardware/Computer.cs b/LibreHardwareMonitorLib/Hardware/Computer.cs index 8da07ce..e544903 100644 --- a/LibreHardwareMonitorLib/Hardware/Computer.cs +++ b/LibreHardwareMonitorLib/Hardware/Computer.cs @@ -71,6 +71,11 @@ public class Computer : IComputer private TaskCompletionSource _deferredGroupCompletionSource = CreateCompletedTaskCompletionSource(); private List _deferredGroupTasks = []; + // Serializes the Open/OpenAsync/Close/Reset lifecycle so an asynchronous open cannot interleave with teardown + // (which would let a background hardware probe outlive OpCode.Close()/Mutexes.Close()) and so two concurrent + // opens cannot double-initialize. Also serializes every access to _deferredGroupCancellationTokenSource. + private readonly object _openLock = new(); + /// /// Creates a new instance with basic initial . /// @@ -480,6 +485,7 @@ public class Computer : IComputer return false; bool added = false; + IHardware[] initialHardware = null; lock (_lock) { if (!cancellationToken.IsCancellationRequested && (isEnabled == null || isEnabled()) && !_groups.Contains(group)) @@ -492,16 +498,28 @@ public class Computer : IComputer hardwareChanged.HardwareRemoved += HardwareRemovedEvent; } - TrackGroupHardwareDiscoveryTask(group); + // Snapshot the hardware present at subscription time. Capturing it here -- before any background + // discovery is started below -- guarantees hardware discovered later is announced exactly once, via the + // event we just subscribed: never missed (subscription precedes discovery) and never also replayed from + // this snapshot (a duplicate). + initialHardware = [.. group.Hardware]; added = true; } } if (added) { + // Start background discovery only now that the subscription and snapshot are in place, then track its task + // so the run's completion (HardwareDiscoveryTask) waits for it. + if (group is IHardwareDiscoveryTask discoveryTask) + { + discoveryTask.StartHardwareDiscovery(); + TrackGroupHardwareDiscoveryTask(group); + } + if (HardwareAdded != null) { - foreach (IHardware hardware in group.Hardware) + foreach (IHardware hardware in initialHardware) HardwareAdded(hardware); } } @@ -563,9 +581,6 @@ public class Computer : IComputer /// public void Open() { - if (_open) - return; - OpenInternal(CancellationToken.None); } @@ -588,17 +603,30 @@ public class Computer : IComputer /// A task that completes once all enabled hardware groups have been discovered. public Task OpenAsync(CancellationToken cancellationToken) { - if (_open) - return Task.CompletedTask; + lock (_openLock) + { + if (_open) + return Task.CompletedTask; + } return Task.Run(() => OpenInternal(cancellationToken), cancellationToken); } private void OpenInternal(CancellationToken cancellationToken) { - if (_open) - return; + // Hold _openLock for the whole open so a concurrent Close()/Reset() cannot run mid-initialization and so a + // second Open()/OpenAsync() observes _open atomically instead of double-initializing OpCode/Mutexes/groups. + lock (_openLock) + { + if (_open) + return; + OpenCore(cancellationToken); + } + } + + private void OpenCore(CancellationToken cancellationToken) + { StartDeferredGroupRun(); try @@ -773,16 +801,9 @@ public class Computer : IComputer private void AddDeferredGroup(Func createGroup, Func isEnabled) { - CancellationTokenSource cancellationTokenSource = _deferredGroupCancellationTokenSource; - if (cancellationTokenSource == null) - { - if (isEnabled()) - Add(createGroup()); - - return; - } - - CancellationToken cancellationToken = cancellationTokenSource.Token; + // StartDeferredGroupRun (run under _openLock before AddGroups) always assigns the token source, so the deferred + // path is the only reachable one here. + CancellationToken cancellationToken = _deferredGroupCancellationTokenSource.Token; Task task = Task.Run(() => { IGroup group = null; @@ -808,23 +829,7 @@ public class Computer : IComputer private void AddDeferredGroups(Func isEnabled, params Func[] createGroups) { - CancellationTokenSource cancellationTokenSource = _deferredGroupCancellationTokenSource; - if (cancellationTokenSource == null) - { - foreach (Func createGroup in createGroups) - { - if (!isEnabled()) - return; - - IGroup group = createGroup(); - if (group != null) - Add(group); - } - - return; - } - - CancellationToken cancellationToken = cancellationTokenSource.Token; + CancellationToken cancellationToken = _deferredGroupCancellationTokenSource.Token; Task task = Task.Run(() => { foreach (Func createGroup in createGroups) @@ -858,19 +863,7 @@ public class Computer : IComputer private bool ShouldDeferDetection(string settingName, string environmentVariable) { - string environmentValue = Environment.GetEnvironmentVariable(environmentVariable); - if (!string.IsNullOrWhiteSpace(environmentValue)) - return IsTruthy(environmentValue); - - return IsTruthy(_settings.GetValue(settingName, "false")); - } - - private static bool IsTruthy(string value) - { - return value.Equals("1", StringComparison.OrdinalIgnoreCase) - || value.Equals("true", StringComparison.OrdinalIgnoreCase) - || value.Equals("yes", StringComparison.OrdinalIgnoreCase) - || value.Equals("on", StringComparison.OrdinalIgnoreCase); + return SettingsParsing.ShouldDefer(_settings, settingName, environmentVariable); } private void StartDeferredGroupRun() @@ -890,10 +883,12 @@ public class Computer : IComputer CancellationTokenSource cancellationTokenSource = _deferredGroupCancellationTokenSource; _deferredGroupCancellationTokenSource = null; TaskCompletionSource completionSource; + Task[] tasks; lock (_deferredGroupLock) { completionSource = _deferredGroupCompletionSource; + tasks = _deferredGroupTasks.ToArray(); _deferredGroupTasks = []; } @@ -904,8 +899,33 @@ public class Computer : IComputer } cancellationTokenSource.Cancel(); - cancellationTokenSource.Dispose(); + + // Mark the run cancelled before draining so the Task.WhenAll continuation in CompleteDeferredGroupRunWhenRegistered + // cannot win the race and raise HardwareDiscoveryCompleted during teardown. completionSource.TrySetCanceled(); + + // Wait for in-flight deferred construction to unwind before disposing the token source or returning to a caller + // that is about to tear down native state. A deferred task only checks the token before createGroup() and again + // in AddCore, so the group constructor (which probes hardware via OpCode/native APIs) always runs to completion; + // draining here keeps it from racing OpCode.Close()/Mutexes.Close() in Close() or a fresh run from Reset()/Open(). + WaitForDeferredGroupTasks(tasks); + + cancellationTokenSource.Dispose(); + } + + private static void WaitForDeferredGroupTasks(Task[] tasks) + { + if (tasks.Length == 0) + return; + + try + { + Task.WaitAll(tasks); + } + catch (AggregateException) + { + // Deferred discovery tasks observe and log their own exceptions; nothing actionable surfaces here. + } } private static TaskCompletionSource CreateCompletedTaskCompletionSource() @@ -933,20 +953,37 @@ public class Computer : IComputer private void CompleteDeferredGroupRunWhenRegistered() { TaskCompletionSource completionSource; + lock (_deferredGroupLock) + completionSource = _deferredGroupCompletionSource; + + WaitForRegisteredTasksThenComplete(completionSource, 0); + } + + /// + /// Completes the run once every tracked deferred task has finished. A deferred group that is itself an + /// registers its nested task from a background thread, so more tasks can + /// appear after the first Task.WhenAll; this re-arms on the larger set (comparing against the count + /// already awaited) until no new task has been registered, so completion never fires while discovery is still running. + /// + private void WaitForRegisteredTasksThenComplete(TaskCompletionSource completionSource, int alreadyAwaited) + { Task[] tasks; lock (_deferredGroupLock) { - completionSource = _deferredGroupCompletionSource; + // A superseding run (Reset()/Close() started a new one) owns completion now; stop. + if (!ReferenceEquals(completionSource, _deferredGroupCompletionSource)) + return; + tasks = _deferredGroupTasks.ToArray(); } - if (tasks.Length == 0) + if (tasks.Length <= alreadyAwaited) { CompleteDeferredGroupRun(completionSource); return; } - _ = Task.WhenAll(tasks).ContinueWith(_ => CompleteDeferredGroupRun(completionSource), + _ = Task.WhenAll(tasks).ContinueWith(_ => WaitForRegisteredTasksThenComplete(completionSource, tasks.Length), CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); @@ -1045,25 +1082,31 @@ public class Computer : IComputer /// public void Close() { - if (!_open) - return; - - CancelDeferredGroupRun(); - - lock (_lock) + lock (_openLock) { - while (_groups.Count > 0) + if (!_open) + return; + + // Cancel and DRAIN deferred discovery before tearing down native state: a deferred group constructor runs + // to completion even after cancellation, so without waiting here a background probe could run concurrently + // with OpCode.Close()/Mutexes.Close() and fault in native code. + CancelDeferredGroupRun(); + + lock (_lock) { - IGroup group = _groups[_groups.Count - 1]; - Remove(group); + while (_groups.Count > 0) + { + IGroup group = _groups[_groups.Count - 1]; + Remove(group); + } } + + OpCode.Close(); + Mutexes.Close(); + + _smbios = null; + _open = false; } - - OpCode.Close(); - Mutexes.Close(); - - _smbios = null; - _open = false; } /// @@ -1071,13 +1114,16 @@ public class Computer : IComputer /// public void Reset() { - if (!_open) - return; + lock (_openLock) + { + if (!_open) + return; - StartDeferredGroupRun(); - RemoveGroups(); - AddGroups(null, CancellationToken.None); - CompleteDeferredGroupRunWhenRegistered(); + StartDeferredGroupRun(); + RemoveGroups(); + AddGroups(null, CancellationToken.None); + CompleteDeferredGroupRunWhenRegistered(); + } } private void RemoveGroups() diff --git a/LibreHardwareMonitorLib/Hardware/Cpu/GenericCpu.cs b/LibreHardwareMonitorLib/Hardware/Cpu/GenericCpu.cs index 730ecdc..64b4e90 100644 --- a/LibreHardwareMonitorLib/Hardware/Cpu/GenericCpu.cs +++ b/LibreHardwareMonitorLib/Hardware/Cpu/GenericCpu.cs @@ -9,6 +9,7 @@ using System.Diagnostics; using System.Globalization; using System.Linq; using System.Text; +using System.Threading; using System.Threading.Tasks; namespace LibreHardwareMonitor.Hardware.Cpu; @@ -40,6 +41,7 @@ public class GenericCpu : Hardware private ulong _lastTimeStampCount; private double _timeStampCounterFrequency; private Task _timeStampCounterFrequencyTask; + private CancellationTokenSource _timeStampCounterFrequencyCancellation; public GenericCpu(int processorIndex, CpuId[][] cpuId, ISettings settings) : this(processorIndex, cpuId, settings, null) @@ -109,21 +111,23 @@ public class GenericCpu : Hardware // The estimate is only a seed for the first Update() (~1 s out) and is then self-corrected from real // TSC deltas, so the ~25 ms measurement window can run off the startup critical path. startupTrace?.Skip("GenericCpu.EstimateTimeStampCounterFrequency", "Deferred to background."); + _timeStampCounterFrequencyCancellation = new CancellationTokenSource(); + CancellationToken cancellationToken = _timeStampCounterFrequencyCancellation.Token; _timeStampCounterFrequencyTask = Task.Run(() => { try { - EstimateAndStoreTimeStampCounterFrequency(affinity, null); + EstimateAndStoreTimeStampCounterFrequency(affinity, null, cancellationToken); } catch (Exception ex) { Debug.WriteLine($"Deferred TSC frequency estimation failed: {ex}"); } - }); + }, cancellationToken); } else { - EstimateAndStoreTimeStampCounterFrequency(affinity, startupTrace); + EstimateAndStoreTimeStampCounterFrequency(affinity, startupTrace, CancellationToken.None); } } } @@ -191,7 +195,7 @@ public class GenericCpu : Hardware return startupTrace != null ? startupTrace.Measure(phase, action, getDetail) : action(); } - private static void EstimateTimeStampCounterFrequency(out double frequency, out double error) + private static void EstimateTimeStampCounterFrequency(CancellationToken cancellationToken, out double frequency, out double error) { // preload the function EstimateTimeStampCounterFrequency(0, out double f, out double e); @@ -202,6 +206,10 @@ public class GenericCpu : Hardware frequency = 0; for (int i = 0; i < 5; i++) { + // Bail between measurement windows if Close() asked us to stop, so teardown is not held up. + if (cancellationToken.IsCancellationRequested) + break; + EstimateTimeStampCounterFrequency(0.025, out f, out e); if (e < error) { @@ -241,8 +249,12 @@ public class GenericCpu : Hardware error = beginError + endError; } - private void EstimateAndStoreTimeStampCounterFrequency(GroupAffinity affinity, HardwareStartupTrace startupTrace) + private void EstimateAndStoreTimeStampCounterFrequency(GroupAffinity affinity, HardwareStartupTrace startupTrace, CancellationToken cancellationToken) { + // If Close() cancelled before this ran, skip it so we neither pin a thread nor touch OpCode during teardown. + if (cancellationToken.IsCancellationRequested) + return; + GroupAffinity previousAffinity = ThreadAffinity.Set(affinity); try { @@ -250,7 +262,7 @@ public class GenericCpu : Hardware double error = 0; Measure(startupTrace, "GenericCpu.EstimateTimeStampCounterFrequency", - () => EstimateTimeStampCounterFrequency(out frequency, out error)); + () => EstimateTimeStampCounterFrequency(cancellationToken, out frequency, out error)); StoreTimeStampCounterFrequency(frequency, error); } finally @@ -271,19 +283,7 @@ public class GenericCpu : Hardware private static bool ShouldDeferTscEstimation(ISettings settings) { - string environmentValue = Environment.GetEnvironmentVariable(DeferTscEstimationEnvironmentVariable); - if (!string.IsNullOrWhiteSpace(environmentValue)) - return IsTruthy(environmentValue); - - return IsTruthy(settings.GetValue(DeferTscEstimationSetting, "false")); - } - - private static bool IsTruthy(string value) - { - return value.Equals("1", StringComparison.OrdinalIgnoreCase) - || value.Equals("true", StringComparison.OrdinalIgnoreCase) - || value.Equals("yes", StringComparison.OrdinalIgnoreCase) - || value.Equals("on", StringComparison.OrdinalIgnoreCase); + return SettingsParsing.ShouldDefer(settings, DeferTscEstimationSetting, DeferTscEstimationEnvironmentVariable); } @@ -333,15 +333,22 @@ public class GenericCpu : Hardware public override void Close() { + // Cancel and fully wait for the deferred TSC estimation before tearing down: it calls OpCode.Rdtsc and pins + // thread affinity, so it must finish (or skip on cancellation) before base.Close()/OpCode teardown rather than + // being abandoned after a fixed timeout while still touching native state. + _timeStampCounterFrequencyCancellation?.Cancel(); try { - _timeStampCounterFrequencyTask?.Wait(TimeSpan.FromMilliseconds(100)); + _timeStampCounterFrequencyTask?.Wait(); } catch (AggregateException ex) { Debug.WriteLine($"Deferred TSC frequency estimation failed while closing: {ex}"); } + _timeStampCounterFrequencyCancellation?.Dispose(); + _timeStampCounterFrequencyCancellation = null; + base.Close(); } diff --git a/LibreHardwareMonitorLib/Hardware/Cpu/IntelCpu.cs b/LibreHardwareMonitorLib/Hardware/Cpu/IntelCpu.cs index 254c753..a446755 100644 --- a/LibreHardwareMonitorLib/Hardware/Cpu/IntelCpu.cs +++ b/LibreHardwareMonitorLib/Hardware/Cpu/IntelCpu.cs @@ -587,19 +587,7 @@ internal sealed class IntelCpu : GenericCpu private static bool ShouldDeferInitialUpdate(ISettings settings) { - string environmentValue = Environment.GetEnvironmentVariable(DeferInitialUpdateEnvironmentVariable); - if (!string.IsNullOrWhiteSpace(environmentValue)) - return IsTruthy(environmentValue); - - return IsTruthy(settings.GetValue(DeferInitialUpdateSetting, "false")); - } - - private static bool IsTruthy(string value) - { - return value.Equals("1", StringComparison.OrdinalIgnoreCase) - || value.Equals("true", StringComparison.OrdinalIgnoreCase) - || value.Equals("yes", StringComparison.OrdinalIgnoreCase) - || value.Equals("on", StringComparison.OrdinalIgnoreCase); + return SettingsParsing.ShouldDefer(settings, DeferInitialUpdateSetting, DeferInitialUpdateEnvironmentVariable); } public override string GetReport() diff --git a/LibreHardwareMonitorLib/Hardware/HardwareStartupTrace.cs b/LibreHardwareMonitorLib/Hardware/HardwareStartupTrace.cs index c26c6ea..c5ce7f9 100644 --- a/LibreHardwareMonitorLib/Hardware/HardwareStartupTrace.cs +++ b/LibreHardwareMonitorLib/Hardware/HardwareStartupTrace.cs @@ -97,15 +97,7 @@ internal sealed class HardwareStartupTrace : IDisposable string settingValue = settings.GetValue(EnabledSetting, "false"); string environmentValue = Environment.GetEnvironmentVariable(EnabledEnvironmentVariable) ?? ""; - return IsTruthy(settingValue) || IsTruthy(environmentValue); - } - - private static bool IsTruthy(string value) - { - return value.Equals("1", StringComparison.OrdinalIgnoreCase) - || value.Equals("true", StringComparison.OrdinalIgnoreCase) - || value.Equals("yes", StringComparison.OrdinalIgnoreCase) - || value.Equals("on", StringComparison.OrdinalIgnoreCase); + return SettingsParsing.IsTruthy(settingValue) || SettingsParsing.IsTruthy(environmentValue); } private static string GetLogFileName(ISettings settings) diff --git a/LibreHardwareMonitorLib/Hardware/IHardwareDiscoveryTask.cs b/LibreHardwareMonitorLib/Hardware/IHardwareDiscoveryTask.cs index 4a7f3dd..e8bd01f 100644 --- a/LibreHardwareMonitorLib/Hardware/IHardwareDiscoveryTask.cs +++ b/LibreHardwareMonitorLib/Hardware/IHardwareDiscoveryTask.cs @@ -8,5 +8,13 @@ namespace LibreHardwareMonitor.Hardware; internal interface IHardwareDiscoveryTask { + /// + /// Starts the group's background hardware discovery. calls this only after it has + /// subscribed to the group's event and snapshotted the + /// already-present hardware, so each discovered piece of hardware is announced exactly once: never before the + /// subscription exists (which would drop the notification) and never also via the initial snapshot (a duplicate). + /// + void StartHardwareDiscovery(); + Task HardwareDiscoveryTask { get; } } diff --git a/LibreHardwareMonitorLib/Hardware/Memory/MemoryGroup.cs b/LibreHardwareMonitorLib/Hardware/Memory/MemoryGroup.cs index 9f06823..6b38550 100644 --- a/LibreHardwareMonitorLib/Hardware/Memory/MemoryGroup.cs +++ b/LibreHardwareMonitorLib/Hardware/Memory/MemoryGroup.cs @@ -31,6 +31,9 @@ internal class MemoryGroup : IGroup, IHardwareChanged, IHardwareDiscoveryTask private Task _hardwareDiscoveryTask = Task.CompletedTask; private Exception _lastException; private bool _disposed = false; + private readonly ISettings _settings; + private bool _hasPendingDimmDetection; + private TimeSpan _pendingDimmDetectionDelay; public MemoryGroup(ISettings settings) : this(settings, null) @@ -38,6 +41,8 @@ internal class MemoryGroup : IGroup, IHardwareChanged, IHardwareDiscoveryTask internal MemoryGroup(ISettings settings, HardwareStartupTrace startupTrace) { + _settings = settings; + Measure(startupTrace, "MemoryGroup.Driver.Configure", () => { if (DriverManager.Driver is null || !DriverManager.Driver.IsOpen) @@ -58,12 +63,16 @@ internal class MemoryGroup : IGroup, IHardwareChanged, IHardwareDiscoveryTask if (ShouldDeferDimmDetection(settings)) { + // Defer the start to StartHardwareDiscovery so Computer subscribes before any DIMM is announced. startupTrace?.Skip("MemoryGroup.DimmDetection", "Deferred to background."); - StartDimmDetectionTask(settings, TimeSpan.Zero); + _pendingDimmDetectionDelay = TimeSpan.Zero; + _hasPendingDimmDetection = true; } else if (!TryAddDimms(settings, startupTrace)) { - StartRetryTask(settings); + // Synchronous detection found nothing yet; retry in the background, also started from StartHardwareDiscovery. + _pendingDimmDetectionDelay = _retryInterval; + _hasPendingDimmDetection = true; } } @@ -76,6 +85,15 @@ internal class MemoryGroup : IGroup, IHardwareChanged, IHardwareDiscoveryTask public Task HardwareDiscoveryTask => _hardwareDiscoveryTask; + public void StartHardwareDiscovery() + { + if (!_hasPendingDimmDetection) + return; + + _hasPendingDimmDetection = false; + StartDimmDetectionTask(_settings, _pendingDimmDetectionDelay); + } + public string GetReport() { StringBuilder report = new(); @@ -151,11 +169,6 @@ internal class MemoryGroup : IGroup, IHardwareChanged, IHardwareDiscoveryTask return false; } - private void StartRetryTask(ISettings settings) - { - StartDimmDetectionTask(settings, _retryInterval); - } - private void StartDimmDetectionTask(ISettings settings, TimeSpan initialDelay) { _cancellationTokenSource = new CancellationTokenSource(); @@ -232,19 +245,7 @@ internal class MemoryGroup : IGroup, IHardwareChanged, IHardwareDiscoveryTask private static bool ShouldDeferDimmDetection(ISettings settings) { - string environmentValue = Environment.GetEnvironmentVariable(DeferDimmDetectionEnvironmentVariable); - if (!string.IsNullOrWhiteSpace(environmentValue)) - return IsTruthy(environmentValue); - - return IsTruthy(settings.GetValue(DeferDimmDetectionSetting, "false")); - } - - private static bool IsTruthy(string value) - { - return value.Equals("1", StringComparison.OrdinalIgnoreCase) - || value.Equals("true", StringComparison.OrdinalIgnoreCase) - || value.Equals("yes", StringComparison.OrdinalIgnoreCase) - || value.Equals("on", StringComparison.OrdinalIgnoreCase); + return SettingsParsing.ShouldDefer(settings, DeferDimmDetectionSetting, DeferDimmDetectionEnvironmentVariable); } private void AddDimms(List accessors, ISettings settings, HardwareStartupTrace startupTrace) diff --git a/LibreHardwareMonitorLib/Hardware/SettingsParsing.cs b/LibreHardwareMonitorLib/Hardware/SettingsParsing.cs new file mode 100644 index 0000000..ad6d972 --- /dev/null +++ b/LibreHardwareMonitorLib/Hardware/SettingsParsing.cs @@ -0,0 +1,37 @@ +// This Source Code Form is subject to the terms of the Mozilla Public License, v. 2.0. +// If a copy of the MPL was not distributed with this file, You can obtain one at http://mozilla.org/MPL/2.0/. +// Copyright (C) LibreHardwareMonitor and Contributors. + +using System; + +namespace LibreHardwareMonitor.Hardware; + +/// +/// Shared parsing for the boolean "defer detection / diagnostics" flags that several hardware components read from +/// and environment variables. Keeps the truthy vocabulary and the env-overrides-setting +/// precedence in one place so the components cannot drift. +/// +internal static class SettingsParsing +{ + /// Returns whether is one of the accepted truthy tokens (1/true/yes/on). + public static bool IsTruthy(string value) + { + return value != null + && (value.Equals("1", StringComparison.OrdinalIgnoreCase) + || value.Equals("true", StringComparison.OrdinalIgnoreCase) + || value.Equals("yes", StringComparison.OrdinalIgnoreCase) + || value.Equals("on", StringComparison.OrdinalIgnoreCase)); + } + + /// + /// Resolves a defer flag: a non-empty environment variable wins, otherwise the setting value (default "false"). + /// + public static bool ShouldDefer(ISettings settings, string settingName, string environmentVariable) + { + string environmentValue = Environment.GetEnvironmentVariable(environmentVariable); + if (!string.IsNullOrWhiteSpace(environmentValue)) + return IsTruthy(environmentValue); + + return IsTruthy(settings.GetValue(settingName, "false")); + } +} diff --git a/docs/code-review-followup.md b/docs/code-review-followup.md new file mode 100644 index 0000000..356488c --- /dev/null +++ b/docs/code-review-followup.md @@ -0,0 +1,111 @@ +# `feat-WinUI-3` code-review follow-up + +Date: 2026-06-03 +Scope reviewed: `git diff master...HEAD` (the WinUI 3 app + the `LibreHardwareMonitorLib` startup-optimization changes). + +A max-effort review of the branch produced 15 findings plus a set of investigated-and-cleared items. **All actionable findings have now been fixed** (in two passes); this document records what changed, the few things intentionally left, and the items confirmed correct so they aren't re-flagged. + +Severity legend: πŸ”΄ crash/corruption/security Β· 🟠 functional defect Β· 🟑 cleanup/efficiency. + +Verified after every change: `LibreHardwareMonitorLib.Tests` 3/3, `LibreHardwareMonitor.Windows.WinUI.Tests` 211/211; lib + WinUI + WinForms all build with 0 errors (`-c Release -p:Platform=x64`). + +--- + +## 1. Fixed β€” correctness + +### F1 πŸ”΄ `Computer.Close()` raced native teardown against in-flight deferred discovery +`LibreHardwareMonitorLib/Hardware/Computer.cs` +`CancelDeferredGroupRun()` now cancels, marks the run cancelled, then **drains** the tracked deferred tasks (`Task.WaitAll`, via `WaitForDeferredGroupTasks`) before disposing the token source β€” so a background group constructor can no longer call into OpCode/native APIs while `OpCode.Close()`/`Mutexes.Close()` run. + +### F2 πŸ”΄ `_open` was a non-volatile check-then-act with no re-entrancy guard +`LibreHardwareMonitorLib/Hardware/Computer.cs` +Added `_openLock` serializing `Open`/`OpenAsync`/`OpenInternal`/`Close`/`Reset` (body extracted into `OpenCore`), making the `_open` check-and-set atomic. Stops a `Close()`-during-async-open from no-op-and-leaking, and stops two concurrent opens from double-initializing. + +### F3 πŸ”΄ `Logger` read/wrote `_sensors`/`_identifiers` cross-thread without synchronization +`LibreHardwareMonitor.Windows.WinUI/Services/Logger.cs` +Added `_sync`; the paired arrays are published together under it, mutated under it in `SensorAdded`/`SensorRemoved`, and snapshotted under it in `Log()`. The lock is never held across `VisitComputer` or file I/O (no coupling with `Computer`'s locks β†’ no deadlock). + +### D1 🟠 `GenericCpu`'s deferred TSC task was uncancellable and only waited 100 ms on close +`LibreHardwareMonitorLib/Hardware/Cpu/GenericCpu.cs` +The estimation task now receives a `CancellationToken` (new `_timeStampCounterFrequencyCancellation`), checks it before pinning affinity and between measurement windows, and `Close()` cancels then **fully** waits for it (no 100 ms cap) before `base.Close()`. The task calls `OpCode.Rdtsc`, so it must finish or skip before teardown. + +### D2 🟠 `HardwareAdded`/`HardwareRemoved` can now fire on a background thread; WinForms consumers mutated UI unmarshaled +`LibreHardwareMonitor.Windows.Forms/UI/{MainForm,SystemTray,SensorGadget}.cs`, `Utilities/Logger.cs` +The three UI consumers capture the UI `SynchronizationContext` at construction and re-dispatch `HardwareAdded`/`HardwareRemoved` to it when invoked from another thread (a no-op in the normal synchronous case). The Forms `Logger` got the same `_sync` fix as F3. This makes the WinForms app safe when an `LHM_*_DEFER_DETECTION` env var pushes discovery onto a background thread. + +### D3 βœ… `_deferredGroupCancellationTokenSource` accessed outside `_deferredGroupLock` +Resolved by F2: all access now occurs under `_openLock` (its only callers run under that lock). + +### D4 🟠 Deferred-DIMM `HardwareAdded` ordering wasn't serialized with `AddCore` +`LibreHardwareMonitorLib/Hardware/{IHardwareDiscoveryTask,Memory/MemoryGroup,Computer}.cs` +`IHardwareDiscoveryTask` gained `StartHardwareDiscovery()`. `MemoryGroup` no longer starts its DIMM task in the constructor; it records the pending work and starts it from `StartHardwareDiscovery()`. `AddCore` now, under `_lock`, subscribes and snapshots the group's current hardware, then (outside the lock) calls `StartHardwareDiscovery()` and replays the snapshot β€” so each discovered DIMM is announced exactly once (never dropped, never duplicated). + +### D5 πŸ”΄ PBKDF2 verify accepted an empty key segment β†’ any password authenticated +`LibreHardwareMonitor.Windows.WinUI/Services/PasswordHasher.cs` +`VerifyPbkdf2` now rejects an empty salt or hash segment before the constant-time compare (an empty expected hash made `FixedTimeEquals(empty, empty)` return true). + +### D6 🟠 Run completion ignored a nested `IHardwareDiscoveryTask` registered after the snapshot +`LibreHardwareMonitorLib/Hardware/Computer.cs` +`CompleteDeferredGroupRunWhenRegistered` now re-arms (`WaitForRegisteredTasksThenComplete`): after each `Task.WhenAll` it re-checks for newly-registered tasks (comparing against the count already awaited) and only completes the run once the set is stable, so completion can't fire while discovery is still running. + +--- + +## 2. Fixed β€” cleanup / efficiency + +### D7 🟑 `IsTruthy` duplicated across files +New `LibreHardwareMonitorLib/Hardware/SettingsParsing.cs` (`IsTruthy` + `ShouldDefer`). The five library copies (`Computer`, `HardwareStartupTrace`, `MemoryGroup`, `GenericCpu`, `IntelCpu`) now call it. (The WinUI `StartupTracer` copy is in a different assembly and was left as-is.) + +### D8 🟑 `ShouldDefer*` env-then-setting pattern duplicated 4–5 ways +All collapsed to `SettingsParsing.ShouldDefer(settings, settingKey, envVar)`. + +### D9 🟑 `TrayIconService.Update()` re-rendered every GDI tray icon each tick +`SensorIconRenderer.GetRenderKey(sensor)` returns a key for the drawn text + color; `Update` reuses the existing icon and only refreshes the tooltip when the key is unchanged, skipping the DC/DIB/font/brush/HICON churn. + +### D10 🟑 `PlotTrackingService.Track()` sorted the whole history every tick +Dropped the redundant `OrderBy(value.Time)` over the full (potentially 24h) history; the latest timestamp is tracked in the same single pass and the existing final `GroupBy`/`OrderBy` still produces ordered, de-duplicated output. (A full persistent-list rewrite remains possible but wasn't needed to remove the per-tick full sort.) + +### D11 🟑 `MainWindowViewModel.UpdateStatus()` walked the whole tree each tick +Hardware/sensor counts are cached (`RefreshStatusCounts`, called from `UpdateRoot` when the tree actually changes); the per-tick `UpdateStatus` only formats the string from the cached counts. + +### D12 🟑 Dead `cancellationTokenSource == null` branch in the deferred-add helpers +Removed from `AddDeferredGroup`/`AddDeferredGroups` (unreachable since `StartDeferredGroupRun` always assigns the token source under `_openLock` before `AddGroups`). + +### Web server 🟑 fresh `JsonSerializerOptions` per `data.json` request +`RemoteWebServer` now uses a single `static readonly JsonOptions`, restoring System.Text.Json's per-options metadata cache on the frequently-polled path. + +--- + +## 3. Intentionally left (with rationale) + +- **IntelCpu clock sensors read 0 MHz briefly at startup under deferred-TSC config** (`IntelCpu.cs`): documented, intended behavior β€” the guard keeps the previous value rather than reporting 0, and it self-corrects within ~1–2 updates. +- **`Measure(trace, …)` null-guard wrapper pairs** duplicated across five hardware files: a `NoOp` null-object for `HardwareStartupTrace` (as WinUI's `NoOpStartupTracer` already does) would remove them, but the churn touches many hot construction sites for little gain. +- **WinUI `StartupTracer.IsTruthy`**: a different assembly from the library `SettingsParsing`; not worth widening the lib's public surface for one diagnostic flag. +- **`RemoteWebServer.FindSensor` is O(tree) per `/Sensor` request**: `/Sensor` is an infrequent, user-initiated control action (unlike the polled `data.json`), so an idβ†’sensor map wasn't worth the added state. + +--- + +## 4. Investigated and cleared (correct as written β€” do not re-flag) + +- **`D3DDisplayDevice` try/finally refactor** is a **leak fix** (closes the adapter on every `return` path), not a regression. +- **`Computer.GetIntelCpus()`** does **not** re-run a throwaway CPU probe β€” the synchronous `CpuGroup` is already in `_groups` before the deferred Intel-GPU task runs, so it is reused. +- **`CpuGroup`** was **not** parallelized β€” only refactored to extract `CreateCpu`. Returning `null` for unsupported AMD families preserves the original "add nothing" behavior. +- **`TreeRebuildCoalescer`** correctly coalesces a burst into ~one rebuild (single debounced worker, re-arms on a late change). +- **Disposed-`CancellationToken` reads** after cancellation are safe (`IsCancellationRequested` does not throw post-dispose). +- **Startup tracing** is fully no-op when disabled (`HardwareStartupTrace.Create` returns `null`; the `Measure` wrappers and `MainWindowViewModel.MeasureStartup` short-circuit). +- **WinUI tree updates are marshaled**: the VM's `HardwareMonitor_TreeRebuilt` handler marshals to the UI via `DispatcherQueue.TryEnqueue`. (Only `Logger` was unmarshaled β€” fixed in F3.) + +--- + +## Architectural note (A1) β€” partially addressed + +The defer-detection duplication (D7/D8) is now centralized in `SettingsParsing`, and `IHardwareDiscoveryTask` now owns the start/notify ordering (D4/D6). The deeper generalization remains available: a single group-keyed background-discovery policy (one `(settingKey, envVar)` table + a shared `IHardwareDiscoveryTask` base owning token + completion) would let a new deferrable group be added with no new constant and no new hand-written branch in `AddGroups`, and would automatically be covered by the central drain (F1) and completion accounting (D6). + +--- + +## Verify (run in an elevated VS developer shell; `-p:Platform=x64` is required for the CsWin32 SetupDi generation) + +```pwsh +dotnet test LibreHardwareMonitorLib.Tests/LibreHardwareMonitorLib.Tests.csproj -c Release -p:Platform=x64 +dotnet test LibreHardwareMonitor.Windows.WinUI.Tests/LibreHardwareMonitor.Windows.WinUI.Tests.csproj -c Release -p:Platform=x64 +dotnet build LibreHardwareMonitor.Windows.Forms/LibreHardwareMonitor.Windows.Forms.csproj -c Release -p:Platform=x64 +```