Skip to content

Fix MemoryIndex concurrency issue #175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix MemoryIndex concurrency issue #175

wants to merge 2 commits into from

Conversation

@dustin-decker
Copy link

@dustin-decker dustin-decker commented Sep 29, 2020

When iterating over commits and computing patches concurrently within goroutines, the go race detector detects concurrent writes and reads to the offsetHash map:

fatal error: concurrent map read and map write
goroutine 13 [running]:
runtime.throw(0x8af5e9, 0x21)
	/usr/local/go/src/runtime/panic.go:1116 +0x72 fp=0xc00050b540 sp=0xc00050b510 pc=0x437672
runtime.mapaccess2_fast64(0x81eae0, 0xc00017f650, 0x23a43, 0x40d8b0, 0xc0003144c0)
	/usr/local/go/src/runtime/map_fast64.go:61 +0x1ac fp=0xc00050b568 sp=0xc00050b540 pc=0x412a2c
github.com/go-git/go-git/v5/plumbing/format/idxfile.(*MemoryIndex).FindHash(0xc000192000, 0x23a43, 0x0, 0x0, 0x7f3500000000, 0x0, 0xc00031c120)
	/builds/.cache/pkg/mod/github.com/go-git/go-git/v5@v5.1.0/plumbing/format/idxfile/idxfile.go:176 +0x1a5 fp=0xc00050b5c8 sp=0xc00050b568 pc=0x62f8e5

Fixed by adding a sync.RWMutex to be used during reads and writes.

Thank you for the excellent lib.

@dustin-decker dustin-decker force-pushed the master branch 2 times, most recently from 0ab623a to 32931b1 Compare October 1, 2020 01:30
@dustin-decker dustin-decker mentioned this pull request Nov 22, 2020
Closed
@dustin-decker
Copy link
Author

dustin-decker commented Nov 25, 2020

I prefer #186 over this fix but would be very happy if either get merged 🙏

@MichaelMure MichaelMure mentioned this pull request Jan 24, 2021
Closed
@cep21
Copy link

cep21 commented Nov 10, 2021

Thanks for this. It was very surprising to me that even read only operations on go-git are not race safe.

@pjbgf
Copy link
Member

pjbgf commented Dec 3, 2022 •
edited
Loading

I ran the existing benchmarks on this (rebased), and here are the results (considering only the relevant deltas):

goos: linux goarch: amd64
cpu: 11th Gen Intel(R) Core(TM) i7-11800H @ 2.30GHz

name           old time/op    new time/op    delta
FindOffset-16     279ns ± 4%     427ns ± 7%   ~     (p=0.100 n=3+3)
FindCRC32-16      181ns ± 2%     217ns ±15%   ~     (p=0.100 n=3+3)
Contains-16       124ns ± 1%     142ns ±12%   ~     (p=0.100 n=3+3)
Entries-16       1.23µs ± 4%    0.97µs ±19%   ~     (p=0.100 n=3+3)

I would prefer this fix over #186.

This was referenced Dec 5, 2022
Merged
Closed
@pjbgf
Copy link
Member

pjbgf commented May 20, 2023

I created a branch to start amassing fixes around concurrency issues. As this has a performance cost, I won't propose merging it into master just yet. Folks insterested on using it can consume those changes via:

go get github.com/go-git/go-git/v5@read-concurrency

Closing this in favour of the new branch.

@pjbgf pjbgf closed this May 20, 2023
@pjbgf pjbgf mentioned this pull request May 20, 2023
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dustin-decker @cep21 @pjbgf