store/bucket: wait until chunk loading ends in Close()

[GiedriusS]

The chunk reader needs to wait until the chunk loading ends in Close() because otherwise there will be a race between appending to r.chunkBytes and reading from it. This is because Close() only cancels the context but populateChunk() cannot check the context in a way not to cause a race. So, if the context is canceled between getting data and populating chunks then there's a race.

[fpetkovski]

Hm, where did this go?

[GiedriusS]

Idea was to put everything into the slice that is Put() back during Close() but it seems like this refactoring was faulty. I removed it.

[douglascamata]

This looks like a good place and time to apply https://pkg.go.dev/sync#Cond?

[douglascamata]

Leaving some suggestions on how I think sync.Cond could be use here. Feel free to decide whether to adopt.

[douglascamata]

Suggested change

	loadingChunksMtx sync.Mutex
	loadingChunks bool
	finishLoadingChks chan struct{}
	loadingChunksCond *sync.Cond
	loadingChunks bool

[fpetkovski]

I think sync.Cond is hard to use and understand, and I would prefer to have a simpler solution with a mutex or an atomic variable.

[douglascamata]

@fpetkovski that's also a great idea. If we bump our minimum Go version high enough, we could use generic version of atomics to easily swap something of any type.

[douglascamata]

But I guess the critical part here is "waiting for a condition to be fulfilled": lock released when chunks are finally loaded.

[GiedriusS]

An atomic variable won't work because we must wait until the chunk loading finishes. sync.Cond doesn't work because at the end of load() it will signal only once whereas we need a permanent state of either on or off. As far as I can tell, with your suggestion in a normal operation Close() will hang forever because it will never receive a signal.

I think the only way to simplify this is to get rid of the bool and create the channel under the lock. 🤔

[douglascamata]

@GiedriusS even with sync.Cond you will see that my suggestions keep the bool variable. The instance of sync.Cond only manages the pause/resume of execution when the bool variable (the condition) fails. You can see it in the suggestion below, where we don't even check the sync.Cond of the bool says blocks are loaded:

r.loadingChunksCond.L.Lock()
if r.loadingChunks {
    r.loadingChunksCond.Wait()
}
r.loadingChunksCond.L.Unlock()

[douglascamata]

In a way, the sync.Cond is only abstracting the channel. We need still the condition's bool variable to be checked.

[GiedriusS]

Mhm, so maybe we can merge this now and clean up later to unblock the release?

[saswatamcode]

Let's go ahead with this then, and iterate on it. 🙂

[douglascamata]

Suggested change

	r.loadingChunksMtx.Lock()
	r.loadingChunks = false
	r.finishLoadingChks = make(chan struct{})
	r.loadingChunksMtx.Unlock()
	r.loadingChunksCond = sync.NewCond(&sync.Mutex{})

No need to initialize r.loadingChunks as the default value for a bool is false.

[douglascamata]

Suggested change

	// NOTE(GiedriusS): we need to wait until loading chunks because loading
	// chunks modifies r.block.chunkPool.
	r.loadingChunksMtx.Lock()
	loadingChks := r.loadingChunks
	r.loadingChunksMtx.Unlock()
	if loadingChks {
	<-r.finishLoadingChks
	}
	// Locks the condition and wait for a signal.
	r.loadingChunksCond.L.Lock()
	if r.loadingChunks {
	r.loadingChunksCond.Wait()
	}
	r.loadingChunksCond.L.Unlock()

[douglascamata]

Suggested change

	r.loadingChunksMtx.Lock()
	r.loadingChunks = true
	r.loadingChunksMtx.Unlock()
	defer func() {
	r.loadingChunksMtx.Lock()
	r.loadingChunks = false
	r.loadingChunksMtx.Unlock()
	close(r.finishLoadingChks)
	}()
	// when done loading, signal to anyone waiting on chunks to be loaded.
	defer r.loadingChunksCond.Signal()

Could use r.loadingCunks.Broadcast() here if there could be multiple Go routines waiting for chunks to be loaded.