Skip to content

Close resources left by go-git after request is finished #8916

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 1 commit into from

Conversation

Sebazzz
Copy link

@Sebazzz Sebazzz commented Nov 10, 2019

Rebase of #8915 onto master

In #7947 I figured out that go-git leaves handles to the pack files open. These are eventually garbage collected, but this isn't very deterministic.

What I did:

  • Close any open repository handles after each request via a common middleware so it is on one place instead of littered throughout the source code. (There might still be a possible race condition here if many concurrent page views are happening on the repository and a transfer ownership or rename is attempted)
  • Close the repository before ownership transfer or rename which are the operations that touch the directory.
  • Log if closing the repository fails, apparently go-git can return an error.
  • Tested on Windows based on the testcase in Transfer the ownership 转移仓库所有权 do not work in V1.90~V1.92 #7947 as I'm a Windows user.

I believe this should be quite easy to merge and addresses all issues when it comes to resources left open by requests.

…ctions that touch the repository directory

Especially Windows doesn't like it when there are open handles when a directory is moved or copied.
Still, it is good to clean-up these resources after a request instead of relying on GC.
@codecov-io
Copy link

Codecov Report

Merging #8916 into master will increase coverage by <.01%.
The diff coverage is 48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8916      +/-   ##
==========================================
+ Coverage    41.3%   41.31%   +<.01%     
==========================================
  Files         545      545              
  Lines       70368    70393      +25     
==========================================
+ Hits        29068    29080      +12     
- Misses      37601    37611      +10     
- Partials     3699     3702       +3
Impacted Files Coverage Δ
routers/repo/setting.go 13.13% <0%> (-0.2%) ⬇️
modules/git/repo.go 56.79% <100%> (+0.42%) ⬆️
routers/routes/routes.go 82.61% <100%> (+0.04%) ⬆️
modules/context/context.go 50.22% <72.72%> (+1.17%) ⬆️
models/unit.go 62.16% <0%> (-5.41%) ⬇️
modules/migrations/migrate.go 21.22% <0%> (-1.68%) ⬇️
modules/migrations/gitea.go 8.05% <0%> (-0.64%) ⬇️
models/repo.go 49.07% <0%> (-0.06%) ⬇️
models/error.go 33.86% <0%> (+1.18%) ⬆️
modules/task/migrate.go 28.94% <0%> (+3.94%) ⬆️

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 01a4a7c...f87ca90. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 10, 2019
@Sebazzz Sebazzz closed this Nov 12, 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.

3 participants