store/bucket: wait until chunk loading ends in Close()#6582
store/bucket: wait until chunk loading ends in Close()#6582saswatamcode merged 1 commit intomainfrom
Conversation
| r.stats.chunksTouched++ | ||
| r.stats.ChunksTouchedSizeSum += units.Base2Bytes(int(chunkDataLen)) | ||
|
|
||
| r.block.chunkPool.Put(nb) |
There was a problem hiding this comment.
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.
9bf6078 to
08da27e
Compare
08da27e to
cfc6e39
Compare
cfc6e39 to
5273244
Compare
1d0523c to
e55687c
Compare
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. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
e55687c to
6eb3eb7
Compare
|
This looks like a good place and time to apply https://pkg.go.dev/sync#Cond? |
douglascamata
left a comment
There was a problem hiding this comment.
Leaving some suggestions on how I think sync.Cond could be use here. Feel free to decide whether to adopt.
| loadingChunksMtx sync.Mutex | ||
| loadingChunks bool | ||
| finishLoadingChks chan struct{} |
There was a problem hiding this comment.
| loadingChunksMtx sync.Mutex | |
| loadingChunks bool | |
| finishLoadingChks chan struct{} | |
| loadingChunksCond *sync.Cond | |
| loadingChunks bool |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
But I guess the critical part here is "waiting for a condition to be fulfilled": lock released when chunks are finally loaded.
There was a problem hiding this comment.
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. 🤔
There was a problem hiding this comment.
@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()There was a problem hiding this comment.
In a way, the sync.Cond is only abstracting the channel. We need still the condition's bool variable to be checked.
There was a problem hiding this comment.
Mhm, so maybe we can merge this now and clean up later to unblock the release?
There was a problem hiding this comment.
Let's go ahead with this then, and iterate on it. 🙂
| r.loadingChunksMtx.Lock() | ||
| r.loadingChunks = false | ||
| r.finishLoadingChks = make(chan struct{}) | ||
| r.loadingChunksMtx.Unlock() |
There was a problem hiding this comment.
| 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.
| // 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 | ||
| } |
There was a problem hiding this comment.
| // 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() |
| r.loadingChunksMtx.Lock() | ||
| r.loadingChunks = true | ||
| r.loadingChunksMtx.Unlock() | ||
|
|
||
| defer func() { | ||
| r.loadingChunksMtx.Lock() | ||
| r.loadingChunks = false | ||
| r.loadingChunksMtx.Unlock() | ||
|
|
||
| close(r.finishLoadingChks) | ||
| }() | ||
|
|
There was a problem hiding this comment.
| 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.
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. Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
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.