pool: add pool raciness tests, fix p.usedTotal house-keeping

[GiedriusS]

I have noticed with these tests that the p.usedTotal value keeps
jumping like this:

6
3
6
6
18446744073709551604
3
18446744073709551607
18446744073709551604

It could happen that the value of it would underflow as the tests show:

unexpected error: goroutine barbazbaz: pool exhausted

This happens when a caller does like a Get(3) and then makes the slice larger by appending more items to it. Then, when returning a slice we subtract its capacity from p.usedTotal and that can underflow. Also, that can happen because there is no synchronization between different users.

Furthermore, lets fix the house-keeping of the p.usedTotal completely by not subtracting the p.usedTotal from itself whenever Put was called. I am not sure that it was ever working correctly.

Also, go test -race reports without these changes:

==================
WARNING: DATA RACE
Read at 0x00c0000c0538 by goroutine 11:
  github.com/improbable-eng/thanos/pkg/pool.(*BytesPool).Put()
      /home/gstatkevicius/dev/thanos/pkg/pool/pool.go:93 +0x152
  github.com/improbable-eng/thanos/pkg/pool.TestRacePutGet.func1()
      /home/gstatkevicius/dev/thanos/pkg/pool/pool_test.go:103 +0x9a

Previous write at 0x00c0000c0538 by goroutine 12:
  sync/atomic.AddInt64()
      /usr/local/go/src/runtime/race_amd64.s:276 +0xb
  github.com/improbable-eng/thanos/pkg/pool.(*BytesPool).Get()
      /home/gstatkevicius/dev/thanos/pkg/pool/pool.go:71 +0x186
  github.com/improbable-eng/thanos/pkg/pool.TestRacePutGet.func1()
      /home/gstatkevicius/dev/thanos/pkg/pool/pool_test.go:80 +0xe4

Goroutine 11 (running) created at:
  github.com/improbable-eng/thanos/pkg/pool.TestRacePutGet()
      /home/gstatkevicius/dev/thanos/pkg/pool/pool_test.go:109 +0x2ca
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:865 +0x163

Goroutine 12 (running) created at:
  github.com/improbable-eng/thanos/pkg/pool.TestRacePutGet()
      /home/gstatkevicius/dev/thanos/pkg/pool/pool_test.go:110 +0x2f5
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:865 +0x163
==================

After these changes, the race detector doesn't complain anymore.

[GiedriusS]

Tests work well on my laptop but fail on Circle CI. Need to investigate 😖

[povilasv]

I think this used to work because, previously return value was []byte slice, where you can't really grow the slice as you can't change the slice header. (https://github.com/thanos-io/thanos/pull/1227/files)

Since we did change to *[]byte, IMO we should just add docs, that users of pool shouldn't make byte slices larger. And if they do, the outcome might be unexpected.

IMO atomic is more preferred way than mutex in this case as it's more efficient. So I would keep the old code, just update docs and modify tests.

[GiedriusS]

I agree that it would be more beneficial to us from performance's perspective to stay with the old code which uses atomic. So if it's okay with everyone then lets document that assumption very clearly and fix the p.usedTotal house-keeping so that we would not subtract that number from itself?

[povilasv]

👍

Basically atomic.AddUint64(&p.usedTotal, ^uint64(p.usedTotal-1)) -> atomic.AddUint64(&p.usedTotal, ^uint64(sz-1)) and then docs + tests?

[GiedriusS]

Yes, is that's OK with you?

[povilasv]

yy SGTM 🥇

[bwplotka]

Sad we are locking underlying sync.Pool as it is already go routine safe, but definitely we want to have solid limit on used - many races where possible here indeed.

Thanks! LGTM!

[GiedriusS]

@bwplotka have you taken a look at the previous comments posted by @povilasv? 👀 Looking at this again, AFAICT it is impossible to properly maintain p.usedTotal with atomic since two goroutines can be in Put() or Get() and because there's no lock, inconsistencies can arise.

[povilasv]

Why not? I can't see how that wouldn't work :D I mean a couple of gourotines would do Get and at a later point they would return it with Put, as we are adding number atomically, shouldn't it all converge?

[GiedriusS]

Because we are putting that data into a separate variable and it can change from underneath us since there's no locking, right @povilasv?

[GiedriusS]

It won't necessarily converge into 0 since, for example, one goroutine will get that there's X bytes used and then set it to X-Y, but another one will already have X too in the temporary variable but it will set it to X-Z. So the first update is lost unless we will do this sequentially. Does that make sense? However, we could sprinkle more checks all over the code so that we wouldn't use a temporary variable but I am not sure that it would really be faster than holding a mutex. Sorry for being terse, writing on my phone.

[povilasv]

yeah it does. Then mutex makes sense.

[GiedriusS]

Sorry, wanted to add one last thing that came to my mind: I think the problem here is that what we need to do is to atomatically compare one number with another, and then make a decision based on that - should we allocate or not? And with atomic it's impossible to do that - we still need to load it into a separate variable and write if .... However, a problem lies there: it's a classical set-and-get race. I think we should really stay with this PR as-is.

[povilasv]

👍