preparations to remove context from repo struct

[TheFox0x7]

first round of adding contexts to functions requiring it

---

trying keep it small'ish

[wxiaoguang]

Is it clear that the change is safe? If the ctx is abused, there would be deadlock problems.

For example: in CatFileBatchCheck, although there is a ctx parameter, actually it also uses "cached" repo.check created by previous ctx.

[wxiaoguang]

There are already some deadlock problems like this

• PR Diff View takes a very long time to load: failed to run attr-check: context deadline exceeded #31600

So there could be 3 results of the "ctx" refactoring:

• Will fix the deadlock problems: good end but we don't know how/why it fixes (unclear ....)
• Leave the deadlock problems there: not good or bad, we still need to try to fix the deadlock problems.
• Introduce more deadlock problems: bad end, things become worse

So ideally I think we should figure out and fix the deadlock problems before introducing unclear changes, and only introduce clear changes with necessary tests.

---

The first clear and necessary change in my mind is to clarify the func (repo *Repository) CatFileBatchCheck behavior: make sure the ctx won't be abused and the BatchCheck won't use wrong cached ctx.

[TheFox0x7]

As far as I know this brings no functional change, it just changes/how when the context gets passed from: request -> open gitRepo with context -> function, to request -> open gitRepo -> function with context.
And it's far from being finished but I don't want to do it all in one go as few functions will have a 100+ line change on their own so it'll start being unreadable.

---

For whatever reason it took me a far bit to understand what exactly you meant but I see what you're talking about now.
If the cat-file is meant to be used request wide (as I think it's used now) then primarily the request context should handle it's lifecycle.

// Note those are very basic and don't represent real operations
func SomeEndpoint(ctx *context.Context){
gitRepo,err:=git.openRepository(ctx.repo)

...:=gitRepo.SomeGitFunction(ctx)

func (repo ...) SomeGitFunction(ctx,...){
// It uses CatFileBatch underneath
...:=repo.CatFileBatch(ctx) // request scoped so might be stuck of a lot of data gets requested
}

If it's meant to more usecase specific one I'd opt for local one but that's more wasteful probably:

func SomeEndpoint(ctx *context.Context){
gitRepo,err:=git.openRepository(ctx.repo)

...:=gitRepo.SomeGitFunction(ctx)

func (repo ...) SomeGitFunction(ctx,...){
ctx,cancel:=context.WithCancel(ctx)
defer cancel()
// It uses CatFileBatch underneath
...:=repo.CatFileBatch(ctx) // as soon as it's useless it's removed
}

I'll look into it more though input is welcome.
Few loose thoughts:
Why is the output buffered (twice)?
Maybe channels for IO would be better here? Or req/reply (I've spent too much time with nats lately and it show)

[TheFox0x7]

The first clear and necessary change in my mind is to clarify the func (repo *Repository) CatFileBatchCheck behavior: make sure the ctx won't be abused and the BatchCheck won't use wrong cached ctx.

Can you clarify what I can do to help with it?
I think both batch functions should use request context when possible and cancellable ones otherwise but this is pretty much expected.
I'm also not sure why is there a temporary version when the cached one is busy...

[wxiaoguang]

Maybe it needs a complete refactoring to make these functions overall right.

TBH I don't quite understand the old design either. By reading the commit history briefly, I can see that at the beginning there was no "ctx", so the "cached batch checker" was right. Then, "ctx" support was added, then more and more places started using "ctx", then the "cached batch checker with ctx" might go wrong. There are still many technical debts to pay. 🤷‍♂️

IIRC (just FYI) there were other "ctx" patches Use request timeout for git service rpc (#20689), Make git batch operations use parent context timeout instead of default timeout (#26325) due to various legacy ctx abuses. And #26325 is more or less related to this discussion & refactoring: CatFileBatchCheck

---

I'm also not sure why is there a temporary version when the cached one is busy...

No idea, either some people did too much "defensive programming" and added unnecessary design, or there is a real case that it would happen in real world. 🤷‍♂️

[lunny]

I created #33934 to try to rework CatFile implementation.