Add finalizers to ensure that repos are closed and blobreaders are closed

[zeripath]

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 an extract from #19448

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

[wxiaoguang]

There could be an error log to tell developers that the resource is not closed correctly. Otherwise this patch only hide problems.

[lunny]

I don't think it's a good idea to do that. We should find the real problem.

[zeripath]

I don't think it's a good idea to do that. We should find the real problem.

I have no information to suggest that we have any leaks, nor that we should rely on finalizers to deal with them.

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

But I do take your point that this could hide a leak - which isn't great so to that end I have pushed a commit that would add some warnings IF the finalizer were to be relied on.

[6543]

I just wonder if this cost us performance?

[zeripath]

So there will probably be a small perf hit:

1. The addition of locks on the Repository will have some effect.
2. The GC will hit these objects twice. So in the finalizer it may be sensible to nil out as much of the struct as we can.

Now... the addition of locks is going to be helpful because if this gets merged we can try not opening the batch and batchcheck readers until they're actually first called for.

[Gusted]

2. The GC will hit these objects twice.

We should remove the finalizer on the object on the Close() calls to avoid this as much as possible.

[Gusted]

Small note: since when are backports merged before the normal PR are merged?

[zeripath]

OK @Gusted I've setFinalizer(nil) for these structs on close. (except the go-git variant as I am fairly sure that the storage can be reopened even after close.

[ashimokawa]

The backport version #19496 introduced a major memory leak.

I am not sure if the recent commits added this one would fix it.

[6543]

#19659

[wxiaoguang]

FYI, I added a Context.Close() recently, it might help for closing http request context resources. Well, if there are some resources really leaked for other reasons, the Close can not help them. And the reason why the finalizers caused more memory leaks needs to be investigated if it's really the case.

gitea/modules/context/context.go

Lines 78 to 86 in cd99540

	// Close frees all resources hold by Context
	func (ctx *Context) Close() error {
	var err error
	if ctx.Req != nil && ctx.Req.MultipartForm != nil {
	err = ctx.Req.MultipartForm.RemoveAll() // remove the temp files buffered to tmp directory
	}
	// TODO: close opened repo, and more
	return err
	}