refactor(gregor): connection identity, own ctx, and app-state gating - #29659
Open
chrisnojima wants to merge 5 commits into
Open
chrisnojima wants to merge 5 commits into
chrisnojima wants to merge 5 commits into
Conversation
The gregor handler had no way to tell one connection from the next. A late callback from a torn-down connection -- OnConnect finishing its tail, a Syncer.Connected landing after Disconnected, a reconnect fired by a connection that had already shut down -- would act on whatever connection happened to be current. Shutdown used the caller's ctx, so work started by a connection outlived it. Every connection now has an identity and a ctx of its own. OnConnect's post-sync steps run only while their connection is still the current one, and report "no longer current" back to the caller so onConnectSynced can return ErrDuplicateConnection instead of installing state for a dead connection. Shutdown cancels that connection's ctx, so its in-flight work stops with it rather than with whoever called Shutdown. Syncer.Connected bails when its ctx is already cancelled, so it cannot mark the syncer connected after the matching Disconnected. Connecting is gated on app state in one place, gregorConnGate: only BACKGROUND, or a desktop suspend, takes the connection down; INACTIVE keeps it up. A BACKGROUND that lands after a connect has read the state wakes the monitor, which waits for that connect to finish before taking the connection down, so a connect and a state change cannot cross. The service asks for ConnectFresh(uri) rather than doing its own IsConnected()/Reset() dance, and OnLogout disconnects instead of resetting. ServerConnection.Reconnect no longer returns anything: callers never waited for the result, and the value it returned described a connection that could already be gone. connTransport guards conn/transport/stagedTransport with a mutex -- the connection dials on its own goroutine while Shutdown closes the transport.
OnDisconnected and OnConnectError now go through a per-connection handler that ignores events from a connection already shut down. Reconnect requests carry the ctx of the connection they are for and are skipped once it is replaced. pingOnce no longer leaks its goroutine on shutdown, a queued replay is skipped once its user logs out, and the ping redial test bounds elapsed time instead of counting in a fixed window.
With no connection, as in BACKGROUND, a reset only dropped the gregor client until the next connect.
Only BACKGROUND stops it, matching gregor. INACTIVE is transient on iOS (control center, the app switcher, an incoming call).
OnConnect's gated steps go through connGate.runIfLive, which also hands over the uri. Drop the non-resetting Connect, which only tests used.
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
1 participant
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.
Split out of #29637 so the gregor connection rework can be reviewed on its own. Based on
master, independent of the app-state stack.Why
The handler tracked "which connection is the current one" out of band, in fields beside the connection rather than on it. Every asynchronous step afterwards — the
OnConnecttail, the ping loop, the push-state debouncer, installing the gregor client — then had to re-derive whether it was still relevant, and several did not. The fix is to give each connection its own ctx and gate the work on connection identity, so a superseded connection's work simply stops.Master bugs this fixes
OnConnectis not gated on connection identity, so a connect that lost the race still completes its tail.connTransporthas a data race —conn,transportandstagedTransportare read and written without synchronisation.Reconnectblocks its caller while dialling.Syncer.Connectedruns against a cancelled ctx instead of bailing.Connectionhas no shutdown flag, so any later call on a replaced connection starts a new reconnect loop, whoseOnDisconnectedmarks the syncer offline and clearsconnectedAtwhile the current connection is up.What this changes
Each connection carries its own ctx, cancelled when that connection shuts down.
OnConnectis gated on connection identity.connectTLS/connectNoTLSreturn the connection instead of assigning it and spawning goroutines, and a failed connect starts none. All connect decisions move into one reconciler: only BACKGROUND, or a desktop suspend, takes the connection down; INACTIVE keeps it up.Reconnectbecomes asynchronous, and the connect tail is serialised with shutdown on one mutex.Each connection gets its own
rpc.ConnectionHandlerholding that connection's ctx, soOnDisconnectedandOnConnectErrorfrom a replaced connection are ignored. Reconnect requests carry the ctx of the connection they are for and are skipped once it is replaced. A login resets only an existing connection, so logging in while in BACKGROUND no longer drops the gregor client until the next foreground. A queued replay is skipped once its user has logged out. The attachment HTTP server now also stops only on BACKGROUND.Ordering
Independent of the app-state stack at compile level — it consumes only
MobileAppState.State()/NextUpdate()andDesktopAppState.Suspended()/NextSuspendUpdate(), all unchanged on master.But it should land before or with #29651. That PR makes Android boot in BACKGROUND, and master's
gregor.go:275seeds its loop with a hardcodedMobileAppState_FOREGROUND— so after #29651 the loop wakes immediately at boot, reads BACKGROUND and disconnects. This PR is what makes that seed correct. Landing it after #29651 leaves a window where Android boots with gregor down.Touches
go/chat/types/interfaces.go,go/service/main.goandgo/kbhttp/manager/manager.go, which the stack also touches in different hunks — a trivial rebase for whichever merges second.Verification
go build,go vet,gofmtclean,golangci-lint --new-from-rev master0 issues. 34 gregor connection tests and the HTTP server app-state test pass, including under-race. The tests for the replaced-connection, reconnect, replay and HTTP server guards each fail with their guard removed.Not verified here:
TestGregorHandler,TestGregorBadgesIBM,TestGregorTeamBadges,TestGregorBadgesOOBMand mostTestSyncer*need kbweb onlocalhost:3000, which was not available. They are untouched by this change; CI is the first real check on them.TestGregorConnScenarioReplayis deliberately omitted — it needs thelifecycle/lifecycletestpackage, which lands in #29651. Add it once both are in.