Skip to content

WIP: Fix data race (Repository.Units) #8223

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.

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

Closed
wants to merge 10 commits into from

Conversation

typeless
Copy link
Contributor

@typeless typeless force-pushed the fix-repo-units-race branch 2 times, most recently from 2a73a72 to 904d90f Compare September 18, 2019 05:09
jaqra and others added 5 commits September 18, 2019 13:23
The current webhook just shows the amount of commits, but misses the actual commit description. While the code is actually there to include the description, it is just not included.

Signed-off-by: Bjoern Petri <[email protected]>
@lunny
Copy link
Member

lunny commented Sep 18, 2019

These races may be caused by permission's Units. I think a simple method is to change []*RepoUnit to []RepoUnit but not change more.

// Permission contains all the permissions related variables to a repository for a user
type Permission struct {
	AccessMode AccessMode
	Units      []*RepoUnit
	UnitsMode  map[UnitType]AccessMode
}

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 18, 2019
@typeless
Copy link
Contributor Author

@lunny As I understand, the races are due to the concurrent access to the slice header (the ptr, len, and cap fields), besides the backing array. I am afraid that changing the type of slice element would not help. Maybe I am wrong, though.

It would be better if this can be solved by an architectural change that avoids sharing the data in any way. But I don't have a clear picture of how to do it.

@lunny
Copy link
Member

lunny commented Sep 18, 2019

Or we could copy the slices only but not change the slice element type. Permission's Unit is a reference of Repo.Units that has been changed by some other goroutines. I think that's the problem.

@typeless
Copy link
Contributor Author

Another way is to fix the upper-level code by avoiding sharing the objects in the first place. One implementation I tried is to deep-copy the Issue objects before we forward them into different handlers. Unfortunately, the objects contain cycles, so I cannot use a library routine to accomplish that automatically.

I think there is a chance that we can pass the objects through the channels by value, not by pointers, to avoid sharing the memory.

@lunny
Copy link
Member

lunny commented Sep 29, 2019

Please resolve conflicts

@typeless
Copy link
Contributor Author

typeless commented Oct 1, 2019

@lunny I think this PR is not good. It touches too much code but does not address it fundamentally. We have to decide whether we want to make the model layer thread-safe (which is a big endeavor) or we take care of the concurrency problem at the upper layers (like passing the objects by values instead of pointers).

@typeless typeless closed this Oct 1, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants