Fix Possible Plugin Initialization Issue #3303
Conversation
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
|
🥷 Code experts: no user but you matched threshold 10 Jack251970 has 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: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThis pull request modifies the plugin initialization process by switching from a parallel, task-based approach to a sequential loop approach. The Changes
Sequence Diagram(s)sequenceDiagram
participant PM as PluginManager
participant P as Plugin Instance
loop Initialize Each Plugin Sequentially
PM->>P: InitAsync(context)
alt Successful Initialization
P-->>PM: Completed Task
else Error Occurred
P-->>PM: Exception Thrown
PM->>PM: Mark plugin disabled & enqueue failure
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher.Plugin/Interfaces/IPlugin.cs (1)
33-38: Appropriate implementation for sync-in-async pattern to address clipboard monitoring issues.This change correctly implements the sync-in-async pattern by directly calling the synchronous
Initmethod and then awaitingTask.CompletedTaskrather than wrapping the call inTask.Run. This addresses the issue where plugins like Clipboard+ were unable to properly monitor clipboard events when initialized in a separate thread.While this addresses the specific issue with plugins that interact with system events, consider adding documentation to clarify which types of plugins should not be initialized in tasks (e.g., those that hook into system events, UI, etc.) to guide future plugin developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Core/Plugin/PluginManager.cs(2 hunks)Flow.Launcher.Plugin/Interfaces/IPlugin.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (3)
Flow.Launcher.Core/Plugin/PluginManager.cs (3)
168-168: Clear explanatory comment about sequential initialization decision.The comment clearly explains why
Task.WhenAllcan't be used for plugin initialization, which is helpful for future maintainers understanding this design decision.
180-185: Robust error handling for plugin initialization failures.The error handling approach is well-implemented: it catches exceptions from individual plugin initializations, logs the errors, disables the problematic plugins, and enqueues them for later reporting to the user.
This is an improvement over the previous implementation as it allows the application to continue loading even if some plugins fail to initialize, providing a more resilient user experience.
168-186:❓ Verification inconclusive
Sequential plugin initialization addresses system event hooking issues.
The change from parallel to sequential plugin initialization resolves the issue with plugins that need to hook into system events, such as the Clipboard+ plugin. Instead of using
Task.WhenAllfor parallel initialization, each plugin is now initialized sequentially with proper error handling.While the PR mentions performance is slightly better with the sequential approach based on testing with 15 plugins, let's verify the impact on startup performance across a larger range of plugins:
🏁 Script executed:
#!/bin/bash # Look for timing information related to plugin initialization in the codebase echo "Checking for plugin initialization timing logs:" rg "Total init cost" -A 2 -B 2 echo "" echo "Checking for any performance benchmarks or tests:" rg "milliseconds|performance test|benchmark" --type csLength of output: 2925
Plugin Initialization: Sequential Approach Approved with Caution on Large-Scale Performance
The sequential initialization effectively resolves system event hooking issues (e.g., for the Clipboard+ plugin) as seen in
Flow.Launcher.Core/Plugin/PluginManager.cs. Detailed timing logs across this file,Stopwatch.cs, andPluginsLoader.csconfirm that plugin initialization metrics are correctly recorded. While tests with 15 plugins indicate a slight performance improvement with this approach, please verify the startup performance impact when scaling to a larger number of plugins to guard against any potential bottlenecks.
- Locations to review:
Flow.Launcher.Core/Plugin/PluginManager.cs(Initialization logic and logging)Stopwatch.cs(Timing metrics)Flow.Launcher.Core/Plugin/PluginsLoader.cs(Additional performance logging)
Issue
If using
Task.Runto initialize my plugin Clipboard+, my plugin will fail to monitor the clipboard. (I do not know why, but my plugin cannot listen to system clipboard event if we do so)So we should not use
Taskto run plugin init event.Test
Use
foreachwill affect performance but just a little bit. I use 15 plugins and test withStopwatch.DebugAsync.With
Task.WhenAllthe plugins are fully initialized in about 1350ms, while withforeachthe plugins are fully initialized in around 1250ms.Considering majority of plugins do not need much time to initialize, this solution can serve the purpose.
Clipboard+-2.1.1 - Test InitAsync.zip