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)
|
it's still async init so impact should be minimal. thoughts @taooceros ? If looks good then approve + merge please. |
well it should still be async, so plugins with network requirement can still fire those request concurrently (I am not sure), but not parallel. I think the assumption about plugin initialization is pretty short can be true? I didn't see much CPU intense plugin except Program plugin. The clipboard issue is because it requires the thread being a STA rather than a MTA (which will be what On the other hand, it will block the main thread, which means this will disallow us to show up flow's window before plugin initialization. Now it's fine as that's the current behavior, but I am not sure whether in the future we will want to do that? |
I do not think we need to show up FL window before plugin initialization. The plugins are uninitialized and we cannot get anything from query, and even may encounter issue when executing query methods. And Devs can handle CPU intense task in their own plugin and this will not break anything. I am not sure the clipboard issue is related to sta because I just want to register clipboard monitor event and it does not need sta as I remember. |
May I propose one workaround? You can use windows api to access the built-in clipboard histroy (requires windows 10 1809+ iirc, and rerquires users to turn on the feature in windows settings), rather than creating your own monitor. It might need lots of changes though. |
I have tried this. But this also fails to work.... |
|
I believe using |
yes because its thread pool |
I learnt it. Are we happy to merge? |
Maybe we don't need to actually monitor clipboard, but only access it when user queries? I made a concept plugin long ago and it worked. |
|
@VictoriousRaptor Get it. But your solution fully relies on Windows Clipboard History (10.0.17763+) and every time you query, plugin will get results from Windows Clipboard History. But my plugin is designed for win7+ and I do not want to rely on Windows Clipboard History feature. By the way, my plugin also supports the solution you described above (Sync with Windows Clipboard History). |
Could you try STA first? I am more leaning to let the plugin handle STA themselves. If not then we can think about this. If we provide this guarantee, then in the future we would have a hard time to revert it back as some plugin may depend on this behavior. |
I have tested, but the clipboard monitor created in the STA thread cannot work as well.
|
|
I really have no ideas about 'some plugin may depend on this behavior'? I do not think there are any functions which cannot work on main thread, but it can work with thread pool. |
No you can't create a monitor in STA thread and return that to the main thread. You should hold the monitor consistently in the STA thread (which will be used to handle the message from the operating system), and uses some message passing or whatever to pass the event to the place where you want to handle them. |
Sounds like very complex. Could we add new interface called ISTAPlugin which uses the above solution? |
|
Additionally, if I create this monitor in |
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.
Here is my plugin for test:
Clipboard+-2.1.1 - Test InitAsync.zip