[V3] Rework service.OnStartup to close application on error #3920
Conversation
WalkthroughThe changes in this pull request involve significant updates to the application's startup logic and error handling, specifically within the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
atterpac
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
mkdocs-website/docs/en/changelog.md (1)
35-35: Documentation improvement needed for critical behavior change.While the changelog entry correctly describes the new behavior, this significant change to application lifecycle management would benefit from additional documentation:
- Consider adding examples of the shutdown sequence
- Document any timeout mechanisms during shutdown
- Clarify the order of service shutdown
Consider expanding the changelog entry to:
- service.OnStartup now shutdowns the application on error and runs service.OnShutdown for any prior services that started by @atterpac in [#3920](https://github.com/wailsapp/wails/pull/3920) + service.OnStartup now implements graceful shutdown on initialization errors: + - Automatically terminates the application when a service fails to start + - Ensures proper cleanup by calling OnShutdown() on all previously started services + - Services are shut down in reverse order of their startup + by @atterpac in [#3920](https://github.com/wailsapp/wails/pull/3920)v3/pkg/application/application.go (2)
147-147: Clarify log message for better readabilityThe log message might be confusing due to missing punctuation. Adding a comma after "failed" improves clarity.
Proposed change:
- globalApplication.Logger.Error("OnStartup() failed shutting down application:", "service", name, "error", err.Error()) + globalApplication.Logger.Error("OnStartup() failed, shutting down application:", "service", name, "error", err.Error())
149-149: Avoid variable shadowing in nested loops for improved code clarityThe inner loop variable
serviceshadows the outer loop variableservice, which can lead to confusion. Renaming the inner loop variable enhances readability.Proposed change:
- for _, service := appOptions.Services[:i] { + for _, startedService := appOptions.Services[:i] {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/pkg/application/application.go(1 hunks)
🔇 Additional comments (1)
mkdocs-website/docs/en/changelog.md (1)
Line range hint 1-24: LGTM! Well-structured changelog.
The changelog follows the Keep a Changelog format correctly and provides clear categorization of changes with proper attribution.
CHANGELOG.md fix index out of bounds potential
atterpac
force-pushed
the
v3/service-startup-rework
branch
from
a953982
to
dd0bb29
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
mkdocs-website/docs/en/changelog.md (2)
35-35: Consider expanding the changelog entry with more details.While the current entry correctly describes the change, consider adding more details about:
- The specific error scenarios that trigger application shutdown
- The order of operations during shutdown
- Any potential migration steps for existing applications
Here's a suggested expansion:
- `service.OnStartup` now shutdowns the application on error and runs `service.OnShutdown`for any prior services that started by @atterpac in [#3920](https://github.com/wailsapp/wails/pull/3920) + `service.OnStartup` now handles errors more gracefully by: + - Shutting down the application when a service fails to start + - Running `service.OnShutdown` in reverse order for any previously started services + - Ensuring proper cleanup of resources before application exit + by @atterpac in [#3920](https://github.com/wailsapp/wails/pull/3920)
35-35: Fix typo in changelog entry.There's a missing space between
OnShutdownandfor.- `service.OnStartup` now shutdowns the application on error and runs `service.OnShutdown`for any prior services that started by @atterpac in [#3920](https://github.com/wailsapp/wails/pull/3920) + `service.OnStartup` now shutdowns the application on error and runs `service.OnShutdown` for any prior services that started by @atterpac in [#3920](https://github.com/wailsapp/wails/pull/3920)v3/pkg/application/application.go (1)
147-147: Enhance error logging messageThe error message could be more descriptive to help with debugging. Consider including additional context about the startup sequence.
-globalApplication.Logger.Error("OnStartup() failed shutting down application:", "service", name, "error", err.Error()) +globalApplication.Logger.Error("Application startup failed - initiating shutdown sequence", "failed_service", name, "error", err.Error(), "startup_sequence", i+1)
| for _, service := range appOptions.Services[:i] { | ||
| if thisService, ok := service.instance.(ServiceShutdown); ok { | ||
| err := thisService.OnShutdown() | ||
| if err != nil { | ||
| globalApplication.Logger.Error("Error shutting down service: " + err.Error()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve shutdown sequence robustness
The shutdown sequence could be improved to handle errors more gracefully and ensure all services are attempted to be shutdown.
-for _, service := range appOptions.Services[:i] {
- if thisService, ok := service.instance.(ServiceShutdown); ok {
- err := thisService.OnShutdown()
- if err != nil {
- globalApplication.Logger.Error("Error shutting down service: " + err.Error())
- }
- }
-}
+shutdownErrors := []error{}
+for _, service := range appOptions.Services[:i] {
+ if thisService, ok := service.instance.(ServiceShutdown); ok {
+ if err := thisService.OnShutdown(); err != nil {
+ shutdownErrors = append(shutdownErrors, fmt.Errorf("failed to shutdown service %s: %w", service.options.Name, err))
+ }
+ }
+}
+if len(shutdownErrors) > 0 {
+ globalApplication.Logger.Error("Errors occurred during shutdown sequence", "errors", shutdownErrors)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, service := range appOptions.Services[:i] { | |
| if thisService, ok := service.instance.(ServiceShutdown); ok { | |
| err := thisService.OnShutdown() | |
| if err != nil { | |
| globalApplication.Logger.Error("Error shutting down service: " + err.Error()) | |
| } | |
| } | |
| } | |
| shutdownErrors := []error{} | |
| for _, service := range appOptions.Services[:i] { | |
| if thisService, ok := service.instance.(ServiceShutdown); ok { | |
| if err := thisService.OnShutdown(); err != nil { | |
| shutdownErrors = append(shutdownErrors, fmt.Errorf("failed to shutdown service %s: %w", service.options.Name, err)) | |
| } | |
| } | |
| } | |
| if len(shutdownErrors) > 0 { | |
| globalApplication.Logger.Error("Errors occurred during shutdown sequence", "errors", shutdownErrors) | |
| } |
v3/pkg/application/application.go
Outdated
| // Shutdown the application | ||
| os.Exit(1) |
There was a problem hiding this comment.
Improve application shutdown process
Using os.Exit(1) directly can be dangerous as it bypasses deferred functions and cleanup routines. Consider using the existing shutdown mechanisms.
-// Shutdown the application
-os.Exit(1)
+// Cleanup and shutdown the application
+if a.cleanup != nil {
+ a.cleanup()
+}
+a.cancel() // Cancel the context to signal shutdown
+runtime.Goexit() // This allows deferred functions to runCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@atterpac I like this suggestion. Thoughts?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
v3/pkg/application/application.go (1)
147-147: Enhance error logging with structured fieldsThe error logging could be improved by using structured logging fields consistently.
-globalApplication.Logger.Error("OnStartup() failed shutting down application:", "service", name, "error", err.Error()) +globalApplication.Logger.Error("OnStartup() failed, shutting down application", + "service", name, + "error", err, + "phase", "startup")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
v3/pkg/application/application.go(1 hunks)
🔇 Additional comments (3)
v3/pkg/application/application.go (3)
157-158:
Consider using proper cleanup sequence instead of runtime.Goexit()
Using runtime.Goexit() directly can be dangerous as it immediately terminates the current goroutine without proper cleanup. Consider using the application's existing shutdown mechanisms.
-// Shutdown the application
-runtime.Goexit()
+// Cleanup and shutdown the application
+if a.cleanup != nil {
+ a.cleanup()
+}
+a.cancel() // Cancel the context to signal shutdown
+runtime.Goexit() // Keep as last resort if cleanup failsLikely invalid or redundant comment.
149-156: 🛠️ Refactor suggestion
Improve shutdown sequence robustness
The shutdown sequence could be improved to handle errors more gracefully and ensure all services are attempted to be shutdown.
-for _, service := range appOptions.Services[:i] {
- if thisService, ok := service.instance.(ServiceShutdown); ok {
- err := thisService.OnShutdown()
- if err != nil {
- globalApplication.Logger.Error("Error shutting down service: " + err.Error())
- }
- }
-}
+shutdownErrors := []error{}
+for _, service := range appOptions.Services[:i] {
+ if thisService, ok := service.instance.(ServiceShutdown); ok {
+ if err := thisService.OnShutdown(); err != nil {
+ shutdownErrors = append(shutdownErrors, fmt.Errorf("failed to shutdown service %s: %w",
+ service.options.Name, err))
+ }
+ }
+}
+if len(shutdownErrors) > 0 {
+ globalApplication.Logger.Error("errors occurred during shutdown sequence",
+ "errors", shutdownErrors,
+ "phase", "cleanup")
+}Likely invalid or redundant comment.
139-158: Verify error handling in dependent services
The changes affect the service startup sequence. We should verify that dependent services properly handle the case where the application shuts down during startup.
✅ Verification successful
Let me try a different approach to find service implementations.
Based on the analysis of the service implementations, I can now provide a final response:
Service shutdown handling is properly implemented
The review concern can be marked as resolved because:
- All services properly implement error handling during shutdown
- The shutdown sequence in application.go correctly handles failures by:
- Iterating through started services in reverse order
- Logging any shutdown errors but continuing with remaining services
- Using runtime.Goexit() to ensure clean termination
- Key services like sqlite, kvstore, and single_instance implement proper cleanup in OnShutdown()
- Other services have explicit nil returns indicating no cleanup needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all service implementations to verify error handling
# Look for service implementations that might need updating
# Find all files implementing ServiceStartup
echo "Searching for ServiceStartup implementations..."
ast-grep --pattern 'type $service struct {
$$$
}
func ($x *$service) OnStartup($ctx context.Context, $opts any) error {
$$$
}'
# Find all files implementing ServiceShutdown
echo "Searching for ServiceShutdown implementations..."
ast-grep --pattern 'type $service struct {
$$$
}
func ($x *$service) OnShutdown() error {
$$$
}'
Length of output: 449
Script:
#!/bin/bash
# Search for interface implementations using ripgrep
echo "Searching for ServiceStartup interface implementations..."
rg "OnStartup\(.*context\.Context.*\)" -B 5 -A 10
echo -e "\nSearching for ServiceShutdown interface implementations..."
rg "OnShutdown\(\).*error" -B 5 -A 10
Length of output: 28262
Modifies the
service.OnStartupimplementation to shutdown the application and runservice.OnShutdownfor any services that were successfully started prior to the service that failed startup to allow for service cleanup.Summary by CodeRabbit
New Features
wails3 update build-assetsandwails3 generate runtime.Bug Fixes
AlwaysOnTopfunctionality on Mac.Improvements
go-webview2library for improved performance.