Add finalizers to ensure that repos are closed and blobreaders are closed (#19495)

[zeripath]

Backport #19495

It may be prudent to add runtime finalizers to the git.Repository and
git.blobReader objects to absolutely ensure that these are both properly
cancelled, cleaned and closed out.

This commit is a backport of an extract from #19448

Signed-off-by: Andrew Thornton [email protected]

[Gusted]

In the Close function we want to set the the Finalizer of that object to nil.

[lunny]

I don't think it's a good idea.

[zeripath]

In the Close function we want to set the the Finalizer of that object to nil.

Not necessarily. It's probably worth still rerunning the Close() again at time of GC().

[wxiaoguang]

For 1.16, it might work and might resolve some resource leaking problems.

For 1.17, I am a little conservative about it. The reason is that GC time can not be controlled (well, should not be controlled), and sometimes the leaked resource may be still referenced by other variables while the resource itself should be closed. So I think it's safer and proper to fix these leaked resources if there is a chance. The runtime finalizers might help developers to find these leaked resources.

[zeripath]

Please note, I have no information to suggest that we have any leaks, nor that we should consider that relying on finalizers is a good idea.

However, if we do have leaks - it is infinitely better for a finalizer to take care of them than to have a leak there.

[wxiaoguang]

Please note, I have no information to suggest that we have any leaks, nor that we should consider that relying on finalizers is a good idea.

If no leak, why the finalizers are needed? Close a already closed object?

However, if we do have leaks - it is infinitely better for a finalizer to take care of them than to have a leak there.

I also agree that finalizers can help to prevent leaking. I mean if there are some leaks, we need logs to tell developers that there is something incorrect.

---

I saw more details in #19495 👍

[6543]

hop or top - should we move or merge?!

[wxiaoguang]

Vote to merge

[Gusted]

Agree to merge this. It doesn't any harm to merge and is a nice safety measurement.

[ashimokawa]

This PR introduced a memory leak on codeberg.org leading to about 25GB of leaked memory in 2 days. We had to revert it.

I think the real cause should be fixed before the next release.