lazygit
lazygit copied to clipboard
jesseduffield
Crash on commit
Describe the bug Lazygit crashed on a commit
To Reproduce Cannot reproduce at the moment :(
Screenshots
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x102c002d4]
goroutine 1 [running]:
github.com/jesseduffield/lazygit/pkg/gui/controllers.(*FilesController).canEditFiles.func1(...)
github.com/jesseduffield/lazygit/pkg/gui/controllers/files_controller.go:760
github.com/samber/lo.NoneBy[...](...)
github.com/samber/[email protected]/intersect.go:85
github.com/jesseduffield/lazygit/pkg/gui/controllers.(*FilesController).canEditFiles(0x140000cddb8?, {0x1400079a178?, 0x140001f0980?,
0x1038653c8?})
github.com/jesseduffield/lazygit/pkg/gui/controllers/files_controller.go:760 +0x34
github.com/jesseduffield/lazygit/pkg/gui/controllers.(*FilesController).GetKeybindings.(*ListControllerTrait[...]).itemsSelected.func5
()
github.com/jesseduffield/lazygit/pkg/gui/controllers/list_controller_trait.go:98 +0xec
github.com/jesseduffield/lazygit/pkg/gui/controllers.(*FilesController).GetKeybindings.(*ListControllerTrait[...]).require.func6()
github.com/jesseduffield/lazygit/pkg/gui/controllers/list_controller_trait.go:38 +0x58
github.com/jesseduffield/lazygit/pkg/gui/types.(*Binding).IsDisabled(...)
github.com/jesseduffield/lazygit/pkg/gui/types/keybindings.go:47
github.com/jesseduffield/lazygit/pkg/gui.(*OptionsMapMgr).renderContextOptionsMap.func1(0x1400079a170?, 0x1?)
github.com/jesseduffield/lazygit/pkg/gui/options_map.go:44 +0x38
github.com/samber/lo.Filter[...]({0x14000894588, 0x4a, 0x102ccba94}, 0x1030551d8?)
github.com/samber/[email protected]/slice.go:15 +0x94
github.com/jesseduffield/lazygit/pkg/gui.(*OptionsMapMgr).renderContextOptionsMap(0x140000cf340)
github.com/jesseduffield/lazygit/pkg/gui/options_map.go:43 +0x224
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).renderContextOptionsMap(0x140001ec568?)
github.com/jesseduffield/lazygit/pkg/gui/options_map.go:27 +0x4c
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).layout(0x14000033808, 0x14000250000)
github.com/jesseduffield/lazygit/pkg/gui/layout.go:187 +0x60c
github.com/jesseduffield/gocui.ManagerFunc.Layout(0x0?, 0x140009a7520?)
github.com/jesseduffield/[email protected]/gui.go:726 +0x28
github.com/jesseduffield/gocui.(*Gui).flush(0x14000250000)
github.com/jesseduffield/[email protected]/gui.go:1179 +0xb0
github.com/jesseduffield/gocui.(*Gui).processEvent(0x14000250000)
github.com/jesseduffield/[email protected]/gui.go:810 +0x208
github.com/jesseduffield/gocui.(*Gui).MainLoop(0x14000250000)
github.com/jesseduffield/[email protected]/gui.go:775 +0xf0
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).Run(0x14000033808, {{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}})
github.com/jesseduffield/lazygit/pkg/gui/gui.go:855 +0x434
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).RunAndHandleError.func1()
github.com/jesseduffield/lazygit/pkg/gui/gui.go:861 +0x48
github.com/jesseduffield/lazygit/pkg/utils.SafeWithError(0x14a3935c8?)
github.com/jesseduffield/lazygit/pkg/utils/utils.go:99 +0x5c
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).RunAndHandleError(0x14000033808, {{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}})
github.com/jesseduffield/lazygit/pkg/gui/gui.go:860 +0xc4
github.com/jesseduffield/lazygit/pkg/app.(*App).Run(...)
github.com/jesseduffield/lazygit/pkg/app/app.go:270
github.com/jesseduffield/lazygit/pkg/app.Run({0x10306a498?, 0x14000134580?}, 0x140001f0840, {{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x0,
...}})
github.com/jesseduffield/lazygit/pkg/app/app.go:48 +0xb4
github.com/jesseduffield/lazygit/pkg/app.Start(0x14000049ef8, {0x0, 0x0})
github.com/jesseduffield/lazygit/pkg/app/entry_point.go:168 +0x9b4
main.main()
github.com/jesseduffield/lazygit/main.go:23 +0x98
Version info: commit=, build date=, build source=homebrew, version=0.45.2, os=darwin, arch=arm64, git version=2.48.1 git version 2.48.1
Additional context I'm running lazygit inside Neovim, using the default LazyVim config.
@tfeldmann I'm taking a look into this, trying to reproduce. Do you use range selections in your file tree? The stack trace seems to indicate that, but I don't want to tracking a red herring.
I personally don't use range selects while staging files, and I imagine most others just stage entire directories or individual files, which could explain why this bug hasn't been reported before.
Logically, I could see some sort of race condition existing between updating the range selection list, and the commit action removing newly committed files from the file tree.....
I got a similar one, tried to reproduce a couple of times now, but can't. The line seems to be a different one.
Circumstances:
- open lazygit
- add two files to a commit (pyproject.toml / uv.lock)
- create the commit -> press enter to commit
- crash 💥
Here's the log:
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x103569af0]
goroutine 1 [running]:
github.com/jesseduffield/lazygit/pkg/gui/controllers.(*FilesController).canRemove.func1(...)
github.com/jesseduffield/lazygit/pkg/gui/controllers/files_controller.go:1180
github.com/samber/lo.CountBy[...](...)
github.com/samber/[email protected]/slice.go:410
github.com/jesseduffield/lazygit/pkg/gui/controllers.(*FilesController).canRemove(0x140002e6a30, {0x14000d3c0b0, 0x1, 0x14000d1c1e0?})
github.com/jesseduffield/lazygit/pkg/gui/controllers/files_controller.go:1179 +0x70
github.com/jesseduffield/lazygit/pkg/gui/controllers.(*FilesController).GetKeybindings.(*ListControllerTrait[...]).itemsSelected.func15()
github.com/jesseduffield/lazygit/pkg/gui/controllers/list_controller_trait.go:98 +0xe0
github.com/jesseduffield/lazygit/pkg/gui/controllers.(*FilesController).GetKeybindings.(*ListControllerTrait[...]).require.func16()
github.com/jesseduffield/lazygit/pkg/gui/controllers/list_controller_trait.go:38 +0x58
github.com/jesseduffield/lazygit/pkg/gui/types.(*Binding).IsDisabled(...)
github.com/jesseduffield/lazygit/pkg/gui/types/keybindings.go:47
github.com/jesseduffield/lazygit/pkg/gui.(*OptionsMapMgr).renderContextOptionsMap.func1(...)
github.com/jesseduffield/lazygit/pkg/gui/options_map.go:44
github.com/samber/lo.Filter[...](...)
github.com/samber/[email protected]/slice.go:15
github.com/jesseduffield/lazygit/pkg/gui.(*OptionsMapMgr).renderContextOptionsMap(0x14000c332f0)
github.com/jesseduffield/lazygit/pkg/gui/options_map.go:43 +0x250
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).renderContextOptionsMap(0x140002ac4d0?)
github.com/jesseduffield/lazygit/pkg/gui/options_map.go:27 +0x48
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).layout(0x14000270008, 0x14000274000)
github.com/jesseduffield/lazygit/pkg/gui/layout.go:187 +0x948
github.com/jesseduffield/gocui.ManagerFunc.Layout(0x0?, 0x10324fbd8?)
github.com/jesseduffield/[email protected]/gui.go:728 +0x28
github.com/jesseduffield/gocui.(*Gui).flush(0x14000274000)
github.com/jesseduffield/[email protected]/gui.go:1185 +0xac
github.com/jesseduffield/gocui.(*Gui).processEvent(0x14000274000)
github.com/jesseduffield/[email protected]/gui.go:813 +0x208
github.com/jesseduffield/gocui.(*Gui).MainLoop(0x14000274000)
github.com/jesseduffield/[email protected]/gui.go:778 +0x108
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).Run(0x14000270008, {{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}})
github.com/jesseduffield/lazygit/pkg/gui/gui.go:855 +0x434
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).RunAndHandleError.func1()
github.com/jesseduffield/lazygit/pkg/gui/gui.go:861 +0x48
github.com/jesseduffield/lazygit/pkg/utils.SafeWithError(0x1400004b928?)
github.com/jesseduffield/lazygit/pkg/utils/utils.go:99 +0x5c
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).RunAndHandleError(0x14000270008, {{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}})
github.com/jesseduffield/lazygit/pkg/gui/gui.go:860 +0xc4
github.com/jesseduffield/lazygit/pkg/app.(*App).Run(...)
github.com/jesseduffield/lazygit/pkg/app/app.go:270
github.com/jesseduffield/lazygit/pkg/app.Run({0x103a43f38?, 0x140000d8580?}, 0x14000208940, {{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x0, ...}})
github.com/jesseduffield/lazygit/pkg/app/app.go:48 +0xb0
github.com/jesseduffield/lazygit/pkg/app.Start(0x1400004bef8, {0x0, 0x0})
github.com/jesseduffield/lazygit/pkg/app/entry_point.go:168 +0x9b0
main.main()
github.com/jesseduffield/lazygit/main.go:23 +0x98
I believe I have reproduced on https://github.com/ChrisMcD1/lazygit/tree/proving-race-condition-file-tree with the test:
go test ./... -run TestRefreshRaceCondition -count=20 -v
If your machine is behaving the same way as mine, roughly half of those tests should pass because the "refresh" happens after the critical section below, and half should fail because the "refresh" happens during the critical section below.
Problem seems to come down to that there is no synchronization mechanism ensuring that the slice you got indexes from with GetSelectionRange is the same slice that you index into with Get(i).
https://github.com/jesseduffield/lazygit/blob/391654984b854de32703a9ea30234d79aeff6e3b/pkg/gui/filetree/file_tree_view_model.go#L62-L67
I'm looking into a way to wrap that in a mutex. Go's intentional exclusion of re-entrant locks makes this slightly tricky for me to get right. Gonna have to rewire my brain a bit.
I'm looking into a way to wrap that in a mutex.
Long term I don't think this is a good solution. We'd have to put mutexes all over the place, this makes the code very messy and hard to maintain. The solution I envision long-term is the opposite, get rid of most mutexes and ensure that we access the data only from one thread. There's a lot of discussion about this in #2974.
That said, it's likely a pretty large amount of work to get there, and it's still not entirely clear to me if it will work out in the end. As an intermediate solution I suppose I'd be fine with adding a mutex here and there to fix those concurrency problems that actually seem to occur in practice, like this one.
I read up on #2974. Yeah, I agree "Just throw a mutex at it" doesn't feel like an elegant solution. I think it feels inelegant though for the same reason that "Move all of the reads to a single location on the UI thread" is a massive lift. There are a large number of readers that aren't acting through a shared, higher-level object that even could gate access via a RWLock. If we had centralized modifying and reading, then the locking mechanisms wouldn't feel that bad. The read-only methods would just acquire a RLock and the write methods would acquire a Lock.
I'm obviously not clairvoyant though, so I'm just gonna take the idea for a spin, and see how it comes out, with the expectation it might turn into a non-merged PR.
I imagine the steps look something like this:
- Limit the public interface of the
IFileTreeViewModel. Currently it lets its callers do anything with its underlyingIListCursorandIFileTreethat it itself can do, meaning we have no guarantee it is passing through the correct object. - Route all callers that have been implicitly relying on direct access to use the higher level object
- Move much of the business logic into private methods that don't acquire a lock
- Have all public methods acquire a read or write lock depending on their operations.
I believe that we need to do 3 and for 4 like that because of how go's mutexes don't allow you to lock for reading twice in the same goroutine. So the current implementation of FileTreeViewModel.SetTree calls FileTreeViewModel.GetSelected. The former would need a write lock, the latter would need a read lock, which is just an instant deadlock. Instead SetTree can call some private getSelected that is shared between SetTree and GetSelected, each of which acquires the appropriate lock.
If anybody knows a better Go pattern for managing locks, let me know!
I don't like this direction. It seems that the question how data can be guarded by locks would become a major design criterion for how we model our APIs, and I don't think it should be. I'd much rather model them so that they make sense from an API point of view.
Also:
"Move all of the reads to a single location on the UI thread" is a massive lift
This is not at all what I'm suggesting. Currently, most of the read access to FileTreeViewModel and similar objects is already from the UI thread (the two crashing call stacks above are examples). What I'm suggesting is that we move the writers also to the UI thread, so that we don't need to use a lock at all. We have a convenient mechanism for this already (OnUiThread), so this doesn't sound like a huge amount of work. Most of the code can probably just stay the way it is.
The approach does have a few other implications, some of which are mentioned in #2974; that's what makes it a bit expensive. But I don't think we are talking about a huge amount of client code changes.
When I said above that I'd be fine with adding a mutex as an intermediate solution, then what I meant is: if it's possible to fix the problem by adding a mutex but leaving the code mostly as it is, then I'd be fine with that. From what you explain above, this doesn't seem to be the case here, it looks like it would require fundamental API changes. In that case I'm against doing that, and would rather live with the very occasional crash that we are seeing (it seems to be pretty rare), until we have a better solution to our overall concurrency.
Ok, I hadn't looked at the code at all. We already have the mutex that I said I'd be fine with adding as a quick fix (FileTreeViewModel.RWMutex). It's just that it's only locked when writing, but not when reading.
So the quick fix would be to RLock this mutex inside canEditFiles, canRemove, or whatever other crashes get reported.
Yeah, I see how we could take that quick fix and spread it around a little bit. I'd like to spend a bit more time though and see if I can produce a more elegant solution where external callers are unaware that there is a RWMutex inside of the FileTreeViewModel. It would then only expose methods that ensure consistency between the cursor and the file tree at the end of every public method.
Here's the PR with the quick fix mentioned above, at last: #5074.
Since there's no good way to test it (it seems), it would be good to have another pair of eyes on the code changes. @ChrisMcD1, feel like having a look?