Fixes #5016 - Fix deadlock in EventIPCTransport.DispatchWailsEvent holding RLock during InvokeSync#5107
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughDispatch now snapshots Changes
Sequence Diagram(s)sequenceDiagram
participant App as App
participant Lock as windowsLock
participant Snapshot as windows (slice)
participant Window as Window
App->>Lock: RLock()
Lock-->>App: copy app.windows -> Snapshot
App->>Lock: RUnlock()
loop for each window in Snapshot
App->>Window: DispatchWailsEvent(event)
Window-->>App: InvokeSync / response (may block)
alt event.IsCancelled()
App->>App: break loop
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
v3/pkg/application/transport_event_ipc.go (1)
22-27: Please add a regression test for lock inversion deadlock.This path is concurrency-sensitive; a targeted test that dispatches while windows are added/removed would help prevent regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/pkg/application/transport_event_ipc.go` around lines 22 - 27, Add a concurrent regression test that reproduces the lock-inversion scenario by repeatedly calling DispatchWailsEvent while other goroutines concurrently add/remove entries from the windows collection; specifically create a test (e.g., TestDispatchWhileModifyingWindows) that spins up one goroutine to loop window.DispatchWailsEvent(event) and another to concurrently add and remove windows, coordinate start/stop with sync.WaitGroup or channels, enforce a timeout/context deadline to fail the test on deadlock, and assert progress (e.g., event delivered or loop iterations completed) to detect hangs; target the code paths around windows, DispatchWailsEvent and event.IsCancelled to ensure the concurrency-sensitive path is covered under the race detector.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v3/pkg/application/transport_event_ipc.go`:
- Around line 22-27: Add a concurrent regression test that reproduces the
lock-inversion scenario by repeatedly calling DispatchWailsEvent while other
goroutines concurrently add/remove entries from the windows collection;
specifically create a test (e.g., TestDispatchWhileModifyingWindows) that spins
up one goroutine to loop window.DispatchWailsEvent(event) and another to
concurrently add and remove windows, coordinate start/stop with sync.WaitGroup
or channels, enforce a timeout/context deadline to fail the test on deadlock,
and assert progress (e.g., event delivered or loop iterations completed) to
detect hangs; target the code paths around windows, DispatchWailsEvent and
event.IsCancelled to ensure the concurrency-sensitive path is covered under the
race detector.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e8d59ec-2082-44c3-99d0-7767a809b2cd
📒 Files selected for processing (1)
v3/pkg/application/transport_event_ipc.go
|
Thanks for opening this. This introduces a small and unlikely race condition if the window gets deleted after the copy but before the dispatch, the application will crash. that was likely the intent of having a lock there in the first place. There is other code in Window that checks if it is being destroyed. It would be good to follow that pattern here to prevent crashes |
wayneforrest
force-pushed
the
fix-dead-lock-ipc-dispatch
branch
from
87bbd19 to
506d558
Compare
Morning @leaanthony -- I have added one more commit: 506d558 It guards against dispatching events when the window has been destroyed. |
|
Thanks for doing this 🙏 |
2 participants
Description
Potential deadlock in EventIPCTransport.DispatchWailsEvent lock ordering issue.
See details in #5106
Fixes #5016
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.If you checked Linux, please specify the distro and version.
Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Bug Fixes
Refactor