[v3] app quit on message #3990
[v3] app quit on message #3990
Conversation
WalkthroughThe changes introduce enhanced Windows application termination handling in the Changes
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
Documentation and Community
|
![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)
v3/pkg/application/application_windows.go (1)
208-214: Consider documenting the Windows message handling behavior.The implementation successfully adds application-level quit message handling, which is a significant architectural change in how the application responds to system events. Consider adding documentation that explains:
- The difference between window-level and application-level message handling
- The behavior when
WindowsOptions{DisableQuitOnLastWindowClosed: true}is set- The interaction with system tray applications
This will help maintainers understand the message handling hierarchy and system integration points.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/application/application_windows.go(1 hunks)
🔇 Additional comments (1)
v3/pkg/application/application_windows.go (1)
208-214: Verify message handling order and potential race conditions.The implementation correctly adds application-level quit message handling, but there are a few considerations:
- The order of message handling might be important. Consider whether
WM_ENDSESSIONshould be handled differently as it's a system shutdown event that requires immediate response.- There could be potential race conditions if
globalApplication.Quit()is called concurrently from multiple message handlers.Let's verify the message handling implementation in other parts of the codebase:
Consider adding these improvements:
- Handle
WM_ENDSESSIONseparately as it requires immediate response during system shutdown:if hwnd == m.mainThreadWindowHWND { - if msg == w32.WM_ENDSESSION || msg == w32.WM_DESTROY || msg == w32.WM_CLOSE { + if msg == w32.WM_ENDSESSION { + // Respond to system shutdown immediately + globalApplication.Quit() + return 0 + } + if msg == w32.WM_DESTROY || msg == w32.WM_CLOSE { globalApplication.Quit() }
- Add synchronization to prevent race conditions:
if hwnd == m.mainThreadWindowHWND { if msg == w32.WM_ENDSESSION || msg == w32.WM_DESTROY || msg == w32.WM_CLOSE { + // Use sync.Once to prevent multiple quit calls + globalApplication.QuitOnce.Do(func() { globalApplication.Quit() + }) }✅ Verification successful
The implementation is correct and thread-safe.
The codebase already implements proper synchronization and cleanup mechanisms:
- Quit() is designed to handle concurrent calls safely through sync primitives
- Shutdown tasks are executed synchronously via InvokeSync
- The message handling order is appropriate for all termination scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other WM_ENDSESSION handlers that might conflict rg "WM_ENDSESSION|WM_DESTROY|WM_CLOSE" --type go # Check for other uses of globalApplication.Quit() rg "globalApplication.Quit" --type goLength of output: 3075
Script:
#!/bin/bash # Check the implementation of globalApplication and Quit() ast-grep --pattern 'func ($app $_) Quit() { $$$ }' # Look for any sync primitives or thread safety mechanisms rg "sync\." --type go v3/pkg/application/application.goLength of output: 427
Script:
#!/bin/bash # Find Quit implementation with a more specific pattern ast-grep --pattern 'func ($app *Application) Quit() { $$$ }' # Look for shutdown-related code rg "func.*Quit|shutdown|cleanup" --type go v3/pkg/application/application.goLength of output: 467
In some cases it's necessary close the application by message.
w32.WM_CLOSE is implemented on Window level but not on Application level.
If systray exists and application.WindowsOptions{DisableQuitOnLastWindowClosed: true} systray can exists without any window. But there is no handle for w32.WM_CLOSE.
So if other app (in my case Inno setup on upgrade) wants to close the application by message, it's not possible.
Discord discussion about this issue
https://discord.com/channels/1042734330029547630/1147312415948668971/threads/1323034810188566539
Summary by CodeRabbit