Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[V3] Rework service.OnStartup to close application on error #3920

Merged
merged 3 commits into from
Nov 30, 2024
Merged

[V3] Rework service.OnStartup to close application on error #3920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd0bb29
rework service.OnStartup to close applicatoin on error
atterpac Nov 27, 2024
6fc8b67
os.Exit -> runtime.Goexit
atterpac Nov 27, 2024
81c0d82
Revert to os.Exit
leaanthony Nov 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions mkdocs-website/docs/en/changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
in [#3888](https://github.com/wailsapp/wails/pull/3888)

### Changed
- `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)
- Refactored systray click messaging to better align with user interactions by @atterpac in [#3907](https://github.com/wailsapp/wails/pull/3907)
- Asset embed to include `all:frontend/dist` to support frameworks that generate subfolders by @atterpac in [#3887](https://github.com/wailsapp/wails/pull/3887)
- Taskfile refactor by [leaanthony](https://github.com/leaanthony) in [#3748](https://github.com/wailsapp/wails/pull/3748)
Expand Down
16 changes: 13 additions & 3 deletions v3/pkg/application/application.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,16 +136,26 @@ func New(appOptions Options) *App {
result.handleFatalError(fmt.Errorf("Fatal error in application initialisation: " + err.Error()))
}

for _, service := range appOptions.Services {
for i, service := range appOptions.Services {
if thisService, ok := service.instance.(ServiceStartup); ok {
err := thisService.OnStartup(result.ctx, service.options)
if err != nil {
name := service.options.Name
if name == "" {
name = getServiceName(service.instance)
}
globalApplication.Logger.Error("OnStartup() failed:", "service", name, "error", err.Error())
continue
globalApplication.Logger.Error("OnStartup() failed shutting down application:", "service", name, "error", err.Error())
// Run shutdown on all services that have already started
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())
}
}
}
leaanthony marked this conversation as resolved.
Show resolved Hide resolved
// Shutdown the application
os.Exit(1)
Comment on lines +157 to +158
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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 run

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atterpac I like this suggestion. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}
}
}
Expand Down