Repository navigation
fix: flaky navigation timeouts caused by a lost wakeup in WaitForLoadState and WaitForURL - #641
Conversation
…so a state recorded in between is not lost Frame.WaitForLoadState read the frame's load states, then built its waiter (a timer goroutine and four listener registrations), and only then subscribed to "loadstate". Events arrive on the connection's single dispatch goroutine, which can run in that gap; a "loadstate" it delivered there was emitted to no listener and is never replayed, so the wait slept out its whole timeout with the state it wanted already recorded. WaitForURL had the same shape around "navigated": it checked the URL, then called ExpectNavigation, which subscribes several registrations later. Upstream's client/frame.ts waitForLoadState builds the waiter first, checks _loadStates, and calls waiter.dispose() when the state is already there; JavaScript's run-to-completion makes that check atomic with the subscription. This brings the Go port to the same shape: attach the listener, re-check the recorded state, and dispose of the waiter (new waiter.dispose(), after upstream's) rather than wait for an event that has already been delivered. onLoadState and onFrameNavigated both update their state before emitting, so a re-check after subscribing cannot miss an event that fired before it. Measured with a browser (six pages clicking between two pages whose stylesheet and script make `load` trail the commit by a few ms): page.WaitForURL after a click timed out 6 times in 1,800 with the page already at the wanted URL and `load` delivered 0.2-0.4 ms after the call; with this change 0 in 1,800 under heavy load. The two new unit tests aim the event at the gap with no browser and fail within a few rounds on the previous code at any GOMAXPROCS (they raise it to 2 if needed). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // raceRoundsNeedParallelism gives the emitting goroutine its own P: at | ||
| // GOMAXPROCS=1 the waiting goroutine does not yield until it blocks in Wait(), | ||
| // which is after it has subscribed, and the old code passes trivially. | ||
| func raceRoundsNeedParallelism(t *testing.T) { | ||
| t.Helper() | ||
| if runtime.GOMAXPROCS(0) >= 2 { | ||
| return | ||
| } | ||
| prev := runtime.GOMAXPROCS(2) | ||
| t.Cleanup(func() { runtime.GOMAXPROCS(prev) }) | ||
| } |
There was a problem hiding this comment.
Required, not decorative. I ran these tests against current main (adapted for the extra cb argument): WaitForLoadState lost the event on round 8, WaitForURL on round 4, both timeout:Timeout 2000.00ms exceeded. On this branch they pass in tens of milliseconds.
Using the page’s framedetached registration as the “about to subscribe” signal is a stable way to aim at the gap. The ListenerCount checks afterwards are what would catch a dispose leak.
| // If the page is already closed, fail immediately rather than waiting for | ||
| // the navigation timeout, matching upstream's rejectImmediately guard. The | ||
| // check comes after the subscription: a close dispatched in between would | ||
| // otherwise reach no listener and never be replayed. reject drops the | ||
| // duplicate when the listener saw it first. | ||
| if f.page.IsClosed() { | ||
| waiter.reject(f.page.closeErrorWithReason()) | ||
| } |
There was a problem hiding this comment.
Correct Go-specific divergence from upstream.
TS _setupNavigationWaiter checks isClosed() before rejectOnEvent. That is safe under run-to-completion. Here a close dispatched between an early check and RejectOnEvent is the same lost-wakeup class as loadstate. Checking after the subscriptions, and letting reject drop a duplicate, is the right order. Don’t move this back in front of RejectOnEvent to match frame.ts.
| func (w *waiter) reject(err error) { | ||
| w.fulfilled.Store(true) | ||
| if w.fulfilled.Swap(true) { | ||
| return | ||
| } | ||
| w.errChan <- err | ||
| } |
There was a problem hiding this comment.
Load-bearing for dispose. After a successful re-check, dispose marks the waiter fulfilled and stops the timer; a timeout or close that already passed its fulfilled check must not send a second value into the capacity-2 errChan (that would block the dispatch goroutine). TestWaiterHasNotDeadlockForErrChanCapBiggerThan1AndCallbackErr still covers the old deadlock case.
| // dispose releases the waiter: it marks it fulfilled so a handler still | ||
| // running on the dispatch goroutine drops its event, stops the timeout and | ||
| // removes every listener. Wait calls it once the wait has resolved; a caller | ||
| // whose condition was met after subscribing but before waiting calls it | ||
| // directly and must not Wait afterward. Mirrors upstream's Waiter.dispose(). | ||
| func (w *waiter) dispose() { | ||
| w.fulfilled.Store(true) | ||
| w.mu.Lock() | ||
| defer w.mu.Unlock() | ||
| if w.stopTimeout != nil { | ||
| w.stopTimeout() | ||
| } | ||
| for _, l := range w.listeners { | ||
| l.emitter.RemoveListener(l.event, l.handler) | ||
| } | ||
| w.listeners = nil | ||
| } |
There was a problem hiding this comment.
Needed. Without this, the re-check path would leak the page close/crash/framedetached listeners and leave a timer that later sends on errChan.
Leaving evChan open in WaitForEvent is also right: a handler that already passed the fulfilled check must not send on a closed channel.
| // expectNavigation is ExpectNavigation with one extra knob for WaitForURL. | ||
| // With acceptCurrentURL the frame's URL is compared against options.URL once | ||
| // the "navigated" listener is attached; a match means the commit was recorded | ||
| // before the subscription, so the navigation event is skipped, only the load | ||
| // state is awaited and no Response is returned (the event that carried the | ||
| // request is the one that was missed). ExpectNavigation itself must not do | ||
| // this: its contract is to wait for a new navigation, so a reload of the | ||
| // current URL has to be observed rather than short-circuited. Only meaningful | ||
| // with a nil cb; there is no action to run when the navigation has happened. |
There was a problem hiding this comment.
Good split. WaitForURL means “be at this URL and this load state,” so accepting a commit that was recorded before subscribe is correct (and dropping Response is fine; WaitForURL discards it).
ExpectNavigation must stay acceptCurrentURL=false. Short-circuiting on the current URL would break reload-to-same-URL, which has to observe a new navigation.
| // Re-check after subscribing: a "loadstate" dispatched between the check | ||
| // above and the subscription reached no listener and is never replayed. | ||
| if f.loadStates.ContainsOne(state) { | ||
| waiter.dispose() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This is the right Go shape, and it is not a literal port of client/frame.ts.
TS can check _loadStates once after creating the waiter: waitForEvent adds the listener synchronously, and run-to-completion means the event loop cannot deliver a loadstate between that check and addListener. Python (no await in the gap) and Java (runUntil is the only thing that calls processOneMessage) are safe for the same reason.
Go’s dispatch goroutine can run between the first ContainsOne and On("loadstate"). A loadstate there updates the set, emits to zero listeners, and is never replayed. Re-checking after subscribe is what actually closes the window; matching TS line-for-line (waiter first, check once) would still be racy here.
Please keep this re-check. A later roll that “aligns” this back to the TS order would reintroduce the flake.
|
Approve. Real Go-only lost wakeup; the patch is the right shape. CI is green. I reproduced the unit tests on
This is not a missed roll. Nits, none blocking:
Inline notes are on the re-check, |
|
Thanks for the review. Body corrected: Agreed on the other two notes and leaving the code as is: the post-subscribe re-check and the |
2 participants
Thank you for this awesome project! Here's a fix to a flaky navigation timeout caused by a subtle concurrency bug in the client which causes
WaitForURL,WaitForLoadStateandExpectNavigationto occasionally fail even when the page is already at the wanted URL and fully loaded. This failure became quite common in the CI pipelines of my projects but they have been running fine after taking the fix into use. I published a minimal reproduction here https://github.com/myrjola/playwright-go-lost-wakeup. I've verified the reproduction and fix on my Linux server and Macbook.I did not find a prior issue for this; happy to open one if you want it tracked separately. I'm also happy to put in the work to to improve this PR.
Click to open sequence diagrams for understanding the bug and fix.
sequenceDiagram participant W as Wait goroutine<br/>(waitForLoadStateImpl) participant F as frameImpl<br/>(loadStates, listeners) participant D as Dispatch goroutine<br/>(connection) W->>F: read loadStates F-->>W: "load" absent W->>F: setNavigationWaiter(): start timeout timer,<br/>register close / crash / framedetached listeners rect rgba(220, 38, 38, 0.15) Note over F,D: gap: nobody is subscribed to "loadstate" yet D->>F: onLoadState("load") Note over F: loadStates += "load" Note over F: emit "loadstate" to zero listeners,<br/>never replayed end W->>F: On("loadstate", handler) Note over W: nothing re-reads loadStates W->>W: block on errChan W-->>W: ErrTimeout after the full timeout,<br/>page is at the URL and fully loadedAnd the same interleaving after the fix:
sequenceDiagram participant W as Wait goroutine<br/>(waitForLoadStateImpl) participant F as frameImpl<br/>(loadStates, listeners) participant D as Dispatch goroutine<br/>(connection) W->>F: read loadStates F-->>W: "load" absent W->>W: setNavigationWaiter() D->>F: onLoadState("load"): loadStates += "load",<br/>emit to zero listeners W->>F: On("loadstate", handler) W->>F: re-check loadStates F-->>W: "load" present W->>W: waiter.dispose(): stop timer, remove listeners W-->>W: return nilClick to open AI generated report below with more details.
It shows up as a rare, load-correlated timeout whose failure screenshot shows the right page. One instance from the reproduction below, with `framenavigated` and `load` listeners attached before the click:Cause.
waitForLoadStateImplreadsf.loadStates, then builds its waiter (setNavigationWaiter: a timer goroutine and four listener registrations), and only then subscribes to"loadstate". Events arrive on the connection's single dispatch goroutine, which can run between that read and the subscription. Aloadstatedelivered in the gap is emitted to no listener and is never replayed;loadStatesis updated but nothing re-reads it, so the wait sleeps out its whole timeout.WaitForURLhas the same shape around"navigated": it checksf.URL(), then callsExpectNavigation, which subscribes several registrations later.Upstream.
client/frame.tswaitForLoadStatebuilds the waiter first, checks_loadStates, and callswaiter.dispose()when the state is already there; run-to-completion makes that check atomic with the subscription. The Go port checks first and builds the waiter afterwards, and the dispatch goroutine can interleave.Fix. Bring the port to upstream's shape:
waiter.dispose()(new, after upstream'sWaiter.dispose()): marks the waiter fulfilled so a racing handler drops its event, cancels the timeout goroutine, removes every listener.waitForLoadStateImpl: keep the cheap first check (the common already-loaded case builds no waiter), subscribe, re-checkloadStates, dispose on a hit.onLoadStateadds to the set before emitting, so a re-check after subscribing cannot miss an event that fired before it.WaitForURL: the not-yet-matching path goes through an internalexpectNavigation(cb, alreadyNavigated, ...)that re-checks the URL once the"navigated"listener is attached.ExpectNavigationpassesniland is unchanged. On a hit only the load state is awaited and noResponseis returned, whichWaitForURLdiscards anyway.Evidence. Reproduction with both runs' full output: https://github.com/myrjola/playwright-go-lost-wakeup. Six Chromium pages click between two pages whose stylesheet and script make
loadtrail the commit by a few ms; a timeout is LOST if the page is at the wanted URL andloadwas delivered well inside the budget, otherwise slow. Same box, same knobs,-race:Every loss has the same shape: the navigation committed a few ms before the call,
loadarrived 0.2 to 0.4 ms after it, and the wait ran its full 3 s.Tests.
frame_wait_race_test.go(new, no browser): aframeImplfrom the existingnewTimeoutSemanticsFixture; a goroutine firesonLoadState/onFrameNavigatedthe moment the wait's"framedetached"rejection appears on the page, which is the registration just before the wait subscribes to its own event. Both tests fail within a few rounds on v0.6201.1 and pass here; they raise GOMAXPROCS to 2 when they find 1, since at 1 the waiting goroutine never yields before it subscribes, and they check no listener is left on the frame or page.TestWaiterDisposeRemovesEveryListener(new).-race, and the fulltests/suite on Chromium under-race: pass.golangci-lintwith the repo config: 0 issues. gofumpt clean. Driver version untouched.Written with Claude (Fable 5.1) in Claude Code; the measurements are from real runs on my machine.