fix: FanCurve thread safety with proper locking - #22
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR enhances thread-safety and lifecycle management in AsusControl by adding WinIO shutdown synchronization via static flags and Monitor coordination, instance tracking, and safe disposal patterns. It also adds validation and cloning semantics to FanCurve points and updates curve initialization to sort points by temperature. Changes
Sequence DiagramsequenceDiagram
participant C1 as Constructor 1
participant C2 as Constructor 2
participant Monitor as Hardware Lock<br/>(Monitor)
participant Dispose as Dispose (Last<br/>Instance)
participant WinIO as WinIO Shutdown
C1->>Monitor: Acquire _hardwareLock
C1->>Monitor: Check _shutdownInProgress<br/>(false, proceed)
C1->>WinIO: Initialize WinIO
C1->>Monitor: Release _hardwareLock
C2->>Monitor: Acquire _hardwareLock
C2->>Monitor: Check _shutdownInProgress<br/>(false, proceed)
C2->>WinIO: Initialize WinIO
C2->>Monitor: Release _hardwareLock
Dispose->>Monitor: Acquire _hardwareLock
Dispose->>Monitor: Decrement _instanceCount<br/>(reaches 0)
Dispose->>Monitor: Set _shutdownInProgress = true
Dispose->>Monitor: Release _hardwareLock
C1->>Monitor: (New) Acquire _hardwareLock
C1->>Monitor: Check _shutdownInProgress<br/>(true)
C1->>Monitor: Monitor.Wait() - BLOCK
Dispose->>WinIO: ResetToDefaultInternal()
Dispose->>WinIO: ShutdownWinIo()
Dispose->>Monitor: Acquire _hardwareLock
Dispose->>Monitor: Set _shutdownInProgress = false
Dispose->>Monitor: Monitor.PulseAll()
Dispose->>Monitor: Release _hardwareLock
C1->>Monitor: Unblocked, Acquire _hardwareLock
C1->>WinIO: Initialize WinIO (new instance)
C1->>Monitor: Release _hardwareLock
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the thread safety of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves thread safety for hardware access and fan curve management by introducing proper locking. The changes in AsusControl.cs correctly use a lock for hardware calls and manage the lifecycle of the WinIo library with an instance counter. Similarly, FanCurve.cs is refactored to be thread-safe for all its operations. My review includes a critical fix for a performance anti-pattern where a lock is held during a Thread.Sleep, and a minor code cleanup suggestion.
| private void ResetToDefaultInternal() | ||
| { | ||
| var fanCount = _fanCount; | ||
| for (byte fanIndex = 0; fanIndex < fanCount; fanIndex++) | ||
| { | ||
| SetFanSpeed(0, fanIndex); | ||
| // Minimal blocking delay to ensure hardware processes the command if needed, | ||
| // but keep it fast for shutdown. | ||
| try | ||
| { | ||
| AsusWinIO64.HealthyTable_SetFanIndex(fanIndex); | ||
| AsusWinIO64.HealthyTable_SetFanTestMode(FanModeDefault); | ||
| AsusWinIO64.HealthyTable_SetFanPwmDuty(0); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Debug.WriteLine($"[AsusControl] Failed to reset fan {fanIndex}: {ex.Message}"); | ||
| } | ||
| System.Threading.Thread.Sleep(ResetCommandDelayMs); | ||
| } | ||
| } |
There was a problem hiding this comment.
This method is called from within a lock by ResetToDefault and Dispose, but it contains a Thread.Sleep. Holding a lock while sleeping is an anti-pattern that can cause performance issues and thread contention, as it blocks other threads from accessing the hardware for an unnecessarily long time. To fix this, this method should perform its own locking inside the loop for the brief hardware access, and the callers should be updated to not lock around the call to this method. This suggestion refactors this method to perform its own locking; please ensure the callers (ResetToDefault and Dispose) are updated accordingly to not lock around this method call.
private void ResetToDefaultInternal()
{
var fanCount = _fanCount;
for (byte fanIndex = 0; fanIndex < fanCount; fanIndex++)
{
try
{
lock (_hwLock)
{
if (_disposed) return;
AsusWinIO64.HealthyTable_SetFanIndex(fanIndex);
AsusWinIO64.HealthyTable_SetFanTestMode(FanModeDefault);
AsusWinIO64.HealthyTable_SetFanPwmDuty(0);
}
}
catch (Exception ex)
{
Debug.WriteLine($"[AsusControl] Failed to reset fan {fanIndex}: {ex.Message}");
}
System.Threading.Thread.Sleep(ResetCommandDelayMs);
}
}| List<FanCurvePoint> sortedPoints; | ||
| lock (_lock) | ||
| { | ||
| if (_points == null || _points.Count == 0) |
There was a problem hiding this comment.
The _points field is initialized with a new list and is never reassigned or set to null. Therefore, the _points == null check is redundant and can be removed for improved code clarity.
if (_points.Count == 0)![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 460792ce0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (disposing) | ||
| { | ||
| _instanceCount--; | ||
| if (_instanceCount == 0) |
There was a problem hiding this comment.
The new Dispose(bool disposing) logic only decrements _instanceCount when disposing is true, so objects finalized without an explicit Dispose() leave the static count permanently elevated. After that happens once, later disposals may never hit _instanceCount == 0, which skips both ResetToDefaultInternal() and ShutdownWinIo(), leaving fan state and driver lifetime management inconsistent for the rest of the process.
Useful? React with 👍 / 👎.
| public Task ResetToDefaultAsync() | ||
| { | ||
| if (_disposed) return Task.CompletedTask; | ||
| ResetToDefaultInternal(); |
There was a problem hiding this comment.
ResetToDefaultAsync() calls ResetToDefaultInternal() without taking _hwLock, unlike other hardware-touching methods. If this async API is called concurrently with SetFanSpeed, GetFanSpeed, or Dispose, P/Invoke calls can interleave with ShutdownWinIo() and reintroduce the same hardware race conditions this change is trying to eliminate.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AsusFanControl.Core/AsusControl.cs`:
- Around line 83-91: Acquire the hardware lock before checking disposal in
SetFanSpeed: move the if (_disposed) return; inside the lock block so you
lock(_hwLock) { if (_disposed) return; ... } and then call
AsusWinIO64.HealthyTable_*; apply the same change to the other fan-control
methods that take _hwLock and call AsusWinIO64 P/Invoke (the similar methods
that perform fan PWM/tacho/test-mode updates) so disposal is always checked
while holding _hwLock.
- Around line 162-166: ResetToDefaultAsync currently calls
ResetToDefaultInternal() directly, bypassing the synchronization and disposal
checks used elsewhere; change ResetToDefaultAsync to reuse the public
ResetToDefault() method (which performs the locking/disposal validation) instead
of calling ResetToDefaultInternal(), so ResetToDefaultAsync invokes
ResetToDefault() and then returns a completed Task, preserving thread-safety
with SetFanSpeed, GetFanSpeed, and Dispose checks.
- Around line 23-31: The constructor increments _instanceCount while holding
_hwLock but calls AsusWinIO64.HealthyTable_FanCounts() outside the lock, which
can leave _instanceCount inconsistent if the probe fails; fix by performing the
fan-count probe and any initialization inside the same _hwLock critical section
(or, alternatively, if you prefer to keep the increment before probing, catch
any exception from HealthyTable_FanCounts(), decrement _instanceCount and
rethrow), so update the code around _hwLock/_instanceCount to call
AsusWinIO64.InitializeWinIo() and AsusWinIO64.HealthyTable_FanCounts() inside
the lock (or ensure a rollback decrement of _instanceCount on probe failure) and
assign _fanCount only after a successful probe.
In `@AsusFanControl.Core/FanCurve.cs`:
- Around line 97-109: SetPoints currently clears and mutates _points while
enumerating newPoints, which can leave the curve partially replaced if
enumeration throws or contains nulls; instead, materialize newPoints into a
temporary list (e.g., var temp = new List<FanCurvePoint>()), validate each
element (ensure not null and its Temperature/Speed are valid) while cloning
values with new FanCurvePoint(p.Temperature, p.Speed), and only after successful
materialization and validation enter the lock and replace _points atomically
(clear and add range or swap contents) under _lock to avoid partial updates.
- Around line 26-33: The Points getter currently returns a shallow copy of the
_points list allowing callers to mutate the same FanCurvePoint instances; modify
the Points property to produce and return deep copies while holding the lock
(e.g., lock(_lock) { return _points.Select(p => new FanCurvePoint(p.Temperature,
p.FanSpeed /*or use a Clone() method*/)).ToList(); }), or alternatively make
FanCurvePoint immutable and construct new instances when mutating the curve;
ensure all copies are created inside the lock to prevent race conditions and
reference the _points, _lock, Points and FanCurvePoint identifiers when applying
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0d371c4-2426-41e6-9499-d01b93a2fb0a
📒 Files selected for processing (3)
AsusFanControl.Core/AsusControl.csAsusFanControl.Core/FanCurve.csAsusFanControl.Core/IFanController.cs
460792c to
5f2f419
Compare
Code Review SummaryStatus: Issues Already Documented | Recommendation: Address existing comments before merge OverviewThe PR implements thread-safety improvements for FanCurve and AsusControl. The existing review comments already identify the critical issues in this PR.
Existing Critical Issues (already documented)
VerificationVerified the following fixes in the current diff:
Files Reviewed (6 files)
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
|
|
||
| _fanCount = AsusWinIO64.HealthyTable_FanCounts(); | ||
| _instanceCount++; |
There was a problem hiding this comment.
CRITICAL: Instance count leak on exception
If AsusWinIO64.HealthyTable_FanCounts() throws after _instanceCount++ has already executed, the catch block does not decrement _instanceCount. This leaves the count elevated, causing subsequent instances to skip initialization (since _instanceCount > 0) while WinIO is actually uninitialized.
| @@ -156,75 +230,89 @@ public void SetFanSpeed(int percent, byte fanIndex = 0) | |||
| private async Task SetFanSpeeds(byte value) | |||
There was a problem hiding this comment.
WARNING: Missing bounds check on fanIndex
No validation that fanIndex is within valid range (0 to _fanCount - 1). An out-of-range index could cause undefined behavior with the hardware P/Invoke calls.
| public ulong Thermal_Read_Cpu_Temperature() | ||
| { | ||
| if (_disposed) return 0; | ||
| lock (_hardwareLock) |
There was a problem hiding this comment.
WARNING: Missing bounds check on fanIndex
No validation that fanIndex is within valid range (0 to _fanCount - 1). An out-of-range index could cause undefined behavior with the hardware P/Invoke calls.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (6 files)
Reviewed by minimax-m2.5-20260211 · 189,149 tokens |
- Make Points property private with thread-safe getter returning a copy - Add thread-safe APIs: SetPoints, AddPoint, RemovePointAt, UpdatePointAt, PointCount, ClearPoints - All internal operations now lock properly to prevent concurrent modification
5f2f419 to
92f5c01
Compare
| { | ||
| Trace.TraceError($"[AsusControl] Error shutting down WinIo: {ex}"); | ||
| Debug.WriteLine($"[AsusControl] Error shutting down WinIo: {ex.Message}"); | ||
| } |
There was a problem hiding this comment.
WARNING: Silent exception swallowing - If ShutdownWinIo() throws during dispose, the exception is caught and logged with Debug.WriteLine but not re-thrown. This could mask serious hardware failures during shutdown. Consider logging at error level or re-throwing if shutdown failure is critical.
1 participant
Summary
Summary by CodeRabbit