From 47574590198ddd0f254f47c028333038d9f33fc6 Mon Sep 17 00:00:00 2001 From: Guillaume Jacquet Date: Mon, 20 May 2024 06:28:57 -0400 Subject: [PATCH] Fix scaler leak during cache refresh (#5807) Signed-off-by: Guillaume Jacquet Signed-off-by: Jorge Turrado --- CHANGELOG.md | 1 + pkg/scaling/scale_handler.go | 23 ++++++++++------------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6615f67ed73..6909a16927f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,6 +93,7 @@ Here is an overview of all new **experimental** features: - **MongoDB Scaler**: MongoDB url parses correctly `+srv` scheme ([#5760](https://github.com/kedacore/keda/issues/5760)) - **New Relic Scaler**: Fix CVE-2024-6104 in github.com/hashicorp/go-retryablehttp ([#5944](https://github.com/kedacore/keda/issues/5944)) - **ScaledJob**: Fix ScaledJob ignores failing trigger(s) error ([#5922](https://github.com/kedacore/keda/issues/5922)) +- **General**: Scalers are properly closed after being refreshed ([#5806](https://github.com/kedacore/keda/issues/5806)) - **MongoDB Scaler**: MongoDB url parses correctly `+srv` scheme ([#5760](https://github.com/kedacore/keda/issues/5760)) ### Deprecations diff --git a/pkg/scaling/scale_handler.go b/pkg/scaling/scale_handler.go index 8655cf50dc9..5a955e48e66 100644 --- a/pkg/scaling/scale_handler.go +++ b/pkg/scaling/scale_handler.go @@ -293,7 +293,7 @@ func (h *scaleHandler) getScalersCacheForScaledObject(ctx context.Context, scale // performGetScalersCache returns cache for input scalableObject, it is common code used by GetScalersCache() and getScalersCacheForScaledObject() methods func (h *scaleHandler) performGetScalersCache(ctx context.Context, key string, scalableObject interface{}, scalableObjectGeneration *int64, scalableObjectKind, scalableObjectNamespace, scalableObjectName string) (*cache.ScalersCache, error) { h.scalerCachesLock.RLock() - regenerateCache := false + if cache, ok := h.scalerCaches[key]; ok { // generation was specified -> let's include it in the check as well if scalableObjectGeneration != nil { @@ -301,15 +301,12 @@ func (h *scaleHandler) performGetScalersCache(ctx context.Context, key string, s h.scalerCachesLock.RUnlock() return cache, nil } - // object was found in cache, but the generation is not correct, - // we'll need to close scalers in the cache and - // proceed further to recreate the cache - regenerateCache = false } else { h.scalerCachesLock.RUnlock() return cache, nil } } + h.scalerCachesLock.RUnlock() if scalableObject == nil { @@ -379,17 +376,17 @@ func (h *scaleHandler) performGetScalersCache(ctx context.Context, key string, s default: } - // Scalers Close() could be impacted by timeouts, blocking the mutex - // until the timeout happens. Instead of locking the mutex, we take - // the old cache item and we close it in another goroutine, not locking - // the cache: https://github.com/kedacore/keda/issues/5083 - if regenerateCache { - oldCache := h.scalerCaches[key] + h.scalerCachesLock.Lock() + defer h.scalerCachesLock.Unlock() + + if oldCache, ok := h.scalerCaches[key]; ok { + // Scalers Close() could be impacted by timeouts, blocking the mutex + // until the timeout happens. Instead of locking the mutex, we take + // the old cache item and we close it in another goroutine, not locking + // the cache: https://github.com/kedacore/keda/issues/5083 go oldCache.Close(ctx) } - h.scalerCachesLock.Lock() - defer h.scalerCachesLock.Unlock() h.scalerCaches[key] = newCache return h.scalerCaches[key], nil }