Skip to content
Jump to bottom

Automated Test: asset-loading-optimized #331

Closed
wants to merge 1 commit into from
+13 −3
Closed

Automated Test: asset-loading-optimized #331

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 13 additions & 3 deletions pkg/api/webassets/webassets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"net/http"
"os"
"path/filepath"
"sync"

"github.com/grafana/grafana/pkg/api/dtos"
"github.com/grafana/grafana/pkg/services/licensing"
Expand All @@ -31,12 +32,21 @@ type EntryPointInfo struct {
} `json:"assets,omitempty"`
}

var entryPointAssetsCache *dtos.EntryPointAssets = nil
var (
entryPointAssetsCacheMu sync.RWMutex // guard entryPointAssetsCache
entryPointAssetsCache *dtos.EntryPointAssets // TODO: get rid of global state
)

func GetWebAssets(ctx context.Context, cfg *setting.Cfg, license licensing.Licensing) (*dtos.EntryPointAssets, error) {
if cfg.Env != setting.Dev && entryPointAssetsCache != nil {
return entryPointAssetsCache, nil
entryPointAssetsCacheMu.RLock()
ret := entryPointAssetsCache
entryPointAssetsCacheMu.RUnlock()

if cfg.Env != setting.Dev && ret != nil {
return ret, nil
}
entryPointAssetsCacheMu.Lock()
defer entryPointAssetsCacheMu.Unlock()

Comment on lines 40 to 50
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing double-check after acquiring write lock causes redundant asset loading.

The current implementation has a TOCTOU (time-of-check to time-of-use) issue. Between releasing the read lock (line 43) and acquiring the write lock (line 48), another goroutine may have already populated the cache. Without a re-check, multiple concurrent requests at startup will all perform redundant file/CDN reads.

🔧 Proposed fix: Add double-check after acquiring write lock 
 func GetWebAssets(ctx context.Context, cfg *setting.Cfg, license licensing.Licensing) (*dtos.EntryPointAssets, error) {
 	entryPointAssetsCacheMu.RLock()
 	ret := entryPointAssetsCache
 	entryPointAssetsCacheMu.RUnlock()

 	if cfg.Env != setting.Dev && ret != nil {
 		return ret, nil
 	}
 	entryPointAssetsCacheMu.Lock()
 	defer entryPointAssetsCacheMu.Unlock()

+	// Double-check after acquiring write lock
+	if cfg.Env != setting.Dev && entryPointAssetsCache != nil {
+		return entryPointAssetsCache, nil
+	}
+
 	var err error
 	var result *dtos.EntryPointAssets
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func GetWebAssets(ctx context.Context, cfg *setting.Cfg, license licensing.Licensing) (*dtos.EntryPointAssets, error) {
if cfg.Env != setting.Dev && entryPointAssetsCache != nil {
return entryPointAssetsCache, nil
entryPointAssetsCacheMu.RLock()
ret := entryPointAssetsCache
entryPointAssetsCacheMu.RUnlock()
if cfg.Env != setting.Dev && ret != nil {
return ret, nil
}
entryPointAssetsCacheMu.Lock()
defer entryPointAssetsCacheMu.Unlock()
func GetWebAssets(ctx context.Context, cfg *setting.Cfg, license licensing.Licensing) (*dtos.EntryPointAssets, error) {
entryPointAssetsCacheMu.RLock()
ret := entryPointAssetsCache
entryPointAssetsCacheMu.RUnlock()
if cfg.Env != setting.Dev && ret != nil {
return ret, nil
}
entryPointAssetsCacheMu.Lock()
defer entryPointAssetsCacheMu.Unlock()
// Double-check after acquiring write lock
if cfg.Env != setting.Dev && entryPointAssetsCache != nil {
return entryPointAssetsCache, nil
}
🤖 Prompt for AI Agents 
In `@pkg/api/webassets/webassets.go` around lines 40 - 50, GetWebAssets has a
TOCTOU: after releasing the read lock and before loading assets you must
re-check the cache under the write lock to avoid redundant loads; after
entryPointAssetsCacheMu.Lock() (inside GetWebAssets) add a second check of
entryPointAssetsCache (and return it if non-nil) before performing the expensive
asset loading logic, keeping the existing cfg.Env / setting.Dev behavior and
using the same entryPointAssetsCache and entryPointAssetsCacheMu symbols.

var err error
var result *dtos.EntryPointAssets
Expand Down