fix(v3): deliver events in emit order via a per-window queue - #5934
Merged
Conversation
App.dispatchOnMainThread runs work inline when the caller is already on the
UI thread. So an event emitted from the UI thread executed its eval
immediately while an event emitted moments earlier from a goroutine was
still sitting in the dispatch queue, and the later event arrived first.
DispatchWailsEvent now appends to a per-window queue instead of calling
ExecJS directly, and a single drain on the UI thread empties it in order.
Appending under a mutex makes the queue the one ordering authority: the
order events are added is the order they are delivered, whichever thread
each came from.
The queue is bounded, and the bound applies to goroutine emitters only. A
UI-thread emitter is never made to wait, because it is the drainer -
blocking it could not be relieved by anything and would be a guaranteed
deadlock rather than a slow path. In practice it never queues at all: the
drain scheduled via InvokeAsync runs inline when already on the UI thread,
so a main-thread emit enqueues and immediately drains.
Capacity picked by measurement rather than taste. Ordering held and
5000 ev/s was sustained at every size tried, so only tail latency moved,
growing with depth as events wait behind a longer backlog:
capacity 5000 ev/s mixed-source p99
16 5000/s, 0 inv 179us
64 4991/s, 0 inv 257us
256 4995/s, 0 inv 329us
1024 5000/s, 0 inv 427us
64 keeps it shallow while leaving room to absorb a burst.
Emitting also got cheaper. It no longer blocks on a main-thread round trip,
just an append, so emit p50 at 5000 ev/s falls from ~45us to 1-2us.
Verified on macOS 26.4.1, Ubuntu 26.04 (WebKitGTK) and Windows 11
(WebView2): 0 inversions across mixed-source, interleaved-size and
5000 ev/s runs on all three.
Four unit tests cover it, and they need no GUI so they run in CI. The
headline one drives DispatchWailsEvent itself and fails without the queue,
delivering "second" before "first".
Also corrects the harness scenario that first reported this. It took the
sequence number outside the InvokeAsync callback and dispatched later, so
sequence order differed from call order by construction and the reorders it
counted were the harness racing itself. The number it produced (~4.4%) was
not a valid measure of framework ordering and should not be relied on. With
the sequence number taken at the point of dispatch, and emit order
serialised so it is defined at all, the unfixed code does not merely
reorder - it deadlocks, because a goroutine holds the emit lock while
parked in InvokeSync waiting for the UI thread.
Contributor
Walkthrough
ChangesEvent ordering and dispatch
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Producer as Event producer
participant Window as WebviewWindow
participant UI as UI thread
participant Webview
Producer->>Window: enqueueEventJS
Window->>UI: schedule one queue drain
UI->>Window: drain events in order
Window->>Webview: ExecJS for each event
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes cross-thread event reordering in Wails v3 by ensuring WebviewWindow.DispatchWailsEvent delivers events in the same order they are emitted, even when some emits originate on the UI thread (where dispatchOnMainThread runs inline) and others originate from goroutines (where work is queued).
Changes:
- Route event delivery through a per-window FIFO queue and drain it on the UI thread to preserve emit order and avoid the known deadlock shape of locking across the main-thread hop.
- Update the event-performance harness to take sequence numbers at the moment of dispatch (and serialize seq+dispatch for mixed-source scenarios).
- Add unit tests that deterministically reproduce the inversion and validate ordering/backpressure/close semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
v3/pkg/application/webview_window.go |
Implements per-window event queueing/draining and closes the queue on window destroy. |
v3/pkg/application/event_ordering_test.go |
Adds regression + behavioral tests for ordered delivery and queue semantics without requiring a GUI. |
v3/tests/event-performance/scenarios.go |
Adds a mutex to serialize seq assignment with dispatch in mixed-source scenarios (harness correctness). |
v3/tests/event-performance/main.go |
Moves seq assignment into the actual dispatch callback and uses the new harness mutex to prevent self-racing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v3/pkg/application/event_ordering_test.go`:
- Around line 284-290: Make the blocked producer state deterministic in the test
around win.enqueueEventJS by adding a test-only synchronization signal
immediately before the producer enters eventQueueCond.Wait, then wait for that
signal before calling win.closeEventQueue. Preserve the existing
blocked-producer and completion assertions so the test verifies Broadcast
releases the waiting producer.
In `@v3/pkg/application/webview_window.go`:
- Around line 1441-1450: The event payload path in the dispatcher around
enqueueEventJS and eventPayloads.put can leak when queue closure causes enqueue
rejection. Ensure payload insertion and queue acceptance are atomic, or remove
the specific payload immediately whenever enqueueEventJS rejects it, including
the corresponding logic in the alternate path around the referenced range.
In `@v3/tests/event-performance/main.go`:
- Around line 395-403: Move the emitMu acquisition from the UI callback passed
to application.InvokeAsync into the ticker goroutine before scheduling the
callback, and hold it until the callback finishes win.DispatchWailsEvent. Update
both emitter paths, including the corresponding logic around the second
InvokeAsync occurrence, so the UI thread never waits on emitMu while preserving
sequence generation and unlocking on completion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b393f961-db71-41f1-9c11-452b9c397e3c
📒 Files selected for processing (4)
v3/pkg/application/event_ordering_test.gov3/pkg/application/webview_window.gov3/tests/event-performance/main.gov3/tests/event-performance/scenarios.go
A parked payload could be stranded (CodeRabbit, major). A dispatcher can pass the destroyed check, markAsDestroyed can then close the queue and run dropWindow, and only after that does put succeed - so dropWindow cannot see the entry and the queue refuses the event that would have fetched it, leaving it for the TTL sweep. enqueueEventJS now reports whether it accepted, and a rejected by-reference event reclaims its payload. A regression test covers it and strands 8217 bytes without the fix. The harness's own emit lock could deadlock (CodeRabbit, major), which is the exact hazard this PR is about. A goroutine emitter held emitMu while DispatchWailsEvent waited for queue capacity, and the UI callback then blocked on emitMu - so the drainer could not drain. The lock is now taken on the ticker goroutine and released by the UI callback, so only a goroutine ever waits for it. The goroutine emitter releases via defer, so a panic in dispatch cannot wedge the scenario either. The blocked-producer test was racy (CodeRabbit): close could win before the producer reached Wait, so it could pass without exercising the Broadcast it exists to test. It now waits for the queue to be full and the producer to still be in flight before closing. Also from Copilot: the retracted ~4.4% figure is gone from the code comment now that it is known to be a harness artifact, EventQueueHighWater is unexported since only package tests use it, and a "resliceing" typo.
leaanthony
pushed a commit
that referenced
this pull request
Aug 10, 2026
…ts in emit order via a per-window queue
27 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
App.dispatchOnMainThreadruns work inline when the caller is already on the UI thread. So an event emitted from the UI thread executed its eval immediately, while an event emitted moments earlier from a goroutine was still sitting in the dispatch queue — and the later event arrived first.DispatchWailsEventnow appends to a per-window queue instead of callingExecJSdirectly, and a single drain on the UI thread empties it in order. Appending under a mutex makes the queue the one ordering authority: the order events are added is the order they are delivered, whichever thread each came from.Follow-up to #5930, which found this but deliberately left it alone. Independent of it — that one was about payload size, this is about threading.
Prior art, and how this relates
Event ordering has been reported before and this PR is a third layer, not a competing fix:
Emit→ window dispatch (a goroutine per dispatch)ExecJS(the UI-thread inline fast path)ExecJS→ JS execution on GTK3@DevLumuz raised the ordering problem first in #5757 (6 July), before #5850 was filed, and is credited as co-author on #5851 for the first layer.
#5757 also argued a third cause: GTK3's
webkit_web_view_evaluate_javascriptwith a NULL callback confirms only that WebKit accepted the script, so back-to-back calls might execute out of order. Measured on stock master built with-tags gtk3: 186,047 events at up to 5000/s across three scenarios, 0 reorders and 0 drops. WebKit's per-page IPC is FIFO, so asynchronous does not imply unordered here. On that evidence the synchronous nested-main-loop change in #5757 is not needed, and it would carry real costs — re-entering the UI thread while an eval is pending, and a full JS round trip per event.Why not simply lock around the dispatch
The obvious fix — a per-window mutex around choosing and enqueuing — deadlocks. It would have to be held across the
ExecJSmain-thread hop, so if the UI thread then emits, it blocks on the mutex and the holder'sInvokeSynccan never complete. That is the same shape as the deadlock documented attransport_event_ipc.go:9-14.The queue avoids it because the bound applies to goroutine emitters only. A UI-thread emitter is never made to wait — it is the drainer, so blocking it could not be relieved by anything. In practice it never queues at all: the drain scheduled via
InvokeAsyncruns inline when already on the UI thread, so a main-thread emit enqueues and immediately drains.What happens when the queue is full
Nothing is dropped, and nothing deadlocks. The bound behaves differently depending on which thread is emitting, which is the whole reason it is safe:
DispatchWailsEventblocks until the drain frees a slot.So the bound is a throttle on goroutine emitters, not a discard policy — nothing is lost.
One honest caveat about what that bound does not achieve. Since #5851,
Emitfeeds an unbounded mailbox whose drain goroutine is what callsDispatchWailsEvent. So when this queue fills it blocks the mailbox drainer, not the application:Emitstill returns immediately and the excess accumulates one layer up. This bound therefore limits the per-window queue, not total in-flight memory, and does not surface backpressure to app code. Ordering — the point of this PR — is unaffected either way, but the bound should not be read as system-wide backpressure.The two cases are covered by
EventQueueDoesNotBlockMainThreadWhenFullandEventQueueCloseReleasesBlockedProducer.Sizing
Picked by measurement. Ordering held and 5000 ev/s was sustained at every size, so the only thing that moved was tail latency, which grows with depth as events wait behind a longer backlog:
64 keeps it shallow while leaving room to absorb a burst. Most apps will never reach it. Raise it only with a measurement showing a producer being throttled.
Emitting also got cheaper
Emit no longer blocks on a main-thread round trip — it is just an append. At 5000 ev/s, emit p50 falls from ~45 µs to 1–2 µs.
A correction to #5930
The scenario that first reported this took the sequence number outside the
InvokeAsynccallback and dispatched later, so sequence order differed from call order by construction. The reorders it counted were the harness racing itself, and the ~4.4% figure quoted in #5930 is not a valid measure of framework ordering. It's fixed here.With the sequence number taken at the point of dispatch, and emit order serialised so that it is defined at all, the unfixed code does not merely reorder — it deadlocks, because a goroutine holds the emit lock while parked in
InvokeSyncwaiting for the UI thread. The same run completes in ~35 s with the queue. So the old design cannot express ordered cross-thread emission at all, which is a stronger statement than the retracted percentage.The deterministic proof that the inversion is real is the unit test, which drives
DispatchWailsEventand fails without the queue by delivering"second"before"first".Type of change
How Has This Been Tested?
Four unit tests, no GUI needed so they run in CI:
EventFromMainThreadDoesNotOvertakeQueuedEventEventQueuePreservesSingleProducerOrderEventQueueDoesNotBlockMainThreadWhenFullEventQueueCloseReleasesBlockedProducerPlus
v3/tests/event-performanceon all three platforms —mixedsource,interleaveandrate-5000, 30 s each:rate-5000also 0 inversions on all three, confirming no throughput regression.Linux: Ubuntu 26.04 LTS x86_64, headless under Xvfb, both backends —
default GTK4 (
libgtk-4/libwebkitgtk-6.0) and legacy-tags gtk3(
libgtk-3/libwebkit2gtk-4.1), confirmed by the linked libraries rather than the build tag alone.mixedsourceis 0 inversions on both; unit tests pass under both.Windows: Windows 11 Pro 26200 x86_64, WebView2 151.0.4129.72, interactive console session.
Test Configuration
Checklist:
website/src/pages/changelog.mdxwith details of this PR (v3 changelog entries are added automatically)Summary by CodeRabbit