Skip to content

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

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

I believe this a bugfix so I based this on the branch for hopefully a release in 1.9.6. It is a relatively small change but does prevent some resource leaks from happening. If this is not desired I'Il rebase on another branch.

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.

@zeripath
Copy link
Contributor

@Sebazzz see #8901...

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 10, 2019
…ctions that touch the repository directory

Especially Windows doesn't like it when there are open handles when a directory is moved or copied.
@Sebazzz
Copy link
Author

Sebazzz commented Nov 10, 2019

@zeripath Sorry, I didn't know you also opened a pull request. I hope did not offend you.

@zeripath
Copy link
Contributor

No problem. Could you take a look at #8901 and see if you think that covers how you think it should be done?

Given the git.Repository isn't actually stateless we're probably gonna have to stop opening and re-opening it willy-nilly and instead consider passing it functions that need it. Either that or, we need to consider opening only at the template level for things that need it.

@Sebazzz
Copy link
Author

Sebazzz commented Nov 10, 2019

Given the git.Repository isn't actually stateless we're probably gonna have to stop opening and re-opening it willy-nilly and instead consider passing it functions that need it. Either that or, we need to consider opening only at the template level for things that need it.

I'd consider making opening the repository a lazy function. Then it would run on demand instead of every time.

Or go-git could be modified (if technically possible) to have read-only mode that opens handles in a fashion that does not offend Windows on a directory move/replace.

@lunny
Copy link
Member

lunny commented Nov 10, 2019

@Sebazzz And if you want to continue your pull request, you should send it to master at first and then send back port to v1.10 / v1.9 after that merged.

@Sebazzz
Copy link
Author

Sebazzz commented Nov 10, 2019

Isn't this considered a bug fix? I actually based this in 1.9 because of the first point made in the pull request template.

@Sebazzz Sebazzz closed this Nov 10, 2019
@Sebazzz Sebazzz deleted the release/v1.9 branch November 10, 2019 13:10
@lunny
Copy link
Member

lunny commented Nov 10, 2019

@Sebazzz This is a bug will affect v1.11 / v1.10 / v1.9 . Our habit is to send to v1.11(master) at first.

@Sebazzz
Copy link
Author

Sebazzz commented Nov 10, 2019

Rebased onto master via #8916.

@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