SuppressedLoader should allow setting group

[markandrus]

SuppressedLoader looks useful! Unfortunately, group is private. So if I just create a SuppressedLoader from an existing Loader and try to use it, I get an "invalid memory address or nil pointer dereference" error.

I have had to copy SuppressedLoader into my own code where I can set group myself in order to workaround this.

Ideas

• ttlcache should expose a method for constructing SuppressedLoaders with non-nil group?
• SuppressedLoader should lazily set group to non-nil?

[swithek]

Yes, you are absolutely right! I'm just wondering what would be the best approach to fix this. I think to perform a lazy set of group, a new mutex would have to be introduced as well, which might degrade the performance for no real benefit.

So I'd say we have two options:

• Expose group, as you said. There would be no breaking changes, but as a user I would expect the library to set this field itself, since there are no singleflight.Group options that I could configure.
• Create a NewSuppressedLoader func that would set group and return a fully working loader. In this case, the Loader field could also be unexported and set inside the func (a parameter would be accepted for this), although that's a breaking change. On the other hand, since you are the first one to report this, maybe this change, albeit potentially breaking, might not affect that many users, if any at all. This way the design of the package wouldn't be negatively affected by pieces of code that would be left to be fixed in the next major release.

[markandrus]

I think I prefer the second option; however, I'd request NewSuppressedLoader to take *singleflight.Group as an option. If the *singleflight.Group is nil, NewSuppressedLoader could create one; otherwise, it could use the provided value.

The reason I think this is desirable is because I am following your suggestion here #74 (comment). I want to be able to do

loader := NewSuppressedLoader[K, V](...)
cache.Get("key", ttlcache.WithLoader[K, V](loader))

sharing the same *singleflight.Group between SuppressedLoaders. WDYT?

[swithek]

Would the inner loader wrapped by the suppressed loader be also the same in all these places? If so, you could just reuse the same suppressed loader, e.g., attach it to the service that uses the cache:

type Service struct {
     cache *ttlcache.Cache
     loader ttlcache.Loader
}

func New() *Service {
      return Service{
           cache: ttlcache.New(),
           loader: ttlcache.NewSuppressedLoader(...),
      } 
}

func (s *Service) Get1() {
       item := s.cache.Get("key", ttlcache.WithLoader(s.loader))
}

func (s *Service) Get2() {
       item := s.cache.Get("key", ttlcache.WithLoader(s.loader))
}

[markandrus]

I do something like this, so I don't believe I can share the loader:

type Service struct {
	cache *ttlcache.Cache
	group *singleflight.Group
}

func (s *Service) Get(ctx context.Context, key K) V {
	var loaded V

	loader := func(cache *ttlcache.Cache, key K) *ttlcache.Item {
		// 1. Make an API call, passing ctx.
		// 2. Set loaded to the API result.
		// 3. Update the cache if the API result can be cached; otherwise, return nil.
	}

	loaderFunc := ttlcache.LoaderFunc(loader)

	suppressedLoader := SuppressedLoader{
		Loader: loaderFunc,
		group:  group,
	}

	if cached := s.cache.Get(key, ttlcache.WithLoader(suppressedLoader)); cached != nil {
		return cached.Value()
	}

	return loaded
}

[swithek]

I see. In that case, the required changes are as follows:

• Transform the embedded Loader into an unexported loader Loader field.
• Create a NewSuppressedLoader(loader Loader, group *singleflight.Group) func. If group is nil, it is automatically created:

func NewSuppressedLoader(loader Loader, group *singleflight.Group) *SuppressedLoader {
    if group == nil {
        group = &singleflight.Group{}
    }
    
    return &SuppressedLoader{
        loader: loader,
        group: group,
    }
}

Have I missed anything?

[markandrus]

No, that sounds great! 🙌🏼

[austinbyers]

I just ran into this same issue! I love the SuppressedLoader, but it's not in the latest release. Any chance we can get a new release to include this fix?

[swithek]

Yep, you can find it here. Sorry for the delay.

[austinbyers]

Awesome, thanks for the quick response!