- Notifications
You must be signed in to change notification settings - Fork915
chore: file cache Release tied 1:1 with an acquire#18410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
Made idempotent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It might be even more correct to have each file reference have someuuid. Then we can be sure Aquire to Release is 1:1.
I did not want to refactor too much right now, so kept the underlying release mechanism the same.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I named thisClose to match the pattern of opening a file and deferring aClose
| // | ||
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unexported to force callers to use theClose method for releasing.
| returnnil,err | ||
| } | ||
| returnit.FS,err | ||
| varonce sync.Once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd rather store theOnce on thestruct and then just put the closing logic directly in theClose method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You also have to store thefiles.Cache on the struct then too.
Which allows callingAcquire and other methods.
It would be something like:
typeCloseFSstruct {fs.FSonce sync.OncefileID uuid.UUIDcache*Cache}func (f*CloseFS)Close()error {f.once.Do(func() {f.cache.release(f.fileID)})}
I do not wantcache to be accessible, so I could throw therelease 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 inClose
Uh oh!
There was an error while loading.Please reload this page.
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
just change the loops to iterate over thisreleases map if you really wanna do it this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I didn't feel like changing the test. This works
Uh oh!
There was an error while loading.Please reload this page.
04d202a intomainUh oh!
There was an error while loading.Please reload this page.
Made idempotent