chore: file cache Release tied 1:1 with an acquire #18410
Conversation
Made idempotent
coderd/files/cache.go
Outdated
| type CloseFS struct { | ||
| fs.FS | ||
|
|
||
| close func() error | ||
| } | ||
|
|
||
| func (f *CloseFS) Close() error { return f.close() } |
There was a problem hiding this comment.
I named this Close to match the pattern of opening a file and deferring a Close
| // | ||
| // release should only be called after a successful call to Acquire using the Release() | ||
| // method on the returned *CloseFS. | ||
| func (c *Cache) release(fileID uuid.UUID) { |
There was a problem hiding this comment.
Unexported to force callers to use the Close method for releasing.
| return nil, err | ||
| } | ||
|
|
||
| return it.FS, err | ||
| var once sync.Once |
There was a problem hiding this comment.
I'd rather store the Once on the struct and then just put the closing logic directly in the Close method.
There was a problem hiding this comment.
You also have to store the files.Cache on the struct then too.
Which allows calling Acquire and other methods.
It would be something like:
type CloseFS struct {
fs.FS
once sync.Once
fileID uuid.UUID
cache *Cache
}
func (f *CloseFS) Close() error {
f.once.Do(func() {
f.cache.release(f.fileID)
})
}I do not want cache to be accessible, so I could throw the release as a method, but then we're back to having an anonymous function as a field. In that case, I'd rather not add fields to the struct that are only used in Close
| @@ -206,7 +208,8 @@ func TestRelease(t *testing.T) { | |||
| for closedIdx, id := range ids { | |||
| stillOpen := len(ids) - closedIdx | |||
| for closingIdx := range batchSize { | |||
| c.Release(id) | |||
| _ = releases[id][0]() | |||
| releases[id] = releases[id][1:] | |||
There was a problem hiding this comment.
just change the loops to iterate over this releases map if you really wanna do it this way
There was a problem hiding this comment.
I didn't feel like changing the test. This works
Made idempotent