Index out-of-range panic in examples

[OscarVanL]

I have found a way to reliably produce an index out-of-range in the example.

go run extended_example.go

Send two GET requests to the same renewable resorce within a few seconds (so the second request has to wait for the promise of the first request):

GET localhost:8080/report/renewable/123

(wait 2 seconds)

GET localhost:8080/report/renewable/123

Then one of the requests fails with:

panic: runtime error: index out of range [1] with length 1

goroutine 35 [running]:
github.com/kristoff-it/redis-memolock/go/memolock.(*RedisMemoLock).dispatch(0xc00028c0c0)
        /Users/oscarvanleusen/go/src/github.com/redis-memolock/go/memolock/RedisMemoLock.go:88 +0x4b9
created by github.com/kristoff-it/redis-memolock/go/memolock.NewRedisMemoLock
        /Users/oscarvanleusen/go/src/github.com/redis-memolock/go/memolock/RedisMemoLock.go:125 +0x20a
exit status 2

The same problem can also be reproduced with the /ext/stemmer and /report/oh-no/ routes. I can't reproduce it with the /query/simple route though.

If I were to guess, this only affects GetResourceRenewable and GetResourceExternal, but not GetResource.

[OscarVanL]

The error appears to be related to handling subCh when isUnsub == true, notably this block:

			case true:
				// TODO: there are better strategies...
				if list, ok := r.subscriptions[sub.name]; ok {
					newList := list[:0]
					for _, x := range list {
						if sub.resCh != x {
							newList = append(newList, x)
						}
					}
					for i := len(newList); i < len(list); i++ {
						newList[i] = nil
					}
				}
			}

Here's what appears to be happening:

• We list all result channels for the subscription's name (I guess because multiple requests could be waiting for the promise result simultaneously).
• We create a sub-slice of the list of subscriptions with no elements
  • I assume this is done to avoid any extra allocs.
• Then we go and append all the subscriptions that aren't the one we're unsubscribing from
• We nil-out the leftover elements.

I added some prints:

			case true:
				// TODO: there are better strategies...
				if list, ok := r.subscriptions[sub.name]; ok {
					newList := list[:0]
					fmt.Printf("list %v, len: %d, cap: %d\n", list, len(list), cap(list))
					fmt.Printf("newList (before) %v, len: %d, cap: %d\n", newList, len(newList), cap(newList))
					for _, x := range list {
						if sub.resCh != x {
							newList = append(newList, x)
						}
					}
					fmt.Printf("newList (after append) %v, len: %d, cap: %d\n", newList, len(newList), cap(newList))
					for i := len(newList); i < len(list); i++ {
						newList[i] = nil
					}
				}

And it outputs this:

list [0xc0002804e0 0xc000180720], len: 2, cap: 2
newList (before) [], len: 0, cap: 2
newList (after append) [0xc000180720], len: 1, cap: 2
panic: runtime error: index out of range [1] with length 1

I guess the thought behind this code is that the cap is 2, so we want to null out all the values despite the len being 1.

To be honest I'm not an expert about how Go slices work behind the scenes, but I'm not sure if this is actually needed?

If I delete the second for-loop:

					for i := len(newList); i < len(list); i++ {
						newList[i] = nil
					}

it fixes the panic, but I can see that the len and cap of the slice gets bigger each time I call the API, so clearly I'm not cleaning up subscriptions properly. So that for-loop must have some imporatance.

list [0xc0001808a0 0xc0001808a0], len: 2, cap: 2
newList (before) [], len: 0, cap: 2
newList (after append) [], len: 0, cap: 2
list [0xc0001808a0 0xc0001808a0 0xc0000748a0 0xc0000749c0], len: 4, cap: 4
newList (before) [], len: 0, cap: 4
newList (after append) [0xc0001808a0 0xc0001808a0 0xc0000749c0], len: 3, cap: 4
list [0xc0001808a0 0xc0001808a0 0xc0000749c0 0xc0000749c0], len: 4, cap: 4
newList (before) [], len: 0, cap: 4
newList (after append) [0xc0001808a0 0xc0001808a0], len: 2, cap: 4
list [0xc0001808a0 0xc0001808a0 0xc0000749c0 0xc0000749c0 0xc000074ba0 0xc0002900c0], len: 6, cap: 8
newList (before) [], len: 0, cap: 8
newList (after append) [0xc0001808a0 0xc0001808a0 0xc0000749c0 0xc0000749c0 0xc0002900c0], len: 5, cap: 8
list [0xc0001808a0 0xc0001808a0 0xc0000749c0 0xc0000749c0 0xc0002900c0 0xc0002900c0], len: 6, cap: 8
newList (before) [], len: 0, cap: 8
newList (after append) [0xc0001808a0 0xc0001808a0 0xc0000749c0 0xc0000749c0], len: 4, cap: 8

[OscarVanL]

I can see in the fork by @andreas-tiket there is a fix for this

andreas-tiket@927dd11