fix: reload associated Requests on Session.Close before their WebSocket subscribes#220
Merged
Merged
Conversation
…et subscribes Session.Close promised that Requests associated with the Session would ask the browser to reload, but it delivered the reload only via a key-targeted broadcast. The serve loop distributes broadcasts solely to Requests already present in its subscription map, so a Request still in its pending window (page rendered, WebSocket not yet subscribed) never received the reload and kept running stale UI after logout or session teardown. deadSession now queues one Reload frame directly onto the Request's wsQueue while holding rq.mu, so process delivers it as soon as the WebSocket connects, independent of subscription timing. Session.Close broadcasts a key-targeted what.Update purely as a wake-up for an already-running process loop; since handleBroadcast resolves a key destination to no elements, the Update queues no browser command, so exactly one reload is delivered on every path. To preserve the previous fail-fast behavior of the reload broadcast, a key-targeted Update is treated as critical in mustBroadcast: an overloaded subscription is cancelled rather than silently dropped, while an ordinary (nil or tag destination) Update remains droppable. Fixes #215
…droppable Address review feedback on the overload classification and tests. Restrict droppability to the internal periodic dirty-render tick: only a nil-destination Update is coalescible and safe to drop. Every addressed message, including a tag-targeted Update (one-shot, no periodic re-send) and the key-targeted Update wake-up from Session.Close, now fails-fast an overloaded Request instead of being silently lost. This is a plain nil-destination check, so the lib/key import is no longer needed, and it fixes the pre-existing latent loss of tag-targeted Updates under backlog. Comments in mustBroadcast and runWebSocket are corrected to match. Tests: - Add TestSessionCloseReloadsConnectedRequestExactlyOnce: reads through a post-close marker and counts every Reload, so a delayed duplicate on the connected path is caught (the single-frame read in TestSession_Delete could miss one). - TestServeKeyTargetedUpdateFailFast now proves the droppable subscription survives (drains the tick, then a targeted message must still arrive) and adds a tag-targeted case, covering all three destination classes. - Assert the recycled request's queue stays empty in TestSession_ProducersSkipRecycled, and count every Reload operation rather than only the exact empty-data encoding in the pending-reload test. Verified by mutation: all-Updates-droppable, killSub-without-cancel on the droppable path, and a duplicate queued reload each fail the new assertions.
…rloaded ErrRequestOverloaded said overflowing broadcasts are never dropped, but the internal periodic dirty-render tick (a nil-destination Update) is intentionally coalescible and dropped on overflow. Document that one exception.
The nil-destination Update tick is not dropped because a later tick re-sends the work; distributeDirt has already moved the dirty tags into each Request's todoDirt and cleared the global set, so the tick is only a wake-up. It is safe to drop on a full channel because the already-buffered message wakes Request.process, whose next pass drains todoDirt before waiting again. Correct the comments on ErrRequestOverloaded and mustBroadcast.
A dropped nil-destination Update tick is safe in both phases: a Request already in its process loop is woken by the message that filled the channel and drains todoDirt on the next pass, while one still starting up (subscribed before onConnect) drains todoDirt on its first pass without needing a wake. Also fix the mustBroadcast comment referring to the updateTicker case as 'above' when it is below.
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.
Summary
Fixes #215.
Session.Closedocuments that Requests already associated with the Session "will ask the browser to reload the pages", but it delivered the reload only through a key-targetedJaws.Broadcast. The serve loop'smustBroadcastfans out only to Requests present in its subscription map at that instant, so a Request in its pending window — page rendered byNewRequest, but its/jaws/<key>WebSocket has not yet reachedsubscribe— never received the reload.deadSessionalso clearedrq.session, so the Request kept no record of the close and never reloaded when it finally connected. The page kept running stale authenticated UI after logout / credential rotation.This is distinct from #161, which covered broadcasts issued from
ConnectFn(after the subscription exists); #215 is about a reload sent before the WebSocket request begins at all.Approach
deadSessionnow queues exactly oneReloadframe directly onto the Request'swsQueue, under therq.muit already holds.processdrainswsQueuebefore its firstselect, so the reload is delivered as soon as the WebSocket connects — independent of subscription timing. Holdingrq.muexcludes a concurrent recycle and the process loop'sgetSendMsgs, so the append is safe (rq.mu→muQueueorder).Session.Closebroadcasts a key-targetedwhat.Updatepurely as a wake-up: it makes an already-runningprocessloop iterate and flush the queued reload.handleBroadcastresolves a key destination to no elements, so the Update itself performs no browser operation — exactly one reload is delivered on every path (no duplicatewindow.location.reload()).mustBroadcastoverload classification: only the internal periodic dirty-render tick (a nil-destination Update) is droppable.distributeDirthas already moved the dirty tags into each Request'stodoDirtand cleared the global set, so the tick carries no payload — it only nudges the Request. The dirt is still rendered without it in both phases: a Request already in itsprocessloop is woken by the message that filled the channel and drainstodoDirton the next pass, and one still starting up (subscribed beforeonConnect) drainstodoDirton its first pass without needing a wake. Every addressed message now fails-fast an overloaded Request withErrRequestOverloaded, including a tag-targeted Update (one-shot; this also fixes a pre-existing latent loss of tag-targeted Updates under backlog) and the key-targeted Update wake-up. The check is a plainmsg.Dest != nil, solib/keyis no longer imported byserve.go.Note:
Session.Broadcastaddresses by key, soSession.Broadcast(Update)is fail-fast rather than droppable — consistent with the rule that only the internal nil-destination tick is coalescible.Docs updated to match:
Session.Close,deadSession,mustBroadcast, therunWebSocketbuffer comment, andErrRequestOverloaded(which now notes the one droppable exception).Tests
TestSessionCloseReloadsAssociatedPendingRequest— deterministic regression test for the pending path: a drained control subscription proves (via broadcast ordering) that Close's wake was processed while the target had no subscription, then the target dials and exactly oneReloadis asserted by reading through a post-connect marker and counting every Reload operation. Confirmed to fail on the pre-fix code.TestSessionCloseReloadsConnectedRequestExactlyOnce— connected path: closes the session while the Request is connected, then reads through a post-close marker and asserts exactly one Reload, so a delayed duplicate would be caught (a single-frame read could miss one).TestServeKeyTargetedUpdateFailFast— covers all three destination classes: a nil-destination Update overload leaves the Request alive and proves the subscription survived (drains the tick, then a targeted message must still arrive); a tag-targeted Update overload and a key-targeted Update overload each fail-fast withErrRequestOverloaded.TestSession_ProducersSkipRecycled— updated to expect theUpdatewake, assert exactly one queuedReloadon the live Request, and assert the recycled request's queue stays empty.TestSession_DeleteandTestSessionCloseDoesNotReachLaterRequest— preserved.Mutation-checked: all-Updates-droppable, killSub-without-cancel on the droppable path, and a duplicate queued reload each fail the new assertions.
Verification
go test -race ./...,go test ./...(release renderer),go vet ./...,gofmt -l ., andstaticcheck ./...all pass.deadSession,Session.Close, andServeWithTimeoutreport 100% coverage; the targeted tests pass repeatedly with no flakiness.