Potential unused condition check in internal/xds/server/runner/runner.go

[muyuan0]

Description:

Describe the issue.

There is a nil check in the

gateway/internal/xds/server/runner/runner.go

Lines 137 to 139 in 9036533

	if r.cache == nil {
	r.Logger.Error(err, "failed to init snapshot cache")
	} else {

However, r.cache is initialized in

gateway/internal/xds/server/runner/runner.go

Line 80 in 9036533

r.cache = cache.NewSnapshotCache(false, r.Logger)

and NewSnapshotCache never returns a nil from it:

gateway/internal/xds/cache/snapshotcache.go

Lines 108 to 117 in 9036533

	func NewSnapshotCache(ads bool, logger logr.Logger) SnapshotCacheWithCallbacks {
	// Set up the nasty wrapper hack.
	wrappedLogger := NewLogrWrapper(logger)
	return &snapshotCache{
	SnapshotCache: cachev3.NewSnapshotCache(ads, &Hash, wrappedLogger),
	log: wrappedLogger,
	lastSnapshot: make(snapshotMap),
	streamIDNodeInfo: make(nodeInfoMap),
	}
	}

IMHO, the logger message is suspicious because the code around doesn't init snapshot cache. It could be a remnant of a previous refactor.

[optional Relevant Links:]

Any extra documentation required to understand the issue.

[arkodg]

I believe this was added because there was a case where watchable messages were being received before the cache was created
#745

[muyuan0]

Thanks for pointing that out. I take a look at the PR, before #745 the subscribeAndTranslate and cache.NewSnapshotCache is run concurrently, so it is possible to have r.cache == nil if the cache.NewSnapshotCache is not called yet. After that, subscribeAndTranslate always run after cache.NewSnapshotCache so r.cache won't be nil at this time.

[arkodg]

Thanks for pointing that out. I take a look at the PR, before #745 the subscribeAndTranslate and cache.NewSnapshotCache is run concurrently, so it is possible to have r.cache == nil if the cache.NewSnapshotCache is not called yet. After that, subscribeAndTranslate always run after cache.NewSnapshotCache so r.cache won't be nil at this time.

thanks for the analysis, @Xunzhuo do you see any issue with this check being removed ?

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days.