[BUG] Panics when running under synctest

[kralewitz]

Description

Hello,

I'm running into two different panics when using an otter cache (v2.2.1) inside of a synctest 'bubble'.

Both panics occur when an ExpiryCalculator is set and stem from this cleanup function:

func New[K comparable, V any](o *Options[K, V]) (*Cache[K, V], error) {
        ...
	runtime.AddCleanup(c, func(cacheImpl *cache[K, V]) {
		cacheImpl.close()
	}, cacheImpl)
        ...
}

func (c *cache[K, V]) close() {
	if c.withExpiration {
		c.doneClose <- struct{}{}
	}
}

First panic occurs when the cleanup function actually runs.:

panic: close of synctest channel from outside bubble

This is related to this Go issue: https://github.com/golang/go/issues/75113

The second panic occurs when the test ends without the cleanup function being run and the periodicCleanUp goroutine remains blocked:

panic: deadlock: main bubble goroutine has exited but blocked goroutines remain [recovered, repanicked]

goroutine 10 [select (durable), synctest bubble 1]:
github.com/maypok86/otter/v2.(*cache[...]).periodicCleanUp(0x60dc80)
	/home/kwz/go/pkg/mod/github.com/maypok86/otter/[email protected]/cache_impl.go:1273 +0xb9
created by github.com/maypok86/otter/v2.newCache[...] in goroutine 9
	/home/kwz/go/pkg/mod/github.com/maypok86/otter/[email protected]/cache_impl.go:194 +0x90b

Minimal reproducer

package x

import (
	"testing"
	"testing/synctest"
	"time"

	"github.com/maypok86/otter/v2"
)

func Test_Cache(t *testing.T) {
	synctest.Test(t, func(t *testing.T) {
		cache, err := otter.New(&otter.Options[string, string]{
			ExpiryCalculator: otter.ExpiryAccessing[string, string](time.Minute),
		})
		if err != nil {
			t.Fatal(err)
		}
		cache.Set("key", "value")

                // uncomment to get the first panic
                // runtime.GC()
	})
}

Expected behavior

Ideally, the cache should be usable inside of synctest bubbles, which means dropping the runtime.Cleanup for the time being. My suggestions would be:

1. Reintroduce some sort of Close function to the cache API (which I see was dropped in v2) to handle the cleanup

2. Run the periodicCleanUp worker via the Executor, so the caller can manage its lifetime

Both however would mean that the periodicCleanUp goroutine would be leaked by default.

Oh and thanks for all of your work that went into this library!

[maypok86]

Hi, I'm not sure, but this looks very similar to this discussion, and the verdict is about the same.

[kralewitz]

Ah, sorry for not noticing the discussion before. Having read through it, the code snippets use the old 'experimental' synctest API. The synctest has since moved into 'general availability' with a slightly different API, and the code snippets in the discussion thread do not hang any more. The issue with runtime.Cleanup and synctest still remains though, and from the linked Go issue it does not seem like it will be solved anytime soon.

For now I'm using a forked copy with the following changes:

• removed the runtime.AddCleanup call
• added a public Close method
• run periodicCleanup via the executor, so I can pass in a waitgroup and wait for the cleanup goroutine to return after closing

It's a bit more work, but these changes make the caches play nicely with synctest.

[maypok86]

Hi, I've been trying to avoid adding an extra method to stop the goroutine used for entry expiration, but unfortunately, it would require additional abstraction for launching the Executor. So I added a StopAllGoroutines method that stops the goroutines.

I'll probably release a new version this weekend, but I might also try another approach to solve a different issue.