diff --git a/pkg/commands/git_commands/github.go b/pkg/commands/git_commands/github.go index e05472ef1..b74815301 100644 --- a/pkg/commands/git_commands/github.go +++ b/pkg/commands/git_commands/github.go @@ -210,7 +210,10 @@ func (self *GitHubCommands) fetchRecentPRsAux(endpoint string, repoOwner string, req.Header.Set("Authorization", "token "+token) req.Header.Set("Content-Type", "application/json") - client := &http.Client{} + // Bound the request so that a dead or extremely slow network can't leave + // the pull-request refresh in flight for minutes. The data is auxiliary, + // so giving up and retrying on the next refresh beats waiting. + client := &http.Client{Timeout: 10 * time.Second} resp, err := client.Do(req) if err != nil { return nil, err diff --git a/pkg/gui/background.go b/pkg/gui/background.go index f9eff420b..1e2db853f 100644 --- a/pkg/gui/background.go +++ b/pkg/gui/background.go @@ -119,7 +119,7 @@ func (self *BackgroundRoutineMgr) startBackgroundFetch() { if self.gui.UserConfig().Gui.ShowBottomLine || firstTimeOrRetriggered { return self.gui.helpers.AppStatus.WithWaitingStatusImpl(self.gui.Tr.FetchingStatus, func(gocui.Task) error { return self.backgroundFetch() - }, nil, true) + }, nil) } return self.backgroundFetch() diff --git a/pkg/gui/controllers/helpers/app_status_helper.go b/pkg/gui/controllers/helpers/app_status_helper.go index 69daa7d7a..d0bb03395 100644 --- a/pkg/gui/controllers/helpers/app_status_helper.go +++ b/pkg/gui/controllers/helpers/app_status_helper.go @@ -34,12 +34,7 @@ func (self *AppStatusHelper) Toast(message string, kind types.ToastKind) { self.statusMgr().AddToastStatus(message, kind) - // Render the toast in the background: it's a transient notification, not - // lazygit driving an operation, so it must not count towards being busy — - // otherwise a toast (e.g. the "can't switch, operation in progress" one) - // would itself block a repo switch until it faded. A real operation showing - // a toast still keeps its own foreground task busy independently. - self.renderAppStatus(true) + self.renderAppStatus() } // A custom task for WithWaitingStatus calls; it wraps the original one and @@ -66,14 +61,15 @@ func (self appStatusHelperTask) Continue() { // WithWaitingStatus wraps a function and shows a waiting status while the function is still executing func (self *AppStatusHelper) WithWaitingStatus(message string, f func(gocui.Task) error) { self.c.OnWorker(func(task gocui.Task) error { - return self.WithWaitingStatusImpl(message, f, task, false) + return self.WithWaitingStatusImpl(message, f, task) }) } -// background reports whether this waiting status belongs to a background routine -// (the auto-fetch poller); when it does, the spinner it drives must not count -// towards lazygit being busy, or it'd block repo switches while a fetch runs. -func (self *AppStatusHelper) WithWaitingStatusImpl(message string, f func(gocui.Task) error, task gocui.Task, background bool) error { +// WithWaitingStatusImpl is WithWaitingStatus for callers that already run on a +// goroutine of their own (e.g. the auto-fetch poller) rather than wanting the +// work dispatched to a worker. task is used to hide the status while the task +// is paused; it may be nil for callers whose f ignores its task. +func (self *AppStatusHelper) WithWaitingStatusImpl(message string, f func(gocui.Task) error, task gocui.Task) error { // A waiting status means lazygit is driving a git operation itself (often // one that internally runs a rebase and continues it). Pause the background // routines for its duration so they don't refresh from an intermediate @@ -81,7 +77,7 @@ func (self *AppStatusHelper) WithWaitingStatusImpl(message string, f func(gocui. self.c.PauseBackgroundRefreshes(true) defer self.c.PauseBackgroundRefreshes(false) - return self.statusMgr().WithWaitingStatus(message, func() { self.renderAppStatus(background) }, func(waitingStatusHandle *status.WaitingStatusHandle) error { + return self.statusMgr().WithWaitingStatus(message, self.renderAppStatus, func(waitingStatusHandle *status.WaitingStatusHandle) error { return f(appStatusHelperTask{task, waitingStatusHandle}) }) } @@ -112,7 +108,7 @@ func (self *AppStatusHelper) WithWaitingStatusBlockingInput(message string, f fu self.modeHelper.SetSuppressRebasingMode(false) return self.c.GocuiGui().EndBlockingEvents() }) - return self.WithWaitingStatusImpl(message, f, task, false) + return self.WithWaitingStatusImpl(message, f, task) }) } @@ -125,33 +121,36 @@ func (self *AppStatusHelper) GetStatusString() string { return appStatus } -func (self *AppStatusHelper) renderAppStatus(background bool) { - // A background waiting status (auto-fetch) must not count towards lazygit - // being busy, so its spinner worker and per-frame UI updates go through the - // background variants. - onWorker := self.c.OnWorker - onUIThread := self.c.OnUIThread - onUIThreadContentOnly := self.c.OnUIThreadContentOnly - if background { - onWorker = self.c.OnWorkerBackground - onUIThread = self.c.OnUIThreadBackground - onUIThreadContentOnly = self.c.OnUIThreadContentOnlyBackground +// renderAppStatus ensures the render loop that keeps the app-status view up to +// date is running. There is one loop for the whole status stack, no matter how +// many statuses are showing: it draws whatever the top status currently is, +// and exits after drawing a final empty frame once the last status is removed. +// +// The loop always runs as a background task, regardless of what kind of +// operation owns a status: rendering runs no git commands, so it must never +// count towards lazygit being busy — otherwise it would block repo switching +// for as long as anything is showing (e.g. for the whole duration of a hung +// background fetch, or of a toast fading). A foreground operation's busy-ness +// is carried by its own worker task, not by the renderer. +func (self *AppStatusHelper) renderAppStatus() { + if !self.statusMgr().ClaimRenderLoop() { + return } - onWorker(func(_ gocui.Task) error { + self.c.OnWorkerBackground(func(_ gocui.Task) error { ticker := time.NewTicker(time.Millisecond * time.Duration(self.c.UserConfig().Gui.Spinner.Rate)) defer ticker.Stop() prevAppStatus := "" for range ticker.C { appStatus, color := self.statusMgr().GetStatusString(self.c.UserConfig()) - update := onUIThreadContentOnly + update := self.c.OnUIThreadContentOnlyBackground if utils.StringWidth(appStatus) != utils.StringWidth(prevAppStatus) { // Need a full layout whenever the width of the status string changes. This can't // happen during normal spinning because we validate that all spinner frames have // the same width, so typically this will only be triggered at the beginning and end // of a status, or if the status string changes midway for some reason. - update = onUIThread + update = self.c.OnUIThreadBackground } update(func() error { self.c.Views().AppStatus.FgColor = color @@ -160,7 +159,9 @@ func (self *AppStatusHelper) renderAppStatus(background bool) { }) prevAppStatus = appStatus - if appStatus == "" { + // Checked after rendering, so that the frame which clears the view + // has already been drawn when we exit. + if self.statusMgr().ReleaseRenderLoopIfEmpty() { break } } diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 4fedf9c7d..a7c18aeb8 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -365,7 +365,17 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr } if scopeSet.Includes(types.PULL_REQUESTS) { - self.onWorker(env.background, func(gocui.Task) error { + // Fetching pull requests talks to the GitHub API over the network; on + // a bad connection that request can stall for a long time. It runs no + // git commands against the repo, and its model writes are guarded by + // the repo generation (a repo switch mid-fetch simply drops the + // result), so it is safe to run as a background task even when the + // enclosing refresh is a foreground one — a foreground task would + // block repo switching for as long as the request takes. The env copy + // makes the downstream UI-thread bounces background as well. + prEnv := env + prEnv.background = true + self.c.OnWorkerBackground(func(gocui.Task) error { branchesAndRemotesWg.Wait() t := time.Now() @@ -373,7 +383,7 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr // Model().Branches/Remotes: those writes are bounced onto the // UI thread and may not have landed on this worker yet. The // wait above orders us after both loads have stashed theirs. - self.refreshGithubPullRequests(loadedBranches, loadedRemotes, env) + self.refreshGithubPullRequests(loadedBranches, loadedRemotes, prEnv) self.c.Log.Infof("refreshed pull requests in %s", time.Since(t)) return nil }) diff --git a/pkg/gui/status/status_manager.go b/pkg/gui/status/status_manager.go index 414568a69..35e1b7746 100644 --- a/pkg/gui/status/status_manager.go +++ b/pkg/gui/status/status_manager.go @@ -17,6 +17,11 @@ type StatusManager struct { statuses []appStatus nextId int mutex deadlock.Mutex + + // Whether a render loop is currently drawing the statuses. Guarded by + // mutex, so that claiming and releasing the loop stay atomic with the + // changes to statuses; see ClaimRenderLoop and ReleaseRenderLoopIfEmpty. + renderLoopRunning bool } // Can be used to manipulate a waiting status while it is running (e.g. pause @@ -90,6 +95,39 @@ func (self *StatusManager) HasStatus() bool { return len(self.statuses) > 0 } +// ClaimRenderLoop is called by whoever just added a status; it reports whether +// they must start the render loop. When it returns false, a loop is already +// running and will pick the new status up on its next tick. +func (self *StatusManager) ClaimRenderLoop() bool { + self.mutex.Lock() + defer self.mutex.Unlock() + + if self.renderLoopRunning { + return false + } + + self.renderLoopRunning = true + return true +} + +// ReleaseRenderLoopIfEmpty is called by the render loop after each frame it +// draws; a true result releases the loop's claim and tells it to exit, because +// there are no statuses left to draw. The emptiness check and the release are +// atomic with respect to ClaimRenderLoop, so a status added around this moment +// either sees the still-running loop or starts a fresh one — it can't end up +// unrendered. +func (self *StatusManager) ReleaseRenderLoopIfEmpty() bool { + self.mutex.Lock() + defer self.mutex.Unlock() + + if len(self.statuses) > 0 { + return false + } + + self.renderLoopRunning = false + return true +} + func (self *StatusManager) addStatus(message string, statusType string, kind types.ToastKind) int { self.mutex.Lock() defer self.mutex.Unlock()