From 85ddebb9c0ee1d9c3a75fcaae269b630e84d0e0a Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 6 Mar 2023 15:46:22 +0800 Subject: [PATCH 1/8] feat: lifetime for cache context --- modules/cache/context.go | 37 ++++++++++++++++++++++++++++++----- modules/cache/context_test.go | 13 +++++++++++- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/modules/cache/context.go b/modules/cache/context.go index f741a87445383..943a72c3e13c8 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,9 +15,10 @@ 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 + ctx context.Context + data map[any]map[any]any + lock sync.RWMutex + created time.Time } func (cc *cacheContext) Get(tp, key any) any { @@ -46,17 +48,33 @@ func (cc *cacheContext) Delete(tp, key any) { delete(cc.data[tp], key) } +// 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{}{} func WithCacheContext(ctx context.Context) context.Context { return context.WithValue(ctx, cacheContextKey, &cacheContext{ - ctx: ctx, - data: make(map[any]map[any]any), + ctx: ctx, + data: make(map[any]map[any]any), + created: timeNow(), }) } func GetContextData(ctx context.Context, tp, key any) any { if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { + if c.Expired() { + log.Warn("cache context is expired: %v", c) + return nil + } return c.Get(tp, key) } log.Warn("cannot get cache context when getting data: %v", ctx) @@ -65,6 +83,10 @@ func GetContextData(ctx context.Context, tp, key any) any { func SetContextData(ctx context.Context, tp, key, value any) { if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { + if c.Expired() { + log.Warn("cache context is expired: %v", c) + return + } c.Put(tp, key, value) return } @@ -73,8 +95,13 @@ func SetContextData(ctx context.Context, tp, key, value any) { func RemoveContextData(ctx context.Context, tp, key any) { if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { + if c.Expired() { + log.Warn("cache context is expired: %v", c) + return + } c.Delete(tp, key) } + log.Warn("cannot get cache context when removing data: %v", ctx) } // GetWithContextCache returns the cache value of the given key in the given context. diff --git a/modules/cache/context_test.go b/modules/cache/context_test.go index 77e3ecad2ca70..093830a10878d 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,14 @@ 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) } From e8732e9744eef53406ccbbd22440c31bc44d90b1 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 6 Mar 2023 15:48:54 +0800 Subject: [PATCH 2/8] chore: remove ctx field --- modules/cache/context.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/modules/cache/context.go b/modules/cache/context.go index 943a72c3e13c8..9686c5656e593 100644 --- a/modules/cache/context.go +++ b/modules/cache/context.go @@ -15,7 +15,6 @@ 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 created time.Time @@ -63,7 +62,6 @@ var cacheContextKey = struct{}{} func WithCacheContext(ctx context.Context) context.Context { return context.WithValue(ctx, cacheContextKey, &cacheContext{ - ctx: ctx, data: make(map[any]map[any]any), created: timeNow(), }) From ec2c80e0ea12f8d54bed398045905e9a497026bc Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 6 Mar 2023 16:18:35 +0800 Subject: [PATCH 3/8] feat: WithNoCacheContext --- modules/cache/context.go | 29 +++++++++++++++++++++++++++++ modules/cache/context_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/modules/cache/context.go b/modules/cache/context.go index 9686c5656e593..f807f3c980203 100644 --- a/modules/cache/context.go +++ b/modules/cache/context.go @@ -20,6 +20,9 @@ type cacheContext struct { created time.Time } +// noCacheContext is a context that should discard cache data in context +type noCacheContext struct{} + func (cc *cacheContext) Get(tp, key any) any { cc.lock.RLock() defer cc.lock.RUnlock() @@ -67,14 +70,26 @@ func WithCacheContext(ctx context.Context) context.Context { }) } +func WithNoCacheContext(ctx context.Context) context.Context { + return context.WithValue(ctx, cacheContextKey, noCacheContext{}) +} + 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: %v", c) return nil } return c.Get(tp, key) } + if _, ok := ctx.Value(cacheContextKey).(noCacheContext); ok { + return nil + } + // The warning means that an original context is treated as a cache context, + // it can be resolved with WithNoCacheContext(ctx) or WithCacheContext(ctx). + // If you are not sure which one should be picked, it's always a safe way to use WithNoCacheContext(ctx). log.Warn("cannot get cache context when getting data: %v", ctx) return nil } @@ -82,12 +97,20 @@ func GetContextData(ctx context.Context, tp, key any) any { 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: %v", c) return } c.Put(tp, key, value) return } + if _, ok := ctx.Value(cacheContextKey).(noCacheContext); ok { + return + } + // The warning means that an original context is treated as a cache context, + // it can be resolved with WithNoCacheContext(ctx) or WithCacheContext(ctx). + // If you are not sure which one should be picked, it's always a safe way to use WithNoCacheContext(ctx). log.Warn("cannot get cache context when setting data: %v", ctx) } @@ -99,6 +122,12 @@ func RemoveContextData(ctx context.Context, tp, key any) { } c.Delete(tp, key) } + if _, ok := ctx.Value(cacheContextKey).(noCacheContext); ok { + return + } + // The warning means that an original context is treated as a cache context, + // it can be resolved with WithNoCacheContext(ctx) or WithCacheContext(ctx). + // If you are not sure which one should be picked, it's always a safe way to use WithNoCacheContext(ctx). log.Warn("cannot get cache context when removing data: %v", ctx) } diff --git a/modules/cache/context_test.go b/modules/cache/context_test.go index 093830a10878d..5315547865e19 100644 --- a/modules/cache/context_test.go +++ b/modules/cache/context_test.go @@ -50,3 +50,29 @@ func TestWithCacheContext(t *testing.T) { 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 +} From 10692a0f2360bb5a419cddab3cf5d5c76ada5db5 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 6 Mar 2023 16:35:25 +0800 Subject: [PATCH 4/8] fix: remove noCacheContext --- modules/cache/context.go | 34 ++++++---------------------------- 1 file changed, 6 insertions(+), 28 deletions(-) diff --git a/modules/cache/context.go b/modules/cache/context.go index f807f3c980203..e5a6d504fed88 100644 --- a/modules/cache/context.go +++ b/modules/cache/context.go @@ -20,9 +20,6 @@ type cacheContext struct { created time.Time } -// noCacheContext is a context that should discard cache data in context -type noCacheContext struct{} - func (cc *cacheContext) Get(tp, key any) any { cc.lock.RLock() defer cc.lock.RUnlock() @@ -71,7 +68,7 @@ func WithCacheContext(ctx context.Context) context.Context { } func WithNoCacheContext(ctx context.Context) context.Context { - return context.WithValue(ctx, cacheContextKey, noCacheContext{}) + return context.WithValue(ctx, cacheContextKey, nil) } func GetContextData(ctx context.Context, tp, key any) any { @@ -79,18 +76,11 @@ func GetContextData(ctx context.Context, tp, key any) any { 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: %v", c) + log.Warn("cache context is expired, may be misused for long-life tasks: %v", c) return nil } return c.Get(tp, key) } - if _, ok := ctx.Value(cacheContextKey).(noCacheContext); ok { - return nil - } - // The warning means that an original context is treated as a cache context, - // it can be resolved with WithNoCacheContext(ctx) or WithCacheContext(ctx). - // If you are not sure which one should be picked, it's always a safe way to use WithNoCacheContext(ctx). - log.Warn("cannot get cache context when getting data: %v", ctx) return nil } @@ -99,36 +89,24 @@ func SetContextData(ctx context.Context, tp, key, value any) { 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: %v", c) + log.Warn("cache context is expired, may be misused for long-life tasks: %v", c) return } c.Put(tp, key, value) return } - if _, ok := ctx.Value(cacheContextKey).(noCacheContext); ok { - return - } - // The warning means that an original context is treated as a cache context, - // it can be resolved with WithNoCacheContext(ctx) or WithCacheContext(ctx). - // If you are not sure which one should be picked, it's always a safe way to use WithNoCacheContext(ctx). - 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() { - log.Warn("cache context is expired: %v", c) + // 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) } - if _, ok := ctx.Value(cacheContextKey).(noCacheContext); ok { - return - } - // The warning means that an original context is treated as a cache context, - // it can be resolved with WithNoCacheContext(ctx) or WithCacheContext(ctx). - // If you are not sure which one should be picked, it's always a safe way to use WithNoCacheContext(ctx). - log.Warn("cannot get cache context when removing data: %v", ctx) } // GetWithContextCache returns the cache value of the given key in the given context. From aa1dc7e34d4c7c0db8bdf782d60a3b846f360401 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 6 Mar 2023 17:20:39 +0800 Subject: [PATCH 5/8] feat: redesign cache context --- modules/cache/context.go | 42 ++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/modules/cache/context.go b/modules/cache/context.go index e5a6d504fed88..5e70211d9c8ec 100644 --- a/modules/cache/context.go +++ b/modules/cache/context.go @@ -18,35 +18,44 @@ type cacheContext struct { 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 + } + + d := cc.data[tp] + if d == nil { + d = make(map[any]any) + cc.data[tp] = d } - cc.data[tp][key] = value + 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 +} + // 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. @@ -61,6 +70,12 @@ func (cc *cacheContext) Expired() bool { var cacheContextKey = struct{}{} func WithCacheContext(ctx context.Context) context.Context { + if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { + if !c.discard { + // reuse parent context + return ctx + } + } return context.WithValue(ctx, cacheContextKey, &cacheContext{ data: make(map[any]map[any]any), created: timeNow(), @@ -68,7 +83,14 @@ func WithCacheContext(ctx context.Context) context.Context { } func WithNoCacheContext(ctx context.Context) context.Context { - return context.WithValue(ctx, cacheContextKey, nil) + 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 { From c2a907f4636ebd65fc364946315861326f8432d5 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 6 Mar 2023 17:21:22 +0800 Subject: [PATCH 6/8] fix: remove usage --- services/repository/push.go | 1 - 1 file changed, 1 deletion(-) diff --git a/services/repository/push.go b/services/repository/push.go index 355c2878113fd..cdf030396fcf6 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 { From 1e467271899a39abcc01d0efe204f2641ece9050 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Mon, 6 Mar 2023 21:29:55 +0800 Subject: [PATCH 7/8] docs: add comments --- modules/cache/context.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/modules/cache/context.go b/modules/cache/context.go index 5e70211d9c8ec..a8868ecc46e4a 100644 --- a/modules/cache/context.go +++ b/modules/cache/context.go @@ -69,6 +69,33 @@ func (cc *cacheContext) Expired() bool { 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.discard { From bda1bbfab6a7fcb2b59d8bd9e805bc4137d559ae Mon Sep 17 00:00:00 2001 From: Jason Song Date: Tue, 7 Mar 2023 18:56:36 +0800 Subject: [PATCH 8/8] fix: isDiscard --- modules/cache/context.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/modules/cache/context.go b/modules/cache/context.go index a8868ecc46e4a..62bbf5dcba84a 100644 --- a/modules/cache/context.go +++ b/modules/cache/context.go @@ -56,6 +56,12 @@ func (cc *cacheContext) Discard() { 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. @@ -98,7 +104,7 @@ So: func WithCacheContext(ctx context.Context) context.Context { if c, ok := ctx.Value(cacheContextKey).(*cacheContext); ok { - if !c.discard { + if !c.isDiscard() { // reuse parent context return ctx }