Skip to content
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

Fix data-race problems in git module (quick patch) #19934

Merged
merged 3 commits into from
Jun 11, 2022

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Jun 10, 2022

This patch is somewhat hacky, it might be the most simple one to resolve the data-race problem in git.Init.

Details:

Before git refactoring: git.Init was only called once in GlobalInitInstalled, which is incomplete, because the tests also need to init the git module.

With the last git refactoring #19732: git.Init was called in GlobalInitInstalled and many prepareTestEnv functions, but git.Init changes many global variables, then it leads to data-race problem.

This PR: introduce git.InitOnceWithSync which only changes the global variables once, just like what git.CheckLFSVersion does.

Although this PR is not ideal and hacky, (hopefully) it should work and be enough for 1.17 release. In the future, the setting related code should be refactored (eg: by using atomic.Value), including the git.CheckLFSVersion.

@wxiaoguang wxiaoguang added type/refactoring Existing code has been cleaned up. There should be no new functionality. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Jun 10, 2022
@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Jun 10, 2022
@zeripath
Copy link
Contributor

By using a sync.Once we're limited to configuring this function once and once only per run (which is fine for Gitea as we are currently) but this also applies to the tests too.

Any attempt to call this function to reconfigure will fail but that means it could be possible that a different order of calling Tests could cause a different configuration to be set.

So ... the first function calling this Init should probably be being called from within the TestMain(s) to ensure that there is a consistent configuration.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 10, 2022
@wxiaoguang
Copy link
Contributor Author

By using a sync.Once we're limited to configuring this function once and once only per run (which is fine for Gitea as we are currently) but this also applies to the tests too.

Yes, but the same problem to git.CheckLFSVersion ......

Without a big refactoring, the problem can not be resolved clearly.

@wxiaoguang
Copy link
Contributor Author

Why I think this PR is enough:

Before refactoring:

  • Gitea web: call git.Init once
  • Gitea serv: no call to git.Init
  • Test code: no call to git.Init before GlobalInitInstalled (which calls git.Init)

After the refactoring and this PR:

  • Gitea web: call git.InitOnceWithSync once, it can be guaranteed to be correct
  • Gitea serv: call git.InitSimple once, not bad, no side effect
  • Test code: call git.InitOnceWithSync in prepare with the same settings as GlobalInitInstalled, and at the moment everything works fine.

Indeed, the situation of refactoring the git module is more complex than I thought. IMO at the moment it's impossible to achieve the ideal status with a few PRs, especially there is a blocker in git.CheckLFSVersion, the changing to them will cause a lot of changed code.

And I am open to learn ideas about how to make the git module better.

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we're somewhat forced to do the syncOnce here.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 10, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 11, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #19934 (ba99d59) into main (73382d2) will decrease coverage by 0.02%.
The diff coverage is 57.24%.

@@            Coverage Diff             @@
##             main   #19934      +/-   ##
==========================================
- Coverage   47.32%   47.30%   -0.03%     
==========================================
  Files         959      965       +6     
  Lines      133652   134013     +361     
==========================================
+ Hits        63252    63389     +137     
- Misses      62699    62904     +205     
- Partials     7701     7720      +19     
Impacted Files Coverage Δ
cmd/admin.go 0.00% <0.00%> (ø)
cmd/serv.go 2.22% <0.00%> (-0.11%) ⬇️
models/db/engine.go 32.65% <ø> (-0.91%) ⬇️
models/repo/repo.go 64.66% <0.00%> (-0.49%) ⬇️
models/repo/repo_unit.go 77.14% <ø> (ø)
models/webhook/webhook.go 68.81% <0.00%> (-1.26%) ⬇️
modules/git/commit.go 56.92% <ø> (+0.38%) ⬆️
modules/git/lfs.go 66.66% <ø> (+7.84%) ⬆️
modules/git/remote.go 80.00% <ø> (+11.57%) ⬆️
modules/git/repo_attribute.go 69.17% <ø> (+0.65%) ⬆️
... and 114 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23422f9...ba99d59. Read the comment docs.

@lunny lunny merged commit 88f2e45 into go-gitea:main Jun 11, 2022
@wxiaoguang wxiaoguang deleted the patch-data-race branch June 11, 2022 04:33
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 11, 2022
* giteaoffical/main:
  Fix data-race problems in git module (quick patch) (go-gitea#19934)
  [skip ci] Updated translations via Crowdin
  Fix copy/paste of empty lines (go-gitea#19798)
  Normalize line endings in fomantic build files (go-gitea#19932)
  Make user profile image show full image on mobile (go-gitea#19840)
  Custom regexp external issues (go-gitea#17624)
  Use Golang 1.18 for Gitea 1.17 release (go-gitea#19918)
  Refactor git module, make Gitea use internal git config (go-gitea#19732)
  [skip ci] Updated translations via Crowdin
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
* Fix data-race problems in git module

* use HomeDir instead of setting.RepoRootPath

Co-authored-by: Lunny Xiao <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants