From 7731311674bbb5eb171b7d59b40dfd0e5a6cbc84 Mon Sep 17 00:00:00 2001 From: Brandon Date: Tue, 4 Feb 2025 17:11:09 -0800 Subject: [PATCH] Don't try killing processes if we already know the command finished This may lead to unrelated processes being killed on Windows (https://github.com/jesseduffield/lazygit/issues/3008). Imagine: 1. lazygit is started and runs git diff in process X which completes immediately and exits. 2. lazygit is left in the background for several hours by which process X pid is reused by an unrelated process. 3. lazygit is focused back on and runs another git diff. It first runs this stop logic which will kill process X and its children. --- pkg/tasks/tasks.go | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index e80b63a2a..b488152a5 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -137,21 +137,31 @@ func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), p cmd, r := start() timeToStart := time.Since(startTime) - go utils.Safe(func() { - <-opts.Stop - // we use the time it took to start the program as a way of checking if things - // are running slow at the moment. This is admittedly a crude estimate, but - // the point is that we only want to throttle when things are running slow - // and the user is flicking through a bunch of items. - self.throttle = time.Since(startTime) < THROTTLE_TIME && timeToStart > COMMAND_START_THRESHOLD - if err := oscommands.Kill(cmd); err != nil { - if !strings.Contains(err.Error(), "process already finished") { - self.Log.Errorf("error when running cmd task: %v", err) - } - } + done := make(chan struct{}) - // for pty's we need to call onDone here so that cmd.Wait() doesn't block forever - onDone() + go utils.Safe(func() { + select { + case <-done: + // The command finished and did not have to be preemptively stopped before the next command. + // No need to throttle. + self.throttle = false + case <-opts.Stop: + // we use the time it took to start the program as a way of checking if things + // are running slow at the moment. This is admittedly a crude estimate, but + // the point is that we only want to throttle when things are running slow + // and the user is flicking through a bunch of items. + self.throttle = time.Since(startTime) < THROTTLE_TIME && timeToStart > COMMAND_START_THRESHOLD + + // Kill the still-running command. + if err := oscommands.Kill(cmd); err != nil { + if !strings.Contains(err.Error(), "process already finished") { + self.Log.Errorf("error when trying to kill cmd task: %v; Command: %v %v", err, cmd.Path, cmd.Args) + } + } + + // for pty's we need to call onDone here so that cmd.Wait() doesn't block forever + onDone() + } }) loadingMutex := deadlock.Mutex{} @@ -159,8 +169,6 @@ func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), p // not sure if it's the right move to redefine this or not self.readLines = make(chan LinesToRead, 1024) - done := make(chan struct{}) - scanner := bufio.NewScanner(r) scanner.Split(utils.ScanLinesAndTruncateWhenLongerThanBuffer(bufio.MaxScanTokenSize))