[v3] fix: unhandled error in dialogs.go #4156
Conversation
Does lead to deadlocks otherwise
WalkthroughA change was made in the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🪧 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)
website/src/pages/changelog.mdx (1)
21-21: Avoid Repeating "Fixed" for Improved ReadabilityThe bullet point in line 21 starts its sentences with "Fixed," which can feel repetitive when read in succession. Consider rewording it to enhance clarity and style. For example:
- Fixed locking issue on Windows when multiselect dialog returns an error. Fixed in [PR](https://github.com/wailsapp/wails/pull/4156) by @johannes-luebke + Resolved a locking issue on Windows occurring when a multiselect dialog returns an error. See [PR](https://github.com/wailsapp/wails/pull/4156) by @johannes-luebke.🧰 Tools
🪛 LanguageTool
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...sapp/wails/pull/3545) by @leaanthony. - Fixed locking issue on Windows when multisele...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...en multiselect dialog returns an error. Fixed in [PR](https://github.com/wailsapp/wai...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/src/pages/changelog.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/src/pages/changelog.mdx
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...sapp/wails/pull/3545) by @leaanthony. - Fixed locking issue on Windows when multisele...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...en multiselect dialog returns an error. Fixed in [PR](https://github.com/wailsapp/wai...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/src/pages/changelog.mdx (1)
17-19: Changelog Entry Addition & Consistency Check
The newly added entry clearly documents the fix for the Windows locking issue when the multiselect dialog returns an error (referencing PR #4156 by @johannes-luebke). Please double-check that the wording complies with our changelog style (based on Keep a Changelog and Semantic Versioning) and that it is free of accidental word duplications as hinted by static analysis.🧰 Tools
🪛 LanguageTool
[duplication] ~17-~17: Possible typo: you repeated a word.
Context: ... vulnerabilities. ## [Unreleased] ### Fixed - Fixed locking issue on Windows when multisele...(ENGLISH_WORD_REPEAT_RULE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/src/pages/changelog.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/src/pages/changelog.mdx
[duplication] ~17-~17: Possible typo: you repeated a word.
Context: ... vulnerabilities. ## [Unreleased] ### Fixed - Fixed locking issue on Windows when multisele...
(ENGLISH_WORD_REPEAT_RULE)
johannes-luebke
changed the title
Description
The returned error from
InvokeSyncWithResultAndErrorwasn't handled before and theselectionschannel could benil. This leads to a deadlock, at least on Windows, if there is an error. For example if the user cancels the dialog it will deadlock.I haven't found an issue for this problem. I hope it's fine I jumped straight to PR, otherwise I'm happy to write this down as an issue first.
Type of change
Please select the option that is relevant.
How Has This Been Tested?
I've tested this in my project which has a very particular setup using htmx and it actually results in a full deadlock.
But I've also reproduced the issue with the v3/examples/dialogs-basic/main.go. Here it does lock the goroutine, but because of the way the menu calls work it's hard to notice. Placing a debug print just before the return shows that it is never reached in case the user cancels the dialog.
Test Configuration
Wails Doctor
System
┌─────────────────────────────────────────────────────────────────────────────────┐
| Name | Windows 10 Pro |
| Version | 2009 (Build: 26100) |
| ID | 24H2 |
| Branding | Windows 11 Pro |
| Platform | windows |
| Architecture | amd64 |
| Go WebView2Loader | true |
| WebView2 Version | 134.0.3124.72 |
| CPU | AMD Ryzen Threadripper PRO 5965WX 24-Cores |
| GPU 1 | ASPEED Graphics Family(WDDM) (ASPEED) - Driver: 9.0.10.110 |
| GPU 2 | NVIDIA GeForce RTX 4090 (NVIDIA) - Driver: 32.0.15.6636 |
| Memory | 128GB |
└─────────────────────────────────────────────────────────────────────────────────┘
Build Environment
┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
| Wails CLI | v3.0.0-alpha.9 |
| Go Version | go1.23.5 |
| -buildmode | exe |
| -compiler | gc |
| CGO_CFLAGS | |
| CGO_CPPFLAGS | |
| CGO_CXXFLAGS | |
| CGO_ENABLED | 1 |
| CGO_LDFLAGS | |
| DefaultGODEBUG | asynctimerchan=1,gotypesalias=0,httpservecontentkeepheaders=1,tls3des=1,tlskyber=0,x509keypairleaf=0,x509negativeserial=1 |
| GOAMD64 | v1 |
| GOARCH | amd64 |
| GOOS | windows |
└────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
Dependencies
┌───────────────────────────┐
| npm | 10.7.0 |
| NSIS | Not Installed |
└─ * - Optional Dependency ─┘
Diagnosis
SUCCESS Your system is ready for Wails development!
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit