Fix JsonRpc plugin connection loss exception #3432
Conversation
|
🚨 Warning: Approaching Monthly Automation Limit Monthly PRs automated: 226/250 Your organization has used over 90% of its monthly quota for gitStream automations. Once the quota is reached, new pull requests for this month will not trigger gitStream automations and will be marked as “Skipped”. To avoid interruptions, consider optimizing your automation usage or upgrading your plan by contacting LinearB. |
Jack251970
changed the title
|
🥷 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:
Activity based on git-commit:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs:22
- [nitpick] Using 'new' to hide the inherited JsonRPC constant can be confusing. Consider adding documentation or revisiting the design to clarify the necessity of shadowing the base constant.
public new const string JsonRPC = "JsonRPC";
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
@cibere Could you please check if this PR can fix your plugin's issue? And also please check if your plugin is closed after FL exiting (Here I just ignore that exception and I am not sure if the plugin with this issue can be closed entirely). Thanks! |
📝 WalkthroughWalkthroughThis pull request updates three plugin files. In the main plugin file, the code now reorders using directives, redefines the JSON-RPC constant using the new modifier, and removes several settings fields and properties. In the base plugin file, the using directives are similarly reorganized. In the V2 plugin, exception handling is enhanced by simplifying one catch block, adding a specific catch for connection loss, and including a general exception catch that logs errors. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant PluginV2 as JsonRPCPluginV2
participant Logger as Context.API.LogException
Caller->>PluginV2: Invoke ReloadDataAsync / DisposeAsync
alt RemoteMethodNotFoundException thrown
PluginV2->>PluginV2: Handle silently (simplified catch)
else ConnectionLostException thrown
PluginV2->>PluginV2: Handle connection lost exception
else General Exception thrown
PluginV2->>Logger: Log exception with plugin name
end
PluginV2->>Caller: Return response
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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
🔭 Outside diff range comments (1)
Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs (1)
171-225: 💡 Verification agent🧩 Analysis chain
Verify the ExecuteAsync method's exception handling for connection loss.
Based on the PR objectives, this change aims to fix JSON-RPC connection loss exceptions, but I don't see explicit handling for connection-related exceptions in this method. Consider adding specific handling for network-related exceptions (like
IOExceptionorWebException).Also, the method creates a
registeredEventto handle cancellation but doesn't have specific handling for connection loss during theprocess.WaitForExitAsyncor theTask.WhenAlloperations.Run the following script to search for any changes to exception handling related to connection loss in other files:
🏁 Script executed:
#!/bin/bash # Search for JsonRPC v2 plugin files and relevant exception handling echo "Searching for JsonRPCPlugin v2 implementation files..." fd -e cs "JsonRPC.*V2" --type f echo "Searching for connection loss exception handling..." rg -A 3 -B 3 "catch\s*\(\s*(IOException|WebException|SocketException|ConnectionException)" echo "Searching for changes in Dispose methods in JsonRPC plugins..." rg -A 5 -B 2 "Dispose\s*\(\s*\)|IDisposable" $(fd -e cs "JsonRPC")Length of output: 2249
Action: Enhance Connection Loss Exception Handling in ExecuteAsync
After verifying the code and the search results:
- The current implementation in Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs (lines 171–225) only catches an
OperationCanceledExceptionduring the asynchronous operations.- Our search did not reveal any explicit handling for network-related exceptions (e.g.,
IOException,WebException,SocketException, orConnectionException) within this method.- Although cancellation is managed via the
registeredEvent, exceptions related to connection loss duringprocess.WaitForExitAsync(token)orTask.WhenAll(sourceCopyTask, errorCopyTask)are not specifically handled.Recommendations:
- Add explicit catch blocks for network-related exceptions (such as
IOExceptionorWebException) around the asynchronous process handling to ensure that connection loss is detected and managed appropriately.- Review error logging to differentiate between cancellation and genuine connection errors, ensuring the error messages are informative.
- Consider consistency with JsonRPCPluginV2 (as observed in the lookup) if similar exception handling is expected across implementations.
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs (1)
22-22: Consider adding a comment explaining the use of thenewmodifier.The addition of the
newmodifier to theJsonRPCconstant explicitly indicates that this constant is intended to hide the identically named constant from the base classJsonRPCPluginBase. While this is syntactically correct, adding a comment would improve code clarity for future maintainers.- public new const string JsonRPC = "JsonRPC"; + // Explicitly hiding the base class constant to avoid ambiguity + public new const string JsonRPC = "JsonRPC";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs(2 hunks)Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs(1 hunks)Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs (1)
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#3081
File: Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs:136-145
Timestamp: 2025-04-03T10:38:00.009Z
Learning: In Flow.Launcher JsonRPCV2 plugins, the `reload_data` method is optional for plugins to implement. When the method is not found (RemoteMethodNotFoundException), it should be silently caught without logging as this is an expected scenario.
🧬 Code Graph Analysis (1)
Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs (1)
Flow.Launcher/Msg.xaml.cs (1)
System(44-47)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (4)
Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs (2)
117-126: Good enhancement of the exception handling in ReloadDataAsync.The changes to the exception handling are well structured:
- The simplified catch for
RemoteMethodNotFoundExceptionfollows the proper pattern as this exception is expected when plugins don't implement the optionalreload_datamethod.- Adding specific handling for
ConnectionLostExceptiondirectly addresses the PR objective of fixing connection loss exceptions.- The general exception handler with proper logging will help diagnose other unexpected issues without crashing the application.
135-144: Good implementation of consistent exception handling in DisposeAsync.These changes mirror the improvements made to the ReloadDataAsync method:
- Silently handling
RemoteMethodNotFoundExceptionfor plugins that don't implement the close method- Adding specific handling for
ConnectionLostExceptionwhich prevents unhandled exceptions during plugin disposal- Adding appropriate error logging for unexpected exceptions
This consistent approach to exception handling across similar methods improves the robustness of the plugin system.
Flow.Launcher.Core/Plugin/JsonRPCPluginBase.cs (1)
1-8: Using directive reorganization looks good.The reordering of the using directive for
Flow.Launcher.Core.Resourceis a non-functional change that maintains code organization consistency. This matches similar changes in other files likeJsonRPCPlugin.cs.Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs (1)
1-9: Using directive reorganization looks good.The reordering of the using directive for
Flow.Launcher.Core.Resourceis a non-functional change that maintains code organization consistency. This matches similar changes in the JsonRPCPluginBase.cs file.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs:21
- Hiding the base class constant using the 'new' modifier can be confusing; consider removing the 'new' modifier or renaming this constant to avoid potential shadowing issues.
public new const string JsonRPC = "JsonRPC";
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs:123
- [nitpick] Although intentionally ignored, consider logging the ConnectionLostException at a debug level to aid troubleshooting if future issues arise.
catch (ConnectionLostException)
Flow.Launcher.Core/Plugin/JsonRPCPlugin.cs:21
- [nitpick] Using the 'new' modifier to hide the inherited constant may cause confusion. If overriding is not explicitly required, consider removing 'new' or renaming the constant for clarity.
public new const string JsonRPC = "JsonRPC";
This comment has been minimized.
This comment has been minimized.
@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 ...
|
It seems to do so.
The problem stems from the plugin closing itself voluntarily, so afaik, there is nothing for flow to cleanup |
3 participants
Fix JsonRpc plugin connection loss exception
ConnectionLostExceptionwhen JsonRPC v2 plugins executeclose&reload_contextmethod.close&reload_contextmethod.Fix #3173, #3299.