Repository navigation
[Enhancement]: Cross-app — 39 unfiltered catch (Exception) blocks and 17 silent catches, with no guard #2615
Description
Activity
Fixed in #2664.
Empty catches. Fifteen had nothing in their body when I got to this: three of the 17 above went with the code around them in #2638 and #2651, and
SpeedTestServicehad one with awhenfilter that the count missed. The one inGamingProfileServicecould not run (Process.Exited's remove accessor does not throw, whatever state the process is in) and is gone. The other fourteen say why in a comment rather than logging, because each is an expected case in a cleanup or a scan's inner loop (a process that had already exited, a tab closing, a folder the scan cannot list), and that is how the app's other deliberate swallows already read.Unfiltered
catch (Exception). Two are narrowed to what their call throws:PingMonitorService, wherePinghands back a failed lookup or send asPingException, andThemeService.Save(IOException,UnauthorizedAccessException). The other 37 stay broad, with App.xaml.cs's three and the twocatchblocks with no type that release what they built and rethrow, and each of those 42 now says why beside the code.UpdateService.GetLatestAsyncis one of them: reading the body fails as anIOException, anInvalidDataExceptionor anInvalidOperationExceptionas well as aJsonException, and its siblingGetRecentAsynccatches only the last, which is #2663.What holds it.
ArchitectureTests.EveryCatch_SaysWhy_AndTheBroadOnesAreTheReviewedOnesfails the build on an empty catch body, on a catch of everything with no comment directly above it or first in its body, and on any file whose number of those is not the reviewed one, so the list only shrinks. CONTRIBUTING's "No silent failures" describes the rule.
Problem
The project's own rule is specific exception types only, and an empty catch at least logs at Debug. Today, outside the three in
App.xaml.cs, each of which says why it is broad, the app has 39catch (Exception …)blocks with nowhenfilter, and 17 typed catches with an empty body. Some are there on purpose and say so (LibreHardwareMonitor and NvAPI can throw anything from native code, andViewModelBase.InitializeAsyncis the last-resort net), but nothing tells those apart from the rest, and no test stops a new one.catch (Exception):Services/EtwBandwidthSource.cs,GamingProfileService.cs(3),PingMonitorService.cs,ResourceHistoryService.cs(3),TemperatureService.cs(5),ThemeService.cs(2),TrayIconService.cs(2),UpdateService.cs,ViewModels/AudioMixerViewModel.cs(2),BandwidthMonitorViewModel.cs,BulkInstallerViewModel.cs(3),DashboardViewModel.cs(8),DnsHostsViewModel.cs(4),ProcessManagerViewModel.cs,StandbyMemoryViewModel.cs,ViewModelBase.cs.Helpers/AtomicFile.cs,Helpers/ExplorerShell.cs,Services/AudioMixerService.cs(2),AudioPolicyConfig.cs,GamingProfileService.cs,LargeFileScanner.cs(6),PowerShellRunner.cs(2),ServiceManagerService.cs,ViewModels/DnsHostsViewModel.cs(2).Proposed solution
Go through each: narrow it to the types the call can throw, or keep it broad with a
whenfilter or a one-line reason where the code wraps native or third-party code that can throw anything. Empty catches log at Debug. Then add anArchitectureTestsguard that fails on a new unfilteredcatch (Exception)or empty catch, with the reviewed ones listed by file and reason, so the list only shrinks.Affected tab
Cross-app