From 1960ad5c90df65100488b64e7047d1ba3096c11c Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 9 Mar 2023 01:57:05 +0800 Subject: [PATCH] Improve cache context (#23330) Related to: #22294 #23186 #23054 Replace: #23218 Some discussion is in the comments of #23218. Highlights: - Add Expiration for cache context. If a cache context has been used for more than 10s, the cache data will be ignored, and warning logs will be printed. - Add `discard` field to `cacheContext`, a `cacheContext` with `discard` true will drop all cached data and won't store any new one. - Introduce `WithNoCacheContext`, if one wants to run long-life tasks, but the parent context is a cache context, `WithNoCacheContext(perentCtx)` will discard the cache data, so it will be safe to keep the context for a long time. - It will be fine to treat an original context as a cache context, like `GetContextData(context.Backgraud())`, no warning logs will be printed. Some cases about nesting: When: - *A*, *B* or *C* means a cache context. - ~*A*~, ~*B*~ or ~*C*~ means a discard cache context. - `ctx` means `context.Backgrand()` - *A(ctx)* means a cache context with `ctx` as the parent context. - *B(A(ctx))* means a cache context with `A(ctx)` as the parent context. - `With` means `WithCacheContext` - `WithNo` means `WithNoCacheContext` So: - `With(ctx)` -> *A(ctx)* - `With(With(ctx))` -> *A(ctx)*, not *B(A(ctx))* - `With(With(With(ctx)))` -> *A(ctx)*, not *C(B(A(ctx)))* - `WithNo(ctx)` -> *ctx*, not *~A~(ctx)* - `WithNo(With(ctx))` -> *~A~(ctx)* - `WithNo(WithNo(With(ctx)))` -> *~A~(ctx)*, not *~B~(~A~(ctx))* - `With(WithNo(With(ctx)))` -> *B(~A~(ctx))* - `WithNo(With(WithNo(With(ctx))))` -> *~B~(~A~(ctx))* - `With(WithNo(With(WithNo(With(ctx)))))` -> *C(~B~(~A~(ctx)))* --- modules/cache/context.go | 119 +++++++++++++++++++++++++++++----- modules/cache/context_test.go | 39 ++++++++++- services/repository/push.go | 1 - 3 files changed, 141 insertions(+), 18 deletions(-) diff --git a/modules/cache/context.go b/modules/cache/context.go index f741a8744..62bbf5dcb 100644 --- a/modules/cache/context.go +++ b/modules/cache/context.go @@ -6,6 +6,7 @@ package cache import ( "context" "sync" + "time" "code.gitea.io/gitea/modules/log" ) @@ -14,65 +15,151 @@ import ( // This is useful for caching data that is expensive to calculate and is likely to be // used multiple times in a request. type cacheContext struct { - ctx context.Context - data map[any]map[any]any - lock sync.RWMutex + data map[any]map[any]any + lock sync.RWMutex + created time.Time + discard bool } func (cc *cacheContext) Get(tp, key any) any { cc.lock.RLock() defer cc.lock.RUnlock() - if cc.data[tp] == nil { - return nil - } return cc.data[tp][key] } func (cc *cacheContext) Put(tp, key, value any) { cc.lock.Lock() defer cc.lock.Unlock() - if cc.data[tp] == nil { - cc.data[tp] = make(map[any]any) + + if cc.discard { + return } - cc.data[tp][key] = value + + d := cc.data[tp] + if d == nil { + d = make(map[any]any) + cc.data[tp] = d + } + d[key] = value } func (cc *cacheContext) Delete(tp, key any) { cc.lock.Lock() defer cc.lock.Unlock() - if cc.data[tp] == nil { - return - } delete(cc.data[tp], key) } +func (cc *cacheContext) Discard() { + cc.lock.Lock() + defer cc.lock.Unlock() + cc.data = nil + cc.discard = true +} + +func (cc *cacheContext) isDiscard() bool { + cc.lock.RLock() + defer cc.lock.RUnlock() + return cc.discard +} + +// cacheContextLifetime is the max lifetime of cacheContext. +// Since cacheContext is used to cache data in a request level context, 10s is enough. +// If a cacheContext is used more than 10s, it's probably misuse. +const cacheContextLifetime = 10 * time.Second + +var timeNow = time.Now + +func (cc *cacheContext) Expired() bool { + return timeNow().Sub(cc.created) > cacheContextLifetime +} + var cacheContextKey = struct{}{} +/* +Since there are both WithCacheContext and WithNoCacheContext, +it may be confusing when there is nesting. + +Some cases to explain the design: + +When: +- A, B or C means a cache context. +- A', B' or C' means a discard cache context. +- ctx means context.Backgrand(). +- A(ctx) means a cache context with ctx as the parent context. +- B(A(ctx)) means a cache context with A(ctx) as the parent context. +- With is alias of WithCacheContext. +- WithNo is alias of WithNoCacheContext. + +So: +- With(ctx) -> A(ctx) +- With(With(ctx)) -> A(ctx), not B(A(ctx)), always reuse parent cache context if possible. +- With(With(With(ctx))) -> A(ctx), not C(B(A(ctx))), ditto. +- WithNo(ctx) -> ctx, not A'(ctx), don't create new cache context if we don't have to. +- WithNo(With(ctx)) -> A'(ctx) +- WithNo(WithNo(With(ctx))) -> A'(ctx), not B'(A'(ctx)), don't create new cache context if we don't have to. +- With(WithNo(With(ctx))) -> B(A'(ctx)), not A(ctx), never reuse a discard cache context. +- WithNo(With(WithNo(With(ctx)))) -> B'(A'(ctx)) +- With(WithNo(With(WithNo(With(ctx))))) -> C(B'(A'(ctx))), so there's always only one not-discard cache context. +*/ + func WithCacheContext(ctx context.Context) context.Context { + if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { + if !c.isDiscard() { + // reuse parent context + return ctx + } + } return context.WithValue(ctx, cacheContextKey, &cacheContext{ - ctx: ctx, - data: make(map[any]map[any]any), + data: make(map[any]map[any]any), + created: timeNow(), }) } +func WithNoCacheContext(ctx context.Context) context.Context { + if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { + // The caller want to run long-life tasks, but the parent context is a cache context. + // So we should disable and clean the cache data, or it will be kept in memory for a long time. + c.Discard() + return ctx + } + + return ctx +} + func GetContextData(ctx context.Context, tp, key any) any { if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { + if c.Expired() { + // The warning means that the cache context is misused for long-life task, + // it can be resolved with WithNoCacheContext(ctx). + log.Warn("cache context is expired, may be misused for long-life tasks: %v", c) + return nil + } return c.Get(tp, key) } - log.Warn("cannot get cache context when getting data: %v", ctx) return nil } func SetContextData(ctx context.Context, tp, key, value any) { if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { + if c.Expired() { + // The warning means that the cache context is misused for long-life task, + // it can be resolved with WithNoCacheContext(ctx). + log.Warn("cache context is expired, may be misused for long-life tasks: %v", c) + return + } c.Put(tp, key, value) return } - log.Warn("cannot get cache context when setting data: %v", ctx) } func RemoveContextData(ctx context.Context, tp, key any) { if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { + if c.Expired() { + // The warning means that the cache context is misused for long-life task, + // it can be resolved with WithNoCacheContext(ctx). + log.Warn("cache context is expired, may be misused for long-life tasks: %v", c) + return + } c.Delete(tp, key) } } diff --git a/modules/cache/context_test.go b/modules/cache/context_test.go index 77e3ecad2..531554786 100644 --- a/modules/cache/context_test.go +++ b/modules/cache/context_test.go @@ -6,6 +6,7 @@ package cache import ( "context" "testing" + "time" "github.com/stretchr/testify/assert" ) @@ -25,7 +26,7 @@ func TestWithCacheContext(t *testing.T) { assert.EqualValues(t, 1, v.(int)) RemoveContextData(ctx, field, "my_config1") - RemoveContextData(ctx, field, "my_config2") // remove an non-exist key + RemoveContextData(ctx, field, "my_config2") // remove a non-exist key v = GetContextData(ctx, field, "my_config1") assert.Nil(t, v) @@ -38,4 +39,40 @@ func TestWithCacheContext(t *testing.T) { v = GetContextData(ctx, field, "my_config1") assert.EqualValues(t, 1, v) + + now := timeNow + defer func() { + timeNow = now + }() + timeNow = func() time.Time { + return now().Add(10 * time.Second) + } + v = GetContextData(ctx, field, "my_config1") + assert.Nil(t, v) +} + +func TestWithNoCacheContext(t *testing.T) { + ctx := context.Background() + + const field = "system_setting" + + v := GetContextData(ctx, field, "my_config1") + assert.Nil(t, v) + SetContextData(ctx, field, "my_config1", 1) + v = GetContextData(ctx, field, "my_config1") + assert.Nil(t, v) // still no cache + + ctx = WithCacheContext(ctx) + v = GetContextData(ctx, field, "my_config1") + assert.Nil(t, v) + SetContextData(ctx, field, "my_config1", 1) + v = GetContextData(ctx, field, "my_config1") + assert.NotNil(t, v) + + ctx = WithNoCacheContext(ctx) + v = GetContextData(ctx, field, "my_config1") + assert.Nil(t, v) + SetContextData(ctx, field, "my_config1", 1) + v = GetContextData(ctx, field, "my_config1") + assert.Nil(t, v) // still no cache } diff --git a/services/repository/push.go b/services/repository/push.go index 355c28781..cdf030396 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -80,7 +80,6 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("PushUpdates: %s/%s", optsList[0].RepoUserName, optsList[0].RepoName)) defer finished() - ctx = cache.WithCacheContext(ctx) repo, err := repo_model.GetRepositoryByOwnerAndName(ctx, optsList[0].RepoUserName, optsList[0].RepoName) if err != nil {