Fix Possible OperationCancelException for WaitAsync Operations #4072
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Removed the `ResetCache` method and its associated calls to streamline caching management and improve performance. Refactored the `QueryAsync` method for better thread safety, synchronization, and exception handling. Simplified UWP and Win32 program filtering logic and removed redundant code. Eliminated manual cache disposal and reset logic, favoring a more efficient and automated caching mechanism. These changes enhance maintainability, responsiveness, and overall plugin performance.
The caching-related code, including `cacheOptions` and `cache`, has been removed from `Main.cs`, indicating that the caching mechanism is no longer in use or has been refactored elsewhere. Additionally, the `emptyResults` list is now explicitly initialized as an empty list (`[]`). No changes were made to the `Context` property or the `commonUninstallerNames` array.
Introduced `_lastIndexTimeLock` to ensure thread-safe access and updates to `_settings.LastIndexTime`, preventing race conditions. Refactored reindexing logic to use a `lock` block for evaluating and updating the reindexing condition. Added `emptyResults` as a static readonly placeholder list. Improved code clarity and maintainability without altering existing functionality.
Added detailed debug logging to `IndexWin32ProgramsAsync` and `IndexUwpProgramsAsync` to track the indexing process, including preparation, start, retrieval, caching, and completion. Replaced direct updates to `_settings.LastIndexTime` with a thread-safe lock to prevent race conditions. Enhanced `IndexProgramsAsync` with a debug log to indicate the start of indexing for better traceability. Updated program retrieval logic to process Win32 and UWP programs in parallel with cancellation support and applied the `HideUninstallersFilter` for cleaner results.
Introduced a MemoryCache to improve QueryAsync performance by caching query results, reducing redundant computations. Added a ResetCache method to reinitialize the cache when settings are updated. Integrated cache reset calls into settings property setters to ensure consistency. Refactored query logic to leverage MemoryCache.GetOrCreateAsync for streamlined caching. Removed redundant code and debug logging for improved readability and maintainability. Ensured thread safety with proper locking mechanisms. Simplified and consolidated caching logic for better maintainability.
Enhanced logging to provide better traceability and insights: - Added debug logs for query reception, cache misses, and lock acquisition. - Logged query cancellation and completion with result counts. - Added logs for caching results, including item counts and query details. - Improved logging for filtering and program selection processes. - Ensured no functional changes to existing query and filtering logic.
Added a debug log statement to indicate when the lock for querying Win32 programs is being acquired. This enhances granularity in logging, making it easier to distinguish between the acquisition of locks for Win32 and UWP programs. Improves traceability and debugging of the program's execution flow.
Removed the `CancellationToken` parameter from `WaitAsync` calls on semaphores in `EverythingAPI.cs` and `Main.cs`. This change eliminates cancellation support for semaphore waits, likely due to a design decision prioritizing simplicity or avoiding issues with cancellation handling. In `EverythingAPI.cs`, `WaitAsync(token)` was replaced with `WaitAsync()` in two methods. Similarly, in `Main.cs`, the `WaitAsync` calls for `_win32sLock` and `_uwpsLock` were updated to remove the `token` parameter. Note: This change may impact the ability to gracefully handle cancellation during semaphore waits.
Jack251970
changed the title
Enhanced cancellation handling by adding `token.IsCancellationRequested` checks to improve responsiveness. Refactored locking mechanisms for `_win32sLock` and `_uwpsLock` using `try-finally` blocks to ensure proper acquisition and release, improving thread safety and preventing deadlocks. Reorganized Win32 and UWP program querying logic for better modularity and readability. Replaced shared collection access with local variables to improve clarity and maintain thread safety. Simplified empty result handling by directly returning `emptyResults` when canceled. Removed redundant debug log statements to reduce verbosity and updated remaining logs for clarity. Suppressed unused result warnings by replacing direct calls to `EverythingApiDllImport.Everything_GetMajorVersion()` with null-coalescing assignments.
Added `ResetCache` calls after clearing `_win32s` and `_uwps` lists to ensure proper cache reset during indexing. Updated logic to return `resultList` after setting cache size and expiration for improved clarity. Removed `await Task.WhenAll` to adjust asynchronous flow in the indexing process.
Simplified the `if` statement that checks for token cancellation by condensing it into a single line. This improves code readability and eliminates unnecessary line breaks.
Added comments in `EverythingAPI.cs` and `Main.cs` to explain why `CancellationToken` is not directly passed to semaphore locks, preventing unexpected `OperationCanceledException`. Updated debug log messages in `Main.cs` for better clarity, including changing "Start handling programs" to "Start querying programs". Removed redundant log messages to improve logging consistency.
Jack251970
changed the title
3 tasks
Jack251970
added
bug
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors cancellation token handling in semaphore operations to prevent OperationCanceledException from being thrown when operations are cancelled. The changes ensure that cancellation is checked manually after acquiring locks rather than passing the cancellation token directly to WaitAsync().
Key changes:
- Introduced thread-safe access to
_settings.LastIndexTimeusing a newLockobject - Modified semaphore acquisition to not pass cancellation tokens directly, checking cancellation manually inside critical sections instead
- Created local copies of program lists to reduce lock holding time during query operations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Plugins/Flow.Launcher.Plugin.Program/Main.cs | Added _lastIndexTimeLock for thread-safe timestamp access; refactored QueryAsync to copy program lists quickly and release locks before processing; added thread safety to LastIndexTime updates in indexing methods |
| Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs | Modified IsEverythingRunningAsync and SearchAsync to handle cancellation manually instead of passing token to WaitAsync() |
Comments suppressed due to low confidence (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs:88
- The
SearchAsyncmethod now handles cancellation manually after acquiring the semaphore, but there's no check immediately afterWaitAsync()before entering the try block. If cancellation occurs while waiting for the semaphore, the token won't be checked until inside the try block. Consider adding a cancellation check immediately after acquiring the semaphore and before the try block to release the semaphore promptly if cancellation was requested during the wait.
await _semaphore.WaitAsync();
try
{
if (token.IsCancellationRequested)
yield break;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
Outdated
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
Show resolved
Hide resolved
Updated comments to explain the rationale for not directly passing CancellationToken to methods and instead checking IsCancellationRequested within locks. This prevents unexpected OperationCanceledException. Changes made in EverythingAPI.cs (IsEverythingRunningAsync) and Main.cs (Win32 and UWP program preparation). No functional changes to the code.
|
🥷 Code experts: VictoriousRaptor Jack251970, VictoriousRaptor have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds cancellation-aware semaphore/lock handling across EverythingAPI, Program Main, and PluginsManifest: WaitAsync calls now catch OperationCanceledException and exit early; Program/Main splits combined waits, copies shared collections to locals, and adds a dedicated lock to guard LastIndexTime updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant EverythingAPI
participant Semaphore as "Semaphore"
Note over EverythingAPI,Semaphore: Search/IsEverythingRunning (cancellation-aware)
Caller->>EverythingAPI: SearchAsync(token)
EverythingAPI->>Semaphore: WaitAsync() [try/catch OperationCanceledException]
alt Wait succeeded
EverythingAPI->>EverythingAPI: check token.IsCancellationRequested
opt not cancelled
EverythingAPI->>EverythingAPI: call Everything_GetMajorVersion / perform search
EverythingAPI-->>Caller: return results
end
else Wait cancelled
EverythingAPI-->>Caller: exit early / return
end
sequenceDiagram
autonumber
participant Caller
participant ProgramMain as "Program/Main"
participant Win32Lock as "_win32sLock"
participant UwpLock as "_uwpsLock"
participant LastIndexLock as "_lastIndexTimeLock"
Note over ProgramMain: QueryAsync locking refactor (separate, cancellable waits)
Caller->>ProgramMain: QueryAsync(token)
ProgramMain->>Win32Lock: WaitAsync() [try/catch OperationCanceledException]
alt Acquired
ProgramMain->>ProgramMain: copy `_win32s` -> local `win32s`
ProgramMain->>Win32Lock: Release
else Canceled
ProgramMain-->>Caller: return emptyResults
end
ProgramMain->>UwpLock: WaitAsync() [try/catch OperationCanceledException]
alt Acquired
ProgramMain->>ProgramMain: copy `_uwps` -> local `uwps`
ProgramMain->>UwpLock: Release
else Canceled
ProgramMain-->>Caller: return emptyResults
end
ProgramMain->>LastIndexLock: Enter
ProgramMain->>ProgramMain: compute `needReindex` / update `LastIndexTime`
ProgramMain->>LastIndexLock: Release
ProgramMain->>ProgramMain: run parallel, cancellable query over local lists
ProgramMain-->>Caller: return results
sequenceDiagram
autonumber
participant Caller
participant PluginsManifest
participant Semaphore as "Semaphore"
Note over PluginsManifest: UpdateManifestAsync uses lockAcquired guard
Caller->>PluginsManifest: UpdateManifestAsync(token)
PluginsManifest->>Semaphore: WaitAsync()
PluginsManifest->>PluginsManifest: set lockAcquired = true
PluginsManifest->>PluginsManifest: perform update (handles HTTP/errors)
alt OperationCanceledException during wait
PluginsManifest-->>Caller: early exit / cancellation ignored for flow
end
PluginsManifest->>PluginsManifest: if lockAcquired then Release Semaphore
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
83-88: Same indefinite blocking issue in SearchAsync.The same pattern is applied here. While the cancellation check at line 87 provides an early exit after acquiring the lock, the
WaitAsynccall can still block indefinitely during cancellation if the semaphore is held by another operation.Consider the same timeout or polling approach suggested for
IsEverythingRunningAsync.
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
34-34: Inconsistent lock type with existing code.The new
Locktype (likely .NET 9'sLock) differs from the existingSemaphoreSlimused for_win32sLockand_uwpsLockat lines 29-30. While functionally appropriate for synchronizingLastIndexTimeaccess, this introduces two different synchronization primitives in the same class.Consider using
SemaphoreSlimfor consistency, or document whyLockis preferred here (e.g., performance characteristics for simple mutual exclusion).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(9 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3573
File: Plugins/Flow.Launcher.Plugin.Shell/Main.cs:330-339
Timestamp: 2025-09-06T05:32:51.575Z
Learning: In Flow.Launcher Shell plugin's StartProcess method, the maintainer Jack251970 prefers not to propagate launch failures from Context.API.StartProcess or throw exceptions when the API call returns false. Silent failure handling is intentional for shell commands in this plugin.
📚 Learning: 2025-10-16T10:48:30.573Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
🧬 Code graph analysis (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiDllImport.cs (1)
EverythingApiDllImport(8-161)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (3)
Flow.Launcher/PublicAPIInstance.cs (4)
LogDebug(275-276)List(250-250)List(252-253)List(532-532)Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (3)
List(475-509)UWPApp(199-238)UWPApp(387-395)Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs (8)
List(217-276)List(832-853)Win32(300-333)Win32(335-391)Win32(393-431)Win32(433-457)Win32(595-610)Win32(671-726)
⏰ 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). (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (4)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (4)
128-129: Good use of local variables for thread safety.Using the local
win32sanduwpscopies instead of the shared_win32sand_uwpsensures the query processing operates on a consistent snapshot without holding locks during the potentially lengthy LINQ operations.
304-309: Proper synchronization for LastIndexTime read.The lock correctly guards the read of
LastIndexTimeto determine reindexing needs, preventing race conditions with the updates in the indexing methods.
343-346: Proper synchronization for LastIndexTime updates.Both indexing methods correctly guard the
LastIndexTimeupdates with_lastIndexTimeLock, ensuring thread-safe writes that coordinate with the read at lines 305-308.Also applies to: 372-375
89-89: Helpful debug logging for troubleshooting.The added debug logs provide visibility into the query and indexing workflow, which will be valuable for diagnosing the Program plugin indexing issues mentioned in the PR objectives.
Also applies to: 104-104, 117-117, 332-332, 361-361, 399-399
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
Outdated
Show resolved
Hide resolved
Updated `_semaphore.WaitAsync` in `EverythingAPI.cs` to accept a `CancellationToken` and handle `OperationCanceledException` gracefully, returning `false` instead of propagating the exception. Refactored locking mechanisms in `Main.cs` to use `CancellationToken` for `_win32sLock` and `_uwpsLock`. Added `try-catch` blocks to handle `OperationCanceledException` and ensure proper lock release. Methods now return `emptyResults` when operations are canceled.
jjw24
requested changes
Nov 6, 2025
Member
jjw24
left a comment
jjw24
left a comment
There was a problem hiding this comment.
Can we also fix up the spelling 'idx' while we are at it, make it a better descriptive name
Added a `catch` block for `OperationCanceledException` in `PluginsManifest.cs` to ignore canceled operations. Updated `EverythingAPI.cs` to use cancellation tokens with `_semaphore.WaitAsync` and handle cancellations by exiting the method cleanly with `yield break`.
Member
Author
@jjw24 I rethought this problem and refactored my logic. I believe it can resolve the infinite reindexing bug since I remove the coupling of two locks in Program plugin: _win32Lock and _uwpsLock. Please review my changes. I do not know how I can fix 'idx' spelling and could you please do me a favor? |
Member
Author
|
I have tested that bug in my stimulation environment to reproduce this lock issue. And I can confirm catching OperationCanceledException can resolve that problem. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (2)
51-58: Past review requested timeout pattern instead of token-based cancellation.While the current implementation correctly handles cancellation by passing the token to
WaitAsync, the past review comments from jjw24 explicitly requested implementing a timeout-based approach to prevent indefinite blocking. Line 202 (IncrementRunCounterAsync) already demonstrates this pattern:await _semaphore.WaitAsync(TimeSpan.FromSeconds(1));Consider adopting the timeout pattern consistently:
- try - { - await _semaphore.WaitAsync(token); - } - catch (OperationCanceledException) - { - return false; - } + if (!await _semaphore.WaitAsync(TimeSpan.FromSeconds(1))) + return false; + + if (token.IsCancellationRequested) + { + _semaphore.Release(); + return false; + }
87-94: Apply timeout pattern for consistency with IncrementRunCounterAsync.Same concern as
IsEverythingRunningAsync: the timeout pattern at line 202 should be used here for consistency and to address the indefinite blocking concerns raised in past reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs(1 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(6 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3573
File: Plugins/Flow.Launcher.Plugin.Shell/Main.cs:330-339
Timestamp: 2025-09-06T05:32:51.575Z
Learning: In Flow.Launcher Shell plugin's StartProcess method, the maintainer Jack251970 prefers not to propagate launch failures from Context.API.StartProcess or throw exceptions when the API call returns false. Silent failure handling is intentional for shell commands in this plugin.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-08-13T06:12:43.382Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-09-04T11:52:29.096Z
Learnt from: jjw24
Repo: Flow-Launcher/Flow.Launcher PR: 3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.096Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs
📚 Learning: 2025-10-16T10:48:30.573Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-06-29T08:31:07.816Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/Helper/PluginInstallationHelper.cs:251-252
Timestamp: 2025-06-29T08:31:07.816Z
Learning: In Flow Launcher's PluginInstallationHelper, when the progress box encounters an exception during download (indicated by reportProgress becoming null), the retry logic intentionally downloads without a progress handler to avoid repeating the same progress reporting failure. This prevents cascading exceptions in the progress reporting mechanism.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs
📚 Learning: 2025-09-06T05:32:51.575Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3573
File: Plugins/Flow.Launcher.Plugin.Shell/Main.cs:330-339
Timestamp: 2025-09-06T05:32:51.575Z
Learning: In Flow.Launcher Shell plugin's StartProcess method, the maintainer Jack251970 prefers not to propagate launch failures from Context.API.StartProcess or throw exceptions when the API call returns false. Silent failure handling is intentional for shell commands in this plugin.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs
📚 Learning: 2025-10-16T09:29:19.653Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:519-523
Timestamp: 2025-10-16T09:29:19.653Z
Learning: In Flow Launcher's PluginManager.cs QueryDialogJumpForPluginAsync method, when a DialogJump plugin is still initializing, returning null is intentional behavior. This allows the query to fall through to the default Explorer plugin, which serves as a fallback handler. This is different from QueryForPluginAsync and QueryHomeForPluginAsync, which show "still initializing" messages because they don't have fallback mechanisms.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-09-13T06:04:26.977Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
🧬 Code graph analysis (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiDllImport.cs (1)
EverythingApiDllImport(8-161)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (3)
Plugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs (1)
List(10-17)Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (3)
List(475-509)UWPApp(199-238)UWPApp(387-395)Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs (8)
List(217-276)List(832-853)Win32(300-333)Win32(335-391)Win32(393-431)Win32(433-457)Win32(595-610)Win32(671-726)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (4)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs (1)
57-60: Correct cancellation handling for manifest updates.The addition of the explicit
OperationCanceledExceptioncatch block properly handles cancellation during semaphore acquisition. SinceWaitAsync(token)guarantees the semaphore is not acquired when it throwsOperationCanceledException, the catch block correctly returns without releasing, and the finally block ensures cleanup in all other paths.Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
62-62: Discard assignment is appropriate for version check side-effect.The change to discard assignment (
_ =) clearly indicates the call is for its side-effect (setting error state) rather than using the return value.Plugins/Flow.Launcher.Plugin.Program/Main.cs (2)
34-34: Proper synchronization for LastIndexTime access.The introduction of
_lastIndexTimeLockand its consistent use to guard allLastIndexTimereads and writes correctly prevents race conditions during concurrent indexing operations.Also applies to: 300-304, 338-341, 366-369
113-139: Local list copies enable safe parallel querying.Using local copies (
win32s,uwps) instead of shared fields during the parallel query operation eliminates the risk of concurrent modification exceptions and ensures consistent results throughout the query. The cancellation-aware parallel processing withWithCancellation(token)properly handles user cancellation.
Added `lockAcquired` flags in `PluginsManifest.cs` and `Main.cs` to ensure semaphore locks are only released if successfully acquired, preventing potential runtime errors. Updated `finally` blocks to conditionally release locks based on these flags. Removed redundant cancellation check in `EverythingAPI.cs` to simplify code, assuming cancellation is handled elsewhere. These changes enhance reliability and maintainability of the codebase.
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Flow.Launcher.Plugin.Program/Main.cs
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4072
File: Plugins/Flow.Launcher.Plugin.Program/Main.cs:100-111
Timestamp: 2025-11-06T12:57:11.574Z
Learning: In Flow Launcher's Program plugin Main.cs QueryAsync method, the semaphore pattern where WaitAsync(token) is inside a try-catch that only catches OperationCanceledException, followed by an explicit Release() call after the try-catch block, is safe and doesn't cause semaphore leaks. When WaitAsync throws OperationCanceledException, the semaphore is not acquired, so no release is needed. When WaitAsync succeeds, the Release() call after the try-catch executes normally.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
📚 Learning: 2025-11-06T12:57:11.574Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4072
File: Plugins/Flow.Launcher.Plugin.Program/Main.cs:100-111
Timestamp: 2025-11-06T12:57:11.574Z
Learning: In Flow Launcher's Program plugin Main.cs QueryAsync method, the semaphore pattern where WaitAsync(token) is inside a try-catch that only catches OperationCanceledException, followed by an explicit Release() call after the try-catch block, is safe and doesn't cause semaphore leaks. When WaitAsync throws OperationCanceledException, the semaphore is not acquired, so no release is needed. When WaitAsync succeeds, the Release() call after the try-catch executes normally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.csFlow.Launcher.Core/ExternalPlugins/PluginsManifest.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.csFlow.Launcher.Core/ExternalPlugins/PluginsManifest.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.csFlow.Launcher.Core/ExternalPlugins/PluginsManifest.cs
📚 Learning: 2025-09-04T11:52:29.096Z
Learnt from: jjw24
Repo: Flow-Launcher/Flow.Launcher PR: 3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.096Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs
📚 Learning: 2025-10-16T10:48:30.573Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs
📚 Learning: 2025-08-13T06:12:43.382Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs
🧬 Code graph analysis (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingApiDllImport.cs (1)
EverythingApiDllImport(8-161)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs (2)
Plugins/Flow.Launcher.Plugin.WindowsSettings/Log.cs (1)
Exception(15-18)Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs (1)
LogException(479-482)
⏰ 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). (3)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Agent
- GitHub Check: build
🔇 Additional comments (4)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs (3)
29-33: LGTM! Proper semaphore acquisition tracking.The
lockAcquiredflag correctly tracks whether the semaphore was successfully acquired. IfWaitAsyncthrowsOperationCanceledException, the flag remains false and the semaphore won't be released in the finally block, preventing aSemaphoreFullException.Based on learnings
59-62: LGTM! Proper cancellation handling.Explicitly catching and ignoring
OperationCanceledExceptionis appropriate. Cancellation is an expected control flow during shutdown or when operations are deliberately cancelled, and doesn't require error logging.
67-71: LGTM! Conditional release prevents semaphore errors.The conditional release in the finally block correctly ensures the semaphore is only released when it was successfully acquired. This prevents
SemaphoreFullExceptionfrom attempting to release a semaphore that was never acquired due to cancellation.Based on learnings
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
62-62: Good use of discard operator.Using
_ =makes it explicit that the return value is intentionally ignored and we're calling this method for its side effect (populating the error state checked on line 63).
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
Show resolved
Hide resolved
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (2)
51-69: Critical: Implement the requested timeout for semaphore wait.Per previous review feedback from @jjw24 stating "we need to implement this suggestion," a timeout is required here to prevent indefinite blocking. The current implementation can still block indefinitely if another thread holds the semaphore during a long operation, even when cancellation is requested.
The codebase already establishes this pattern at line 200 in
IncrementRunCounterAsync, which usesTimeSpan.FromSeconds(1).Apply this diff to implement the timeout pattern:
try { - await _semaphore.WaitAsync(token); + if (!await _semaphore.WaitAsync(TimeSpan.FromSeconds(1), token)) + { + return false; // Timeout - another operation in progress + } } catch (OperationCanceledException) { return false; }
87-94: Critical: Implement the requested timeout for semaphore wait.Per previous review feedback from @jjw24 stating "we need to implement this suggestion," a timeout is required here to prevent indefinite blocking during concurrent searches. This is particularly important for
SearchAsyncsince it can hold the semaphore for extended periods while iterating through results.The codebase already establishes this pattern at line 200 in
IncrementRunCounterAsync, which usesTimeSpan.FromSeconds(1).Apply this diff to implement the timeout pattern:
try { - await _semaphore.WaitAsync(token); + if (!await _semaphore.WaitAsync(TimeSpan.FromSeconds(1), token)) + { + yield break; // Timeout - another search in progress + } } catch (OperationCanceledException) { yield break; }
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
87-124: LGTM! Properly decoupled lock handling with correct cancellation-safe pattern.The refactoring successfully achieves the PR objective of removing coupling between
_win32sLockand_uwpsLock. Each lock is now acquired and released independently with proper tracking vialockAcquiredflags. Copying to local variables (win32s,uwps) minimizes lock hold time.The semaphore pattern correctly handles
OperationCanceledException- whenWaitAsyncthrows, the semaphore is not acquired, so no release is needed.Consider adding timeouts to
WaitAsynccalls (e.g.,TimeSpan.FromSeconds(1)) to prevent indefinite blocking if locks are held during long-running operations, similar to the pattern suggested in previous reviews for consistency withEverythingAPI.cs.Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs(2 hunks)Plugins/Flow.Launcher.Plugin.Program/Main.cs(6 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4072
File: Plugins/Flow.Launcher.Plugin.Program/Main.cs:100-111
Timestamp: 2025-11-06T12:57:11.574Z
Learning: In Flow Launcher's Program plugin Main.cs QueryAsync method, the semaphore pattern where WaitAsync(token) is inside a try-catch that only catches OperationCanceledException, followed by an explicit Release() call after the try-catch block, is safe and doesn't cause semaphore leaks. When WaitAsync throws OperationCanceledException, the semaphore is not acquired, so no release is needed. When WaitAsync succeeds, the Release() call after the try-catch executes normally.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-11-06T12:57:11.574Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 4072
File: Plugins/Flow.Launcher.Plugin.Program/Main.cs:100-111
Timestamp: 2025-11-06T12:57:11.574Z
Learning: In Flow Launcher's Program plugin Main.cs QueryAsync method, the semaphore pattern where WaitAsync(token) is inside a try-catch that only catches OperationCanceledException, followed by an explicit Release() call after the try-catch block, is safe and doesn't cause semaphore leaks. When WaitAsync throws OperationCanceledException, the semaphore is not acquired, so no release is needed. When WaitAsync succeeds, the Release() call after the try-catch executes normally.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-09-04T11:52:29.096Z
Learnt from: jjw24
Repo: Flow-Launcher/Flow.Launcher PR: 3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.096Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-10-16T10:48:30.573Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher/App.xaml.cs:252-275
Timestamp: 2025-10-16T10:48:30.573Z
Learning: In Flow Launcher's App.xaml.cs, the plugin initialization block (lines 252-275) that includes PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, _mainVM.QueryResults(), and API.SaveAppAllSettings() does not require additional Task.Run wrappers or Dispatcher.InvokeAsync calls according to maintainer Jack251970, as the threading model is already safe as designed.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-08-13T06:12:43.382Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csPlugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-10-16T09:29:19.653Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:519-523
Timestamp: 2025-10-16T09:29:19.653Z
Learning: In Flow Launcher's PluginManager.cs QueryDialogJumpForPluginAsync method, when a DialogJump plugin is still initializing, returning null is intentional behavior. This allows the query to fall through to the default Explorer plugin, which serves as a fallback handler. This is different from QueryForPluginAsync and QueryHomeForPluginAsync, which show "still initializing" messages because they don't have fallback mechanisms.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-09-13T06:04:26.977Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3593
File: Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml.cs:143-165
Timestamp: 2025-09-13T06:04:26.977Z
Learning: In Plugins/Flow.Launcher.Plugin.WebSearch/SettingsControl.xaml, the ListView_SizeChanged handler in the C# file assumes 5 GridViewColumns but the XAML may have more columns. The user Jack251970 indicated this mismatch doesn't need to be fixed when pointed out during PR #3593 review.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-09-06T05:32:51.575Z
Learnt from: Jack251970
Repo: Flow-Launcher/Flow.Launcher PR: 3573
File: Plugins/Flow.Launcher.Plugin.Shell/Main.cs:330-339
Timestamp: 2025-09-06T05:32:51.575Z
Learning: In Flow.Launcher Shell plugin's StartProcess method, the maintainer Jack251970 prefers not to propagate launch failures from Context.API.StartProcess or throw exceptions when the API call returns false. Silent failure handling is intentional for shell commands in this plugin.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
⏰ 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). (6)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (7)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs (1)
29-71: LGTM! Correct implementation of cancellation-safe semaphore pattern.The
lockAcquiredflag correctly tracks whether the semaphore was successfully acquired, ensuring the semaphore is only released when it was actually obtained. WhenWaitAsyncthrowsOperationCanceledException, the semaphore is not acquired, so no release is needed.Based on learnings
Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingAPI.cs (1)
62-62: Minor: Discard assignment clarifies intent.Assigning the return value to a discard (
_) makes it clear the result is intentionally unused.Plugins/Flow.Launcher.Plugin.Program/Main.cs (5)
34-34: Good addition: Dedicated lock for LastIndexTime.Adding
_lastIndexTimeLockproperly guards concurrent access toLastIndexTimeacross indexing and initialization paths, preventing race conditions.
136-137: Good practice: Using local copies from minimal critical sections.Using the local
win32sanduwpscopies instead of directly accessing shared fields prevents holding locks during expensive parallel filtering and scoring operations.
312-316: Proper synchronization for needReindex calculation.The lock guard ensures
LastIndexTimeis read atomically when determining whether reindexing is needed.
350-353: Proper synchronization for LastIndexTime update.The lock guard ensures
LastIndexTimeis updated atomically after Win32 indexing completes.
378-381: Proper synchronization for LastIndexTime update.The lock guard ensures
LastIndexTimeis updated atomically after UWP indexing completes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Catch exception for
OperationCanceledExceptionwhen callingWaitAsync(CancellationToken), so that allSemaphoreSlimwill be released correctly.Fix #3962