fix(linux): fix race conditions and potential deadlocks in event handling (#4424) #4776
Conversation
…ling (#4424) This commit addresses issues that could cause menu buttons to stop responding when events are being emitted frequently: 1. Fixed race condition in mainthread callback store (Linux, macOS, iOS): - Changed RLock() to Lock() when deleting from mainThreadFunctionStore - Using RLock for read-only operations but then calling delete() violated the RWMutex contract and could cause undefined behavior 2. Fixed potential deadlock in EventIPCTransport.DispatchWailsEvent: - Changed to snapshot windows list and release lock before dispatching - Previously held windowsLock.RLock() during ExecJS which blocks waiting for the main thread, potentially causing deadlocks if main thread operations needed windowsLock.Lock() 3. Added test case to reproduce the issue 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
WalkthroughThis PR addresses two concurrency issues in Wails v3: race conditions in main thread callback dispatch on Linux, macOS, and iOS (fixed by replacing read locks with exclusive locks for safe map deletion), and a potential deadlock in event dispatch (fixed by decoupling lock acquisition from the blocking dispatch operation). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v3/pkg/application/transport_event_ipc.go (1)
18-23: Consider resilience if a window closes between snapshot and dispatch.If
t.app.windowscan contain nils or a window can become invalid concurrently, consider a defensive nil-check (or ensureWindow.DispatchWailsEventis safe post-close).
🧹 Nitpick comments (2)
v3/test/4424-event-block/main.go (2)
47-65: Stop the emitter goroutine on app shutdown (avoid leaking / emitting after quit).As a repro, “run forever” is acceptable, but it’s easy to make this cleaner by cancelling on a shutdown/before-quit lifecycle event and using a
time.Tickerinstead ofSleep. (Time sleeping in a goroutine is fine and shouldn’t block UI, per prior learnings.)
58-61: Prefermap[string]anyin Go 1.18+ code.Minor readability/idiomatic improvement.
- app.Event.Emit("backendmessage", map[string]interface{}{ + app.Event.Emit("backendmessage", map[string]any{ "counter": counter, "time": time.Now().Format(time.RFC3339), })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
v3/UNRELEASED_CHANGELOG.md(1 hunks)v3/pkg/application/mainthread_darwin.go(1 hunks)v3/pkg/application/mainthread_ios.go(1 hunks)v3/pkg/application/mainthread_linux.go(1 hunks)v3/pkg/application/transport_event_ipc.go(1 hunks)v3/test/4424-event-block/Taskfile.yml(1 hunks)v3/test/4424-event-block/main.go(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Sammy-T
Repo: wailsapp/wails PR: 4570
File: v2/internal/frontend/desktop/linux/window_webkit6.go:97-108
Timestamp: 2025-10-17T23:16:11.570Z
Learning: For webkit_6/GTK4 builds in v2/internal/frontend/desktop/linux/window_webkit6.go, GTK widget creation should not be wrapped in invokeOnMainThread. The activation mechanism (activateWg + onActivate export) already handles thread safety, and additional wrapping would cause issues.
📚 Learning: 2025-10-17T23:16:11.570Z
Learnt from: Sammy-T
Repo: wailsapp/wails PR: 4570
File: v2/internal/frontend/desktop/linux/window_webkit6.go:97-108
Timestamp: 2025-10-17T23:16:11.570Z
Learning: For webkit_6/GTK4 builds in v2/internal/frontend/desktop/linux/window_webkit6.go, GTK widget creation should not be wrapped in invokeOnMainThread. The activation mechanism (activateWg + onActivate export) already handles thread safety, and additional wrapping would cause issues.
Applied to files:
v3/pkg/application/mainthread_darwin.go
📚 Learning: 2025-04-29T23:54:07.488Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4256
File: v2/internal/frontend/desktop/linux/notifications.go:27-28
Timestamp: 2025-04-29T23:54:07.488Z
Learning: In Wails v2, unlike v3-alpha which has a `ServiceShutdown` method for services, there is no standardized teardown pattern for frontend implementations. When implementing features that require cleanup (like goroutines or resources), add explicit cleanup methods (e.g., `CleanupNotifications()`) that handle resource release, context cancellation, and connection closure.
Applied to files:
v3/pkg/application/transport_event_ipc.go
📚 Learning: 2024-09-15T21:32:51.758Z
Learnt from: leaanthony
Repo: wailsapp/wails PR: 3748
File: v3/internal/templates/_common/Taskfile.tmpl.yml:18-18
Timestamp: 2024-09-15T21:32:51.758Z
Learning: In Taskfile, variables are referenced using `{{ "{{variable}}" }}` without the dot notation to differentiate them from Go template variables.
Applied to files:
v3/test/4424-event-block/Taskfile.yml
📚 Learning: 2024-10-08T22:11:37.054Z
Learnt from: leaanthony
Repo: wailsapp/wails PR: 3763
File: v3/examples/window/main.go:472-475
Timestamp: 2024-10-08T22:11:37.054Z
Learning: In `v3/examples/window/main.go`, `time.Sleep` is used within a goroutine and does not block the UI thread.
Applied to files:
v3/test/4424-event-block/main.gov3/pkg/application/mainthread_linux.go
📚 Learning: 2024-10-08T22:11:37.054Z
Learnt from: leaanthony
Repo: wailsapp/wails PR: 3763
File: v3/internal/commands/appimage_testfiles/main.go:295-299
Timestamp: 2024-10-08T22:11:37.054Z
Learning: In `v3/internal/commands/appimage_testfiles/main.go`, `time.Sleep` is used within a goroutine and does not block the UI thread.
Applied to files:
v3/test/4424-event-block/main.gov3/pkg/application/mainthread_linux.go
📚 Learning: 2025-01-24T22:41:18.566Z
Learnt from: leaanthony
Repo: wailsapp/wails PR: 4031
File: v3/pkg/application/menu.go:199-202
Timestamp: 2025-01-24T22:41:18.566Z
Learning: In the Wails menu system (v3/pkg/application/menu.go), shared state between menus is intentionally designed and desirable. Methods like `Append()` and `Prepend()` should maintain shared references to menu items rather than creating deep copies.
Applied to files:
v3/test/4424-event-block/main.go
🧬 Code graph analysis (2)
v3/pkg/application/transport_event_ipc.go (2)
v3/internal/runtime/desktop/@wailsio/runtime/src/index.ts (1)
Window(43-43)v3/pkg/application/window.go (1)
Window(9-104)
v3/test/4424-event-block/main.go (5)
v3/pkg/application/application.go (1)
AlphaAssets(30-32)v3/pkg/application/application_options.go (3)
MacOptions(193-199)ActivationPolicy(181-181)ActivationPolicyAccessory(188-188)v3/pkg/application/systemtray.go (1)
SystemTray(44-66)v3/pkg/icons/icons.go (1)
WailsLogoBlack(24-24)v3/pkg/application/events.go (1)
ApplicationEvent(14-18)
🪛 GitHub Check: SonarCloud Code Analysis
v3/test/4424-event-block/main.go
[warning] 4-4: Add a comment explaining why this blank import is needed.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
v3/test/4424-event-block/Taskfile.yml (1)
1-6: Reproducer Taskfile is minimal and clear.
task run→go run .is exactly what you want for a manual repro harness.v3/UNRELEASED_CHANGELOG.md (1)
29-30: Changelog entries accurately capture the concurrency fixes.
Wording is clear and ties back to #4424.v3/pkg/application/mainthread_ios.go (1)
34-46: Correct fix: map delete now happens under exclusive lock; lock isn’t held during callback.
The explicit unlock-before-Fatalpath also avoids leaking the lock ifFatalexits the process.v3/pkg/application/mainthread_darwin.go (1)
35-46: Correct fix: enforce exclusive locking for map mutation and release before executing callback.
Matches the intended RWMutex semantics and reduces deadlock/race risk in the callback store.v3/pkg/application/mainthread_linux.go (1)
9-19: Correct fix: stop mutating the callback map under a read lock.
Deleting underLock()and unlocking beforefn()is the right concurrency shape here.v3/pkg/application/transport_event_ipc.go (1)
8-17: Good lock scoping change; note the intentional snapshot semantics.Unlocking before dispatch avoids deadlocks caused by blocking dispatch paths (e.g.,
ExecJSwaiting on main thread). Only caveat: windows added during dispatch won’t see this event; windows removed after unlock may still be invoked.
| import ( | ||
| _ "embed" | ||
| "log" | ||
| "time" | ||
|
|
||
| "github.com/wailsapp/wails/v3/pkg/application" | ||
| "github.com/wailsapp/wails/v3/pkg/events" | ||
| "github.com/wailsapp/wails/v3/pkg/icons" | ||
| ) |
There was a problem hiding this comment.
Remove the unused blank embed import (or add a comment + actual //go:embed).
Right now there’s no embed usage, so _ "embed" is just noise and triggers static analysis warnings.
import (
- _ "embed"
"log"
"time"📝 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.
| import ( | |
| _ "embed" | |
| "log" | |
| "time" | |
| "github.com/wailsapp/wails/v3/pkg/application" | |
| "github.com/wailsapp/wails/v3/pkg/events" | |
| "github.com/wailsapp/wails/v3/pkg/icons" | |
| ) | |
| import ( | |
| "log" | |
| "time" | |
| "github.com/wailsapp/wails/v3/pkg/application" | |
| "github.com/wailsapp/wails/v3/pkg/events" | |
| "github.com/wailsapp/wails/v3/pkg/icons" | |
| ) |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 4-4: Add a comment explaining why this blank import is needed.
🤖 Prompt for AI Agents
In v3/test/4424-event-block/main.go around lines 3 to 11, the blank import _
"embed" is unused and should be removed (or if you intend to embed assets, add
an actual //go:embed directive and a variable to use it). Edit the import block
to either delete the _ "embed" entry or replace it with a real embed usage plus
a comment indicating what is embedded so static analysis no longer flags the
unused import.
Deploying wails with
|
| Latest commit: |
d4cb67a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ba5bb919.wails.pages.dev |
| Branch Preview URL: | https://fix-event-emitter-blocks-men.wails.pages.dev |
Summary
This PR addresses issues that could cause menu buttons to stop responding when events are being emitted frequently on Linux (and potentially macOS/iOS).
Changes:
Fixed race condition in mainthread callback store (Linux, macOS, iOS):
RLock()toLock()when deleting frommainThreadFunctionStoreRLockfor what should be a read-only operation, but then calleddelete()which modifies the map - this violates the RWMutex contractFixed potential deadlock in
EventIPCTransport.DispatchWailsEvent:windowsLock.RLock()duringExecJSwhich blocks waiting for the main threadwindowsLock.Lock(), this could cause a deadlockAdded test case (
v3/test/4424-event-block/) to reproduce the issueTest plan
v3/test/4424-event-block/Fixes #4424
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.