Add option to disable panic recovery in message processing #4136
Conversation
WalkthroughThe changes introduce a new boolean option to control panic recovery during message processing. Specifically, the dispatcher’s constructor now accepts an additional parameter ( Changes
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)
🪧 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 (2)
v2/internal/frontend/dispatcher/dispatcher.go (2)
35-49: Consider adding error logging for the non-recovery case.The implementation conditionally wraps the panic recovery logic based on the
disablePanicRecoveryflag, which is good. However, when panic recovery is disabled, the error logging at lines 45-47 will also be skipped since it's inside the defer block.Consider adding error logging outside the conditional block to ensure errors are still logged even when panic recovery is disabled. This would help with debugging and monitoring in both modes.
func (d *Dispatcher) ProcessMessage(message string, sender frontend.Frontend) (_ string, err error) { if !d.disablePanicRecovery { defer func() { if e := recover(); e != nil { if errPanic, ok := e.(error); ok { err = errPanic } else { err = fmt.Errorf("%v", e) } } if err != nil { d.log.Error("process message error: %s -> %s", message, err) } }() } else { + // Still log errors when panic recovery is disabled + defer func() { + if err != nil { + d.log.Error("process message error: %s -> %s", message, err) + } + }() }
35-49: Add documentation to explain the disablePanicRecovery behavior.The code would benefit from a comment explaining the purpose and behavior of the
disablePanicRecoveryflag. This would help future maintainers understand the intended behavior and the trade-offs involved in enabling/disabling panic recovery.func (d *Dispatcher) ProcessMessage(message string, sender frontend.Frontend) (_ string, err error) { + // If disablePanicRecovery is false (default), we recover from panics during message processing + // and convert them to errors. If true, panics will propagate up the call stack, which can be + // useful for debugging or custom panic handling at a higher level. if !d.disablePanicRecovery { defer func() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
v2/internal/app/app_dev.go(1 hunks)v2/internal/app/app_production.go(1 hunks)v2/internal/frontend/dispatcher/dispatcher.go(1 hunks)v2/pkg/options/options.go(1 hunks)website/docs/reference/options.mdx(1 hunks)
🔇 Additional comments (7)
v2/pkg/options/options.go (1)
101-103: New configuration option looks good!The new
DisablePanicRecoveryfield with its descriptive comment clearly indicates its purpose of controlling panic recovery in message processing. This is a useful option for developers who want to handle panics themselves.website/docs/reference/options.mdx (1)
490-496: Documentation is clear and well-structured.The documentation for the new
DisablePanicRecoveryoption is thorough, explaining both the default behavior (automatic recovery and logging) and what happens when enabled (no automatic recovery). This provides users with a clear understanding of how to use this feature.v2/internal/app/app_dev.go (1)
215-215: Correctly passing the new option to the dispatcher.The
DisablePanicRecoveryoption is properly passed to the dispatcher constructor, allowing the development environment to respect this configuration option.v2/internal/app/app_production.go (1)
85-85: Consistently implemented in production environment.The option is properly passed to the dispatcher in the production environment as well, ensuring consistent behavior across both development and production builds.
v2/internal/frontend/dispatcher/dispatcher.go (3)
13-21: Clean implementation of the disablePanicRecovery field.The addition of the
disablePanicRecoveryboolean field to the Dispatcher struct is clean and well-aligned with the PR objective to provide an option for disabling panic recovery during message processing.
23-33: Constructor properly accepts and initializes the new parameter.The updated
NewDispatcherfunction correctly accepts the new parameter and initializes it in the struct. This change is consistent with the API modification mentioned in the PR summary.
35-49:❓ Verification inconclusive
Verify panics propagate as expected when recovery is disabled.
When
disablePanicRecoveryis set to true, panics will no longer be caught in this function. Make sure that:
- The behavior is documented in the user-facing documentation
- Higher-level code is prepared to handle these panics if necessary
- The application has appropriate crash reporting for unexpected panics
This behavior change could impact application stability if not properly understood by users.
🏁 Script executed:
#!/bin/bash # Look for any existing panic handlers in higher-level code echo "Searching for panic handling in the codebase..." rg -i "recover\(\)" --type goLength of output: 215
Action Required: Confirm Documentation and Higher-Level Panic Handling
The current implementation in
v2/internal/frontend/dispatcher/dispatcher.gocorrectly bypasses the deferred recovery block whendisablePanicRecoveryis set to true, allowing panics to propagate. However, please ensure that:
- The user-facing documentation clearly explains that panics will not be recovered when this flag is enabled.
- All higher-level code is reviewed and adequately prepared to handle any propagated panics.
- Appropriate crash reporting mechanisms are in place to capture unexpected panics.
Our grep search confirmed that the sole use of
recover()in the codebase is within this file. Please verify that no additional panic handlers exist that might interfere with this behavior and that any changes are properly communicated in the documentation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/src/pages/changelog.mdx (1)
22-22: Improve clarity and consistency of the changelog entry.The current entry "- Added
DisablePanicRecoveryoption to allow handle panics manually ..." would be clearer if rephrased and its PR reference formatted consistently. Consider revising it as follows:- - Added `DisablePanicRecovery` option to allow handle panics manually [#https://github.com/wailsapp/wails/pull/4136](https://github.com/wailsapp/wails/pull/4136) by [@APshenkin](https://github.com/APshenkin) + - Added `DisablePanicRecovery` option to allow manual handling of panics [#4136](https://github.com/wailsapp/wails/pull/4136) by [@APshenkin](https://github.com/APshenkin)This change improves grammatical clarity and standardizes the PR reference format.
|
|
Hi @leaanthony Anything that I can do so that it will be merged? Kind regards, |
Description
Allow handle panics manually by providing DisablePanicRecovery option
Fixes #4135
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.If you checked Linux, please specify the distro and version.
Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
DisablePanicRecoveryoption for error handling.DisablePanicRecoveryfeature, including credit to the contributor.