fix: UI timer disposal, PerformanceCounter blocking, and diagnostics - #23
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 (6)
WalkthroughThe changes introduce thread-safe, encapsulated point management in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ 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 application's stability, performance, and robustness. It addresses critical issues such as memory leaks and UI unresponsiveness, while also improving error reporting through comprehensive diagnostics. Furthermore, it introduces thread-safe mechanisms for fan curve management and hardware interaction, laying a more resilient foundation for future development. 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 introduces several important fixes and improvements, including resolving a memory leak in timerRefreshStats, preventing UI blocking from PerformanceCounter, enhancing thread safety in AsusControl and FanCurve, and adding more detailed diagnostics. The changes are well-structured and address the stated goals.
My review focuses on ensuring the new thread-safety mechanisms are robust. I've identified a critical race condition in AsusControl where several methods could attempt to use hardware resources after they have been disposed. I've also pointed out a couple of other areas for improvement regarding an asynchronous method implementation and code redundancy. Addressing these points will help solidify the stability of the application.
| if (_disposed) return; | ||
| AsusWinIO64.HealthyTable_SetFanIndex(fanIndex); | ||
| AsusWinIO64.HealthyTable_SetFanTestMode(value > 0 ? FanModeManual : FanModeDefault); | ||
| AsusWinIO64.HealthyTable_SetFanPwmDuty(value); | ||
| lock (_hwLock) | ||
| { | ||
| AsusWinIO64.HealthyTable_SetFanIndex(fanIndex); | ||
| AsusWinIO64.HealthyTable_SetFanTestMode(value > 0 ? FanModeManual : FanModeDefault); | ||
| AsusWinIO64.HealthyTable_SetFanPwmDuty(value); | ||
| } |
There was a problem hiding this comment.
There's a potential race condition here. The _disposed check is outside the lock. A thread could pass the _disposed check, then another thread could call Dispose(), which would de-initialize the hardware resources. The first thread would then acquire the lock and attempt to use the de-initialized resources, which could lead to a crash or undefined behavior.
To fix this, the _disposed check should be moved inside the lock block. This same issue exists in other methods like GetFanSpeed and Thermal_Read_Cpu_Temperature.
lock (_hwLock)
{
if (_disposed) return;
AsusWinIO64.HealthyTable_SetFanIndex(fanIndex);
AsusWinIO64.HealthyTable_SetFanTestMode(value > 0 ? FanModeManual : FanModeDefault);
AsusWinIO64.HealthyTable_SetFanPwmDuty(value);
}| if (_disposed) return 0; | ||
| AsusWinIO64.HealthyTable_SetFanIndex(fanIndex); | ||
| var fanSpeed = AsusWinIO64.HealthyTable_FanRPM(); | ||
| return fanSpeed; | ||
| lock (_hwLock) | ||
| { | ||
| AsusWinIO64.HealthyTable_SetFanIndex(fanIndex); | ||
| var fanSpeed = AsusWinIO64.HealthyTable_FanRPM(); | ||
| return fanSpeed; | ||
| } |
There was a problem hiding this comment.
Similar to SetFanSpeed, there is a race condition here with Dispose(). The _disposed check is outside the lock block. It should be moved inside to ensure that the hardware is not accessed after being disposed.
lock (_hwLock)
{
if (_disposed) return 0;
AsusWinIO64.HealthyTable_SetFanIndex(fanIndex);
var fanSpeed = AsusWinIO64.HealthyTable_FanRPM();
return fanSpeed;
}| if (_disposed) return 0; | ||
| return AsusWinIO64.Thermal_Read_Cpu_Temperature(); | ||
| lock (_hwLock) | ||
| { | ||
| return AsusWinIO64.Thermal_Read_Cpu_Temperature(); | ||
| } |
There was a problem hiding this comment.
Similar to SetFanSpeed, there is a race condition here with Dispose(). The _disposed check is outside the lock block. It should be moved inside to prevent accessing disposed resources.
lock (_hwLock)
{
if (_disposed) return 0;
return AsusWinIO64.Thermal_Read_Cpu_Temperature();
}| public Task ResetToDefaultAsync() | ||
| { | ||
| if (_disposed) return Task.CompletedTask; | ||
| ResetToDefaultInternal(); | ||
| return Task.CompletedTask; | ||
| } |
There was a problem hiding this comment.
The method ResetToDefaultAsync is not truly asynchronous and has a potential thread-safety issue. It calls ResetToDefaultInternal() directly without acquiring the _hwLock, while other methods do. This could lead to race conditions. Additionally, it blocks the calling thread because ResetToDefaultInternal is synchronous.
To fix this, you should call the public ResetToDefault() method inside Task.Run to make it both thread-safe and truly asynchronous.
public Task ResetToDefaultAsync()
{
if (_disposed) return Task.CompletedTask;
return Task.Run(() => ResetToDefault());
}| private static int _instanceCount = 0; | ||
| private readonly int _fanCount; | ||
| private bool _disposed = false; | ||
| private bool _disposing = false; |
There was a problem hiding this comment.
The _disposing flag is set but its value is never used. It's set to true at the beginning of the Dispose method's lock (line 52) and false at the end (line 79), but no other code path reads it. Any concurrent calls would be blocked by the lock and then stopped by the _disposed check. This flag can be safely removed to simplify the code, along with the lines where it's assigned.
| try | ||
| { | ||
| asusControl.ResetToDefault(); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Debug.WriteLine($"[Dispose] Reset error: {ex.Message}"); | ||
| } |
There was a problem hiding this comment.
This explicit call to asusControl.ResetToDefault() is redundant. The updated AsusControl.Dispose() method now handles resetting the fans to default when the last instance is disposed. Relying on the Dispose method's logic will make the code cleaner and avoid multiple reset calls. This try-catch block can be removed.
![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: 45e2151b40
ℹ️ 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 (_instanceCount == 0) | ||
| { | ||
| try | ||
| { | ||
| ResetToDefaultInternal(); | ||
| } |
There was a problem hiding this comment.
Dispose(bool) now resets all fans whenever the last AsusControl instance is disposed, which breaks the existing CLI flow that intentionally skips reset for --set-fan-speeds / --set-fan-speed before calling Dispose() in Program.Main. As a result, commands meant to leave manual fan settings in effect are always undone at process exit.
Useful? React with 👍 / 👎.
| try | ||
| { | ||
| asusControl.ResetToDefault(); | ||
| } |
There was a problem hiding this comment.
The form disposal path now calls asusControl.ResetToDefault() unconditionally, so even when turnOffControlOnExit is disabled the app still resets fan control on close. This makes the setting ineffective for normal GUI shutdown and regresses the behavior users can configure from the menu.
Useful? React with 👍 / 👎.
Co-authored-by: beenycool <129210955+beenycool@users.noreply.github.com>
- Add static instance counting to prevent multiple WinIo initializations - Add lock to all hardware I/O operations (SetFanSpeed, GetFanSpeed, Thermal_Read_Cpu_Temperature) - Fix finalizer to only call ShutdownWinIo from explicit Dispose(true) - Add ResetToDefaultAsync for interface compatibility alongside sync ResetToDefault - Add Debug.WriteLine diagnostics for disposal errors
- 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
- Fix timerRefreshStats to dispose timer before replacing (memory leak fix) - Wrap PerformanceCounter.NextValue() in Task.Run to avoid UI blocking - Add Debug.WriteLine diagnostics to all catch blocks - Enforce minimum 2 points when deleting fan curve points - Update UI callers to use thread-safe FanCurve APIs
45e2151 to
cc36cd1
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| int HealthyTable_FanCounts(); | ||
| ulong Thermal_Read_Cpu_Temperature(); | ||
| Task ResetToDefaultAsync(); | ||
| void ResetToDefault(); |
There was a problem hiding this comment.
WARNING: Breaking API change - Adding ResetToDefault() to the interface requires all implementations to add this method. Ensure all implementing classes are updated.
Code Review SummaryStatus: 1 New Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues from other reviewers that were addressed in current code:
Issues from other reviewers that remain (already documented):
Files Reviewed (6 files)
Reviewed by minimax-m2.5-20260211 · 785,727 tokens |
1 participant
Summary
Summary by CodeRabbit
New Features
Bug Fixes
Improvements