Use context cache in most queue handlers to avoid warning logs

[wolfogre]

Follow: #23186 #23054

Related to: #22294

The point is that the context cache is not only "HTTP request level cache", but also "queue handling level cache". And there are so many queues...

[wxiaoguang]

Is it correct to use cache for all these background tasks?

If yes, that's fine.

If no (eg: cache items get out-of-sync and cause bugs?), I think it's worth to add a cache.WithNoCacheContext() to explicitly declare that this context doesn't support cache, then the cache code could skip the Warn log if it detects that the context doesn't support cache by purpose.

[wolfogre]

Is it correct to use cache for all these background tasks?

TBH, I'm not entirely sure. It's correct to do this only if all queue handling tasks should take only a short time(less than 5 minutes), so it's fine when the cache gets out-of-sync. But I can't prove it or disprove it.

... add a cache.WithNoCacheContext() ...

Yeah, I believe it can resolve the warning logs. But there will be three ways to use a context:

• ctx = cache.WithCacheContext(ctx)
• ctx = cache.WithNoCacheContext(ctx)
• just ctx

What's the difference? And what if cache.WithCacheContext(cache.WithNoCacheContext(ctx)) or cache.WithNoCacheContext(cache.WithCacheContext(ctx))? 😂

Of course, we can define those cases, but I think it would be quite confusing.

---

So I think the point is not how to avoid warning logs, but that the context cache is designed for "short life cycle tasks", like handling a HTTP request, and (maybe) handling a task from queue, but developers could use it for "long life cycle tasks" by accident. IMO, five minutes is long enough.

There are some ideas for it:

• Remove the warning logs, ctx means "no cache", so we don't need cache.WithNoCacheContext. And use cache.WithCacheContext for handling HTTP requests only. Then add the cache for tasks which we are sure are worth it and have a short life cycle.
• Refactor queue.HandlerFunc from type HandlerFunc func(...Data) (unhandled []Data) to type HandlerFunc func(ctx context.Context, ...Data) (unhandled []Data), so it will be easy to enable or disable cache context for the handlers before invoking them.
• Add a lifetime to the cache context, like 5 minutes. If the cache data is still being read after 5 minutes after it was created, skip using the cache data and print warning logs, and it's also a good idea to log stack trace to identify where it is being misused.

[wxiaoguang]

I think we should explicitly declare a context as "cache-support" or "no-cache".

Think about a case (your confusing part):

ctx := cache.WithCache(...)

foo(ctx)

func foo(ctx) {
    // here, what if `foo` want to bypass the cache?
    // if we have "cache.WithNoCache", we can overwrite the "WithCache"
    // by design, Go's context only use "Value" to access it's context data, so IMO it's overwrite-able.
    ctx = cache.WithNoCache(ctx)
}

As a framework mechanism, I think it's always worth to make everything explicitly clear, instead of using implicit behaviors.

[wxiaoguang]

I have the same idea with "Add a lifetime to the cache context, like 5 minutes" , while I think 5-min is still too long. IMO 5sec is long enough.

[lunny]

Is it correct to use cache for all these background tasks?

TBH, I'm not entirely sure. It's correct to do this only if all queue handling tasks should take only a short time(less than 5 minutes), so it's fine when the cache gets out-of-sync. But I can't prove it or disprove it.

... add a cache.WithNoCacheContext() ...

Yeah, I believe it can resolve the warning logs. But there will be three ways to use a context:

* `ctx = cache.WithCacheContext(ctx)`

* `ctx = cache.WithNoCacheContext(ctx)`

* just `ctx`

What's the difference? And what if cache.WithCacheContext(cache.WithNoCacheContext(ctx)) or cache.WithNoCacheContext(cache.WithCacheContext(ctx))? 😂

Of course, we can define those cases, but I think it would be quite confusing.

So I think the point is not how to avoid warning logs, but that the context cache is designed for "short life cycle tasks", like handling a HTTP request, and (maybe) handling a task from queue, but developers could use it for "long life cycle tasks" by accident. IMO, five minutes is long enough.

There are some ideas for it:

* Remove the warnlogs logs, `ctx` means "no cache", so we don't need `cache.WithNoCacheContext`. And use `cache.WithCacheContext` for handling HTTP requests only. Then add the cache for tasks which we are sure are worth it and have a short life cycle.

* Refactor `queue.HandlerFunc` from `type HandlerFunc func(...Data) (unhandled []Data)` to `type HandlerFunc func(ctx context.Context, ...Data) (unhandled []Data)`, so it will be easy to enable or disable cache context for the handlers before invoking them.

* Add a lifetime to the cache context, like 5 minutes. If the cache data is still being read after 5 minutes after it was created, skip using the cache data and print warning logs, and it's also a good idea to log stack trace to identify where it is being misused.

Or we can just remove the warning log?

[wxiaoguang]

Or we can just remove the warning log?

I do not think so, that log really helps us to think about these problems.

I always prefer to make framework mechanisms have stable/explicit behaviors, to avoid surprises and bugs. Many legacy bugs/problems in Gitea are caused by unclear/misued behaviors.

But if most people like the implicit behaviors and think that no need to distinguish a context with/without cache support, I do not have strong objection at the moment - until some new bug comes.