From 3461e6bce45adfc5f92afaef904ebd9ce18a7752 Mon Sep 17 00:00:00 2001 From: dualfroz Date: Wed, 2 Sep 2026 11:03:59 +0200 Subject: [PATCH] fix: stop prepared statement cache cleanup goroutine on Close The expirable LRU cache backing the prepared statement store starts a background deleteExpired goroutine when it is created with a non-zero TTL. Its Close method was commented out, so the goroutine kept running for the lifetime of the process and leaked every time a PreparedStmtDB was discarded (for example when a *sql.DB is closed and reopened), causing steady goroutine and memory growth in long-running services. Expose LRU.Close, add Close to the stmt_store.Store interface and its lruStore implementation, and call it from PreparedStmtDB.Close so the background cleanup goroutine is stopped together with the cached statements. Close is idempotent and safe for concurrent use. Refs #7788 --- internal/lru/lru.go | 29 +++++++++++--------- internal/stmt_store/stmt_store.go | 10 +++++++ prepare_stmt.go | 5 +++- tests/lru_test.go | 44 +++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 14 deletions(-) diff --git a/internal/lru/lru.go b/internal/lru/lru.go index fbb3b40..9222de4 100644 --- a/internal/lru/lru.go +++ b/internal/lru/lru.go @@ -74,8 +74,9 @@ func NewLRU[K comparable, V any](size int, onEvict EvictCallback[K, V], ttl time // enable deleteExpired() running in separate goroutine for cache with non-zero TTL // - // Important: done channel is never closed, so deleteExpired() goroutine will never exit, - // it's decided to add functionality to close it in the version later than v2. + // The goroutine runs until Close is called, which closes the done channel and + // signals it to exit. Callers that discard the cache should call Close to avoid + // leaking the goroutine. if res.ttl != noEvictionTTL { go func(done <-chan struct{}) { ticker := time.NewTicker(res.ttl / numBuckets) @@ -290,17 +291,19 @@ func (c *LRU[K, V]) Resize(size int) (evicted int) { return diff } -// Close destroys cleanup goroutine. To clean up the cache, run Purge() before Close(). -// func (c *LRU[K, V]) Close() { -// c.mu.Lock() -// defer c.mu.Unlock() -// select { -// case <-c.done: -// return -// default: -// } -// close(c.done) -// } +// Close stops the background cleanup goroutine and releases its resources. +// Close is safe to call multiple times and from multiple goroutines. +// To also drop cached entries, call Purge before Close. +func (c *LRU[K, V]) Close() { + c.mu.Lock() + defer c.mu.Unlock() + select { + case <-c.done: + return + default: + } + close(c.done) +} // removeOldest removes the oldest item from the cache. Has to be called with lock! func (c *LRU[K, V]) removeOldest() { diff --git a/internal/stmt_store/stmt_store.go b/internal/stmt_store/stmt_store.go index 6f13ebb..b593e2b 100644 --- a/internal/stmt_store/stmt_store.go +++ b/internal/stmt_store/stmt_store.go @@ -68,6 +68,11 @@ type Store interface { // Parameters: // key: The key associated with the Stmt object to be deleted. Delete(key string) + + // Close releases resources held by the store, stopping any background + // maintenance goroutines started for cache expiration. It is safe to call + // Close multiple times. The store should not be used after Close is called. + Close() } // defaultMaxSize defines the default maximum capacity of the cache. @@ -137,6 +142,11 @@ func (s *lruStore) Delete(key string) { s.lru.Remove(key) } +// Close stops the underlying LRU cache's background expiration goroutine. +func (s *lruStore) Close() { + s.lru.Close() +} + type ConnPool interface { PrepareContext(ctx context.Context, query string) (*sql.Stmt, error) } diff --git a/prepare_stmt.go b/prepare_stmt.go index 799df5b..762fc99 100644 --- a/prepare_stmt.go +++ b/prepare_stmt.go @@ -48,7 +48,9 @@ func (db *PreparedStmtDB) GetDBConn() (*sql.DB, error) { return nil, ErrInvalidDB } -// Close closes all prepared statements in the store +// Close closes all prepared statements in the store and stops the store's +// background maintenance goroutine to avoid leaking it for the lifetime of the +// process. func (db *PreparedStmtDB) Close() { db.Mux.Lock() defer db.Mux.Unlock() @@ -56,6 +58,7 @@ func (db *PreparedStmtDB) Close() { for _, key := range db.Stmts.Keys() { db.Stmts.Delete(key) } + db.Stmts.Close() } // Reset Deprecated use Close instead diff --git a/tests/lru_test.go b/tests/lru_test.go index 4be94ae..d94905c 100644 --- a/tests/lru_test.go +++ b/tests/lru_test.go @@ -6,6 +6,7 @@ import ( "math" "math/big" "reflect" + "runtime" "sync" "testing" "time" @@ -13,6 +14,49 @@ import ( "gorm.io/gorm/internal/lru" ) +// TestLRU_Close_StopsBackgroundGoroutine verifies that Close terminates the +// background deleteExpired goroutine started for a cache with a non-zero TTL. +// Without Close, every such cache leaks one goroutine for the lifetime of the +// process (see go-gorm/gorm issue #7788). +func TestLRU_Close_StopsBackgroundGoroutine(t *testing.T) { + const n = 100 + + runtime.GC() + time.Sleep(50 * time.Millisecond) + baseline := runtime.NumGoroutine() + + caches := make([]*lru.LRU[string, int], 0, n) + for i := 0; i < n; i++ { + // A non-zero TTL starts a background deleteExpired goroutine per cache. + caches = append(caches, lru.NewLRU[string, int](16, nil, time.Minute)) + } + + // Sanity check: the background goroutines are running. + if got := runtime.NumGoroutine(); got < baseline+n/2 { + t.Fatalf("expected background goroutines to start: got %d, baseline %d", got, baseline) + } + + for _, c := range caches { + c.Close() + } + caches = nil + + // After Close the background goroutines must terminate and the goroutine + // count must return close to the baseline. + deadline := time.Now().Add(2 * time.Second) + for { + runtime.GC() + got := runtime.NumGoroutine() + if got <= baseline+n/10 { + return + } + if time.Now().After(deadline) { + t.Fatalf("background goroutines leaked after Close: got %d, baseline %d", got, baseline) + } + time.Sleep(20 * time.Millisecond) + } +} + func TestLRU_Add_ExistingKey_UpdatesValueAndExpiresAt(t *testing.T) { lru := lru.NewLRU[string, int](10, nil, time.Hour) lru.Add("key1", 1)