Fix a bug that obscures the byte queue when it is full and is extended

[Fabianexe]

Hi,
On searching for the reason for the panics in the current version where versions pre 2.2 works well, I think I have found the cause.
With PR #207 it gets possible that the byte queen is full and q.tail == q.head. However, in the allocateAdditionalMemory method, the condition for resetting head and tail is q.tail < q.head. Thus the tail and head are not changed, and the byte queen is obscured after it. An example that leads to panic in the old logic is shown in the test TestPushEntryAfterAllocateAdditionMemoryInFull.

This is a different reason for panics then #236, so I am not sure which of the reported panic ( #226 #234 #235 ) is caused by this bug.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@9949a06). Click here to learn what that means.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #251   +/-   ##
=========================================
  Coverage          ?   86.56%           
=========================================
  Files             ?       15           
  Lines             ?      640           
  Branches          ?        0           
=========================================
  Hits              ?      554           
  Misses            ?       71           
  Partials          ?       15           

Impacted Files	Coverage Δ
queue/bytes_queue.go	88.88% <100.00%> (ø)
bytes.go	100.00% <0.00%> (ø)
server/cache_handlers.go	90.38% <0.00%> (ø)
config.go	100.00% <0.00%> (ø)
iterator.go	88.67% <0.00%> (ø)
server/stats_handler.go	71.42% <0.00%> (ø)
shard.go	84.93% <0.00%> (ø)
bigcache.go	98.79% <0.00%> (ø)
encoding.go	100.00% <0.00%> (ø)
server/server.go	29.03% <0.00%> (ø)
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9949a06...8162a32. Read the comment docs.

[janisz]

LGTM

I've tested it with test included in #234 but it does not solve that issues

package bigcache

import (
	"math/rand"
	"runtime"
	"strconv"
	"testing"
	"time"
)

const (
	entries = 3000
	repeats = 1000
)

var valBig = append(make([]byte, 100*1024), 1)
var valMed = append(make([]byte, 1024), 1)
var valSmall = append(make([]byte, 2), 1)

func getValue() []byte {
	x := rand.Float64()
	if x < 0.7 {
		return valSmall
	} else if x < 0.9 {
		return valMed
	}
	return valBig
}

// https://github.com/allegro/bigcache/issues/234#issuecomment-673895517
func Test_issue_234(t *testing.T) {
	t.Log("Number of entries: ", entries)
	printAllocs(t)
	rand.Seed(1)

	config := Config{
		Shards:             128,
		LifeWindow:         time.Hour,
		CleanWindow:        time.Second,
		MaxEntriesInWindow: entries,
		MaxEntrySize:       1024,
		Verbose:            true,
		HardMaxCacheSize:   128,
		OnRemoveWithReason: func(key string, entry []byte, reason RemoveReason) {
			t.Log("Evicted:", len(entry), " reason: ", reason)
		},
	}

	bigcache, err := NewBigCache(config)
	if err != nil {
		panic(err)
	}
	for i := 0; i < repeats; i++ {
		printAllocs(t)
		for j := 0; j < entries; j++ {
			key := strconv.FormatInt(int64(j), 10)
			err := bigcache.Set(key, getValue())
			if err != nil {
				t.Fatal(err)
			}
		}
	}
}

func printAllocs(t *testing.T) {
	var m runtime.MemStats
	runtime.ReadMemStats(&m)
	t.Logf("Alloc: %6d MB \n", m.Alloc/1e6)
}

[Fabianexe]

@janisz
I have relooked at the test that you suggested.
This is a related but a little different bug.
By reallocating the byte queen, an artificial entry is added to close the gap between the head and tail. This artificial entry could have no content. This is fine for the byte queen, but by calling Pop this entry is published to the shard, and this shard then expects content.
I have pushed a fix for this bug as well. Simple set the minimum size for such entry to 17 (size+timestamp+hash).

A more minimalistic test for the bug from #234 is:

package bigcache

import (
	"testing"
	"time"
)

// https://github.com/allegro/bigcache/issues/234#issuecomment-673895517
func Test_issue_234(t *testing.T) {

	config := Config{
		Shards:             1,
		LifeWindow:         time.Hour,
		MaxEntriesInWindow: 900,
		MaxEntrySize:       1024,
		Verbose:            true,
		HardMaxCacheSize:   1,
		OnRemoveWithReason: func(key string, entry []byte, reason RemoveReason) {
			t.Log("Evicted:", len(entry), " reason: ", reason)
		},
	}

	bigcache, _ := NewBigCache(config)
	bigcache.Set("1", make([]byte, 400*1024))
	bigcache.Set("2", make([]byte, 400*1024))
	bigcache.Set("3", make([]byte, 400*1024-1))
	bigcache.Set("4", make([]byte, 100*1024))
	bigcache.Set("5", make([]byte, 400*1024))
}

[janisz]

@mxplusb @cristaloleg Can you look at this?

[siennathesane]

LGTM! Thanks!

[Fabianexe]

@mxplusb
I would love to see a new release with this so that bigcache can simple update

[siennathesane]

@Fabianexe I released v2.2.5 just now, which includes this fix, so you should be good to go!

[janisz]

Refs: #253