fix: dispose resource leaks in NetworkSharedState, TrayIconService, MemoryTestService#347
Conversation
…emoryTestService - NetworkSharedState: dispose SKTypeface on LegendTextPaint to release unmanaged SkiaSharp memory (LEAK-003) - TrayIconService: dispose icon resource stream after creating System.Drawing.Icon (LEAK-006) - MemoryTestService: dispose Process returned by Process.Start when launching mdsched.exe (LEAK-007) Closes #305, Closes #321
📝 WalkthroughWalkthroughThis PR fixes three resource leaks by ensuring proper disposal of unmanaged objects: a Process handle in MemoryTestService, an icon resource stream in TrayIconService, and a SkiaSharp typeface in NetworkSharedState. The changelog documents these fixes in a new 0.53.1 release. ChangesResource Leak Fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SysManager/SysManager/ViewModels/NetworkSharedState.cs`:
- Around line 491-493: Dispose the four SolidColorPaint instances owned by
NetworkSharedState in its Dispose() method: call Dispose() (and null them) on
LegendTextPaint, LegendBackgroundPaint, TooltipTextPaint, and
TooltipBackgroundPaint in addition to disposing LegendTextPaint.SKTypeface;
ensure Dispose() is idempotent by null-checking each paint before calling
Dispose() so repeated disposals are safe (update the Dispose method where
LegendTextPaint.SKTypeface is currently disposed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a700e5f8-1a94-4974-a6c4-0b2203f4141a
📒 Files selected for processing (4)
CHANGELOG.mdSysManager/SysManager/Services/MemoryTestService.csSysManager/SysManager/Services/TrayIconService.csSysManager/SysManager/ViewModels/NetworkSharedState.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & unit tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
SysManager/SysManager/Services/MemoryTestService.cs (1)
83-88: LGTM!SysManager/SysManager/Services/TrayIconService.cs (1)
93-96: LGTM!CHANGELOG.md (1)
9-17: LGTM!
| // Dispose SKTypeface (unmanaged SkiaSharp memory) — LEAK-003 | ||
| LegendTextPaint.SKTypeface?.Dispose(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "NetworkSharedState.cs" -type fRepository: laurentiu021/SystemManager
Length of output: 127
🏁 Script executed:
cat -n ./SysManager/SysManager/ViewModels/NetworkSharedState.cs | head -100Repository: laurentiu021/SystemManager
Length of output: 4998
🏁 Script executed:
wc -l ./SysManager/SysManager/ViewModels/NetworkSharedState.csRepository: laurentiu021/SystemManager
Length of output: 131
🏁 Script executed:
tail -20 ./SysManager/SysManager/ViewModels/NetworkSharedState.csRepository: laurentiu021/SystemManager
Length of output: 744
🏁 Script executed:
cat -n ./SysManager/SysManager/ViewModels/NetworkSharedState.cs | sed -n '480,494p'Repository: laurentiu021/SystemManager
Length of output: 572
🏁 Script executed:
rg "SolidColorPaint" ./SysManager/SysManager/ViewModels/NetworkSharedState.cs -B 2 -A 2Repository: laurentiu021/SystemManager
Length of output: 2272
🏁 Script executed:
web_search
SKPaint SkiaSharp disposal IDisposableRepository: laurentiu021/SystemManager
Length of output: 167
🌐 Web query:
SkiaSharp SKPaint Dispose IDisposable memory management
💡 Result:
SKPaint in SkiaSharp implements IDisposable and must be explicitly disposed to release unmanaged native resources promptly, preventing memory leaks especially on resource-constrained devices like mobile.[1][2][3][4] The official documentation states: "Always dispose the object before you release your last reference to the SKPaint. Otherwise, the resources it is using will not be freed until the garbage collector calls the finalizer."[1][5] SKPaint inherits from SKNativeObject, which provides Dispose to release both managed and unmanaged resources, and a protected override Dispose(bool disposing) for the dispose pattern.[1][6][2] Failing to call Dispose delays native resource cleanup until GC finalization, which is non-deterministic and can lead to higher memory usage.[7][8][3] Best practices include: - Use 'using' statements for automatic disposal: using (var paint = new SKPaint) {... }[4] - Cache and reuse SKPaint objects for repeated use (e.g., same stroke color/style), as they are lightweight and do not maintain state like SKCanvas.[6][3][4] Dispose only when no longer needed. - Avoid recreating SKPaint in hot loops; dispose explicitly after caching or when changing properties significantly.[3][4] GC will eventually finalize undisposed objects, but explicit Dispose ensures deterministic cleanup.[9][8][3] Recent issues highlight keeping objects alive during native calls with GC.KeepAlive, but disposal remains essential post-use.[10]
Citations:
- 1: https://learn.microsoft.com/ga-ie/dotnet/api/skiasharp.skpaint.dispose?view=skiasharp-2.80
- 2: https://learn.microsoft.com/en-us/dotnet/api/skiasharp.skpaint?view=skiasharp-3.119
- 3: [QUESTION] What needs to be disposed and what can be cached? mono/SkiaSharp#829
- 4: https://skiasharp.com/
- 5: https://learn.microsoft.com/vi-vn/dotnet/api/skiasharp.skpaint.dispose?view=skiasharp-3.119&viewFallbackFrom=skiasharp-1.68
- 6: https://developer.xamarin.com/api/type/SkiaSharp.SKPaint/
- 7: https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose
- 8: https://stackoverflow.com/questions/5462742/c-sharp-disposing-idisposable
- 9: [BUG] Memory leak when create and dispose SKBitmap and SKCanvas using SkiaSharp mono/SkiaSharp#1541
- 10: [BUG]
SKNativeObjectinstances are not kept alive during native API calls mono/SkiaSharp#2821
🏁 Script executed:
web_search
LiveChartsCore SolidColorPaint Dispose IDisposableRepository: laurentiu021/SystemManager
Length of output: 174
🌐 Web query:
LiveChartsCore.SkiaSharpView.Painting SolidColorPaint Dispose
💡 Result:
In LiveCharts2, LiveChartsCore.SkiaSharpView.Painting.SolidColorPaint inherits Dispose/Dispose semantics from LiveChartsCore.SkiaSharpView.Painting.SkiaPaint, and you should not treat SolidColorPaint as something you manually dispose in normal UI usage. SolidColorPaint itself is a paint wrapper that sets its color during painting (it updates an internal Skia paint’s Color in OnPaintStarted/Transitionate) and does not create/dispose any Skia shader resources the way gradient paints do [1]. Disposal behavior: - The SolidColorPaint API docs show a Dispose member (inherited from its base) described as “freeing, releasing, or resetting unmanaged resources” [2]. - In the library source, gradient paints override DisposeTask to dispose their internal shader objects (example: LinearGradientPaint disposes _shader in both Transitionate and DisposeTask), which indicates why gradient paints may need disposal if you keep them around for a long time or create many of them [3]. SolidColorPaint does not show such shader disposal logic in its implementation [1]. Practical guidance: - If you pass SolidColorPaint instances as Fill/Stroke in your chart/series, LiveCharts is expected to manage their lifecycle; creating them once and reusing them is typically preferred to repeated allocations. - Only call Dispose if you explicitly created a paint and own its lifecycle outside LiveCharts (e.g., you are dynamically creating many paints and want deterministic cleanup). Otherwise, prefer letting LiveCharts manage them. If you tell me your UI framework (WPF/WinForms/MAUI/etc.) and whether you reuse the same SolidColorPaint instance or create one per render/update, I can give more targeted advice on whether disposing is likely to matter in your scenario.
Citations:
- 1: https://github.com/beto-rodriguez/LiveCharts2/blob/master/src/skiasharp/LiveChartsCore.SkiaSharp/Painting/SolidColorPaint.cs
- 2: https://livecharts.dev/api/2.0.0-rc5.4/LiveChartsCore.SkiaSharpView.Painting.SolidColorPaint
- 3: https://github.com/beto-rodriguez/LiveCharts2/blob/master/src/skiasharp/LiveChartsCore.SkiaSharp/Painting/LinearGradientPaint.cs
Dispose all owned paint resources, not only the typeface.
Line 492 fixes only part of LEAK-003. NetworkSharedState owns LegendTextPaint, LegendBackgroundPaint, TooltipTextPaint, and TooltipBackgroundPaint (lines 64-67), which should be disposed in Dispose() as well. All four are SolidColorPaint instances created and reused by the class; they should be disposed when the class is disposed.
Proposed fix
public void Dispose()
{
Pinger.SampleReceived -= OnSample;
TraceMonitor.RouteCompleted -= OnRouteCompleted;
Pinger.Stop();
Pinger.Dispose();
TraceMonitor.Stop();
TraceMonitor.Dispose();
FlushTimer?.Stop();
// Dispose SKTypeface (unmanaged SkiaSharp memory) — LEAK-003
LegendTextPaint.SKTypeface?.Dispose();
+ LegendTextPaint.Dispose();
+ LegendBackgroundPaint.Dispose();
+ TooltipTextPaint.Dispose();
+ TooltipBackgroundPaint.Dispose();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Dispose SKTypeface (unmanaged SkiaSharp memory) — LEAK-003 | |
| LegendTextPaint.SKTypeface?.Dispose(); | |
| } | |
| // Dispose SKTypeface (unmanaged SkiaSharp memory) — LEAK-003 | |
| LegendTextPaint.SKTypeface?.Dispose(); | |
| LegendTextPaint.Dispose(); | |
| LegendBackgroundPaint.Dispose(); | |
| TooltipTextPaint.Dispose(); | |
| TooltipBackgroundPaint.Dispose(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SysManager/SysManager/ViewModels/NetworkSharedState.cs` around lines 491 -
493, Dispose the four SolidColorPaint instances owned by NetworkSharedState in
its Dispose() method: call Dispose() (and null them) on LegendTextPaint,
LegendBackgroundPaint, TooltipTextPaint, and TooltipBackgroundPaint in addition
to disposing LegendTextPaint.SKTypeface; ensure Dispose() is idempotent by
null-checking each paint before calling Dispose() so repeated disposals are safe
(update the Dispose method where LegendTextPaint.SKTypeface is currently
disposed).
## Summary Two fixes in one PR. ### #333 — Traceroute chart not rendering The LiveChartsCore CartesianChart collapsed to zero height when the parent layout didn't provide explicit vertical space. Added MinHeight=250 to ensure the chart always has room to render. ### #334 — Speed Test HTTP values too low (500-600 vs 900+ Mbps) Increased parallel download streams from 4 to 8 and total payload from 25 MB to 50 MB. On high-speed links (1 Gbps+), 4 streams with 6.25 MB each couldn't saturate the connection. 8 streams with 6.25 MB each (50 MB total) provides enough concurrency to measure accurately. ## Files changed - TracerouteView.xaml — added MinHeight=250 to chart - SpeedTestService.cs — DownloadConnections 4→8, PayloadBytes 25→50 MB Closes #333, Closes #334 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
CHANGELOG entry was incorrectly labeled v0.28.5 — the last fix: merge (PR #347) triggered v0.28.6. Corrects the version header. Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
…emoryTestService (#347) - NetworkSharedState: dispose SKTypeface on LegendTextPaint to release unmanaged SkiaSharp memory (LEAK-003) - TrayIconService: dispose icon resource stream after creating System.Drawing.Icon (LEAK-006) - MemoryTestService: dispose Process returned by Process.Start when launching mdsched.exe (LEAK-007) Closes #305, Closes #321 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
2 participants
Summary
Fix 3 resource leaks identified in code review (LEAK-003, LEAK-006, LEAK-007).
Changes
Testing
Closes #305, Closes #321
Summary by CodeRabbit