Fix #1265

[AndrewSav]

This is an attempt to fix #1265. I've used this patch for the last 5 days and did not notice any abnormalities. I discussed the findings in the linked issue thread. I checked that the tests return the same result before and after the bugfix and ran gofumt no cheat sheets or documentation are affected.

Note that there seems to be an issue on master that makes lazygit hang on push or pull, so this PR is forked off latest working tag which is v0.38.2. Some code had changed so it cannot be rebased without a conflict, but conflict resolution should in theory be easy once the issue is solved.

@Aziks0 has discovered that a scanner hangs - huge thanks here, most of my work was based on this finding. This is what preventing the panel from updating. The fix is attempting to workaround this by making the scanner non-blocking. We are wrapping it inside a channel and then putting the channel and the stop condition in the same select. That is making sure that while we are waiting on the scanner we still can stop, so we do not block forever on an attempt to stop a prior panel update anymore.

I also wrote some code in the same select that waits for time.After and then checks for a broken pipe condition. This code relied on a third party library gopsutil. If a check for broken pipe succeeds I used to execute "stop" by calling break outer. It turned out that this was not actually required because the natural "stop" event would come anyway and and let the code proceed successfully, so I removed that bit after testing.

Disclaimer: not a go programmer, susceptible to silly mistakes.

[jesseduffield]

Is there a risk that we'll get a bunch of these goroutines banked up after iterating through a bunch of items in a side view? Like, do we eventually make it out of the scanner loop here? If not, it would be good if there was a way to force us to return from the goroutine.

Although in saying that, if that were possible we'd be able to solve it directly in the original code, hmm...

[AndrewSav]

Is there a risk that we'll get a bunch of these goroutines banked up after iterating through a bunch of items in a side view?

I just updated the PR description, could you please read and tell me if it makes sense. I also had this pre-conception initially and even wrote more code to deal with it but the testing shows that an old goroutine is killed before a new one can go ahead so it seems they will never back up - that code in the end never got warm. There could be more than one as my testing shows but they all will be stopped one by one eventually, as each new one wait for the previous one to stop.

[AndrewSav]

Although in saying that, if that were possible we'd be able to solve it directly in the original code, hmm...

This is entirely up to you, but I think there is still value in merging this:

• it seems to resolve the issue
• it can act as a failsafe if there are any further issue with the other resolution
• it could take some time to do the other resolution, while merging this will unblock people waiting on this long standing issue

[AndrewSav]

Like, do we eventually make it out of the scanner loop here?

Oh sorry, it took some time to sink in. So what I believe happens here, is that on stop we end up with kill and the kill get scanner unblocked because the process and the pipe will no longer be there after the kill so the goroutine is let go. Not 100% sure, but not sure what is the best way to test this.

update No, wait that does not make sense. The suspended processes are still left behind which means they are never killed and those would bank up as you described. The other fix should fix that, but this may be used as stopgap while we wait for it. I do admit there may be some leakage, but hopefully not severe (as per my testing) and we make some progress this way. Once all the processes are killed properly by adding to a windows job it can be resolved completely.

On that note, I'm not 100% sure that the job approach will work when there are TWO shims, one from scoop and one from git for windows itself. We cannot control how inner shims spawn the process, so whether it works on not is remaining to be seen.

[AndrewSav]

Sorry I have a habit of editing my comments as my thinking evolves I edited some of the above. As you mention in the issue thread in theory we could change the scanner to a reader to avoid the blocking altogether, but I would need help with this, as it seems (by looking at the scanner source code) quite complicated. Without doing that and without the other fix I do not think this could be easily resolved. I aimed for something low effort that would allow people to move on.

[jesseduffield]

No need to apologise :) I'm leaning towards going with this approach to fix the immediate problem and then looking for a better fix. It's better to have a memory leak than for nothing to work at all.

My main hesitation is around performance: I'm not sure what effect it'll have to have each line passed to a separate goroutine (for osx/linux where there's no issue atm)

[AndrewSav]

@jesseduffield again, possibly dumb question, but where each line is passed to a separate goroutine? As far as I can see we added one go routine in total the one that hold the loop with the scanner. Are we creating a goroutine for every line anywhere?

You may be referring for the lines on the left of lazygit UI, but this part we have not changed we always created a goroutine per line as user press up and down arrows. Can you talk a bit more about your hesitation?

[AndrewSav]

If we look at go routine creations it this function the new one is one of four. It has probably the least effect on performance of the four because reading a scanner is not very resource intensive operation.

[jesseduffield]

To be clear, I'm not worried about spawning too many goroutines, I'm worried about each line of the command's output needing to go through a channel, such that the go runtime needs to hand off control from the scanner goroutine to the other goroutine many times. This is pure speculation of course, and I'll do some profiling to test the hypothesis now

[jesseduffield]

Great work @AndrewSav !