Skip to content

Commit 04d202a

Browse files
EmyrkEmyrk
authored
chore: file cache Release tied 1:1 with an acquire (#18410)
File cache close made idempotent
1 parent 4039327 commit 04d202a

File tree

3 files changed

+49
-17
lines changed

3 files changed

+49
-17
lines changed

‎coderd/files/cache.go

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -140,20 +140,33 @@ type cacheEntry struct {
140140

141141
type fetcher func(context.Context, uuid.UUID) (CacheEntryValue, error)
142142

143+
var _ fs.FS = (*CloseFS)(nil)
144+
145+
// CloseFS is a wrapper around fs.FS that implements io.Closer. The Close()
146+
// method tells the cache to release the fileID. Once all open references are
147+
// closed, the file is removed from the cache.
148+
type CloseFS struct {
149+
fs.FS
150+
151+
close func()
152+
}
153+
154+
func (f *CloseFS) Close() { f.close() }
155+
143156
// Acquire will load the fs.FS for the given file. It guarantees that parallel
144157
// calls for the same fileID will only result in one fetch, and that parallel
145158
// calls for distinct fileIDs will fetch in parallel.
146159
//
147160
// Safety: Every call to Acquire that does not return an error must have a
148161
// matching call to Release.
149-
func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (fs.FS, error) {
150-
// It's important that this `Load` call occurs outside of `prepare`, after the
162+
func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) {
163+
// It's important that this `Load` call occurs outside `prepare`, after the
151164
// mutex has been released, or we would continue to hold the lock until the
152165
// entire file has been fetched, which may be slow, and would prevent other
153166
// files from being fetched in parallel.
154167
it, err := c.prepare(ctx, fileID).Load()
155168
if err != nil {
156-
c.Release(fileID)
169+
c.release(fileID)
157170
return nil, err
158171
}
159172

@@ -163,11 +176,19 @@ func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (fs.FS, error) {
163176
}
164177
// Always check the caller can actually read the file.
165178
if err := c.authz.Authorize(ctx, subject, policy.ActionRead, it.Object); err != nil {
166-
c.Release(fileID)
179+
c.release(fileID)
167180
return nil, err
168181
}
169182

170-
return it.FS, err
183+
var once sync.Once
184+
return &CloseFS{
185+
FS: it.FS,
186+
close: func() {
187+
// sync.Once makes the Close() idempotent, so we can call it
188+
// multiple times without worrying about double-releasing.
189+
once.Do(func() { c.release(fileID) })
190+
},
191+
}, nil
171192
}
172193

173194
func (c *Cache) prepare(ctx context.Context, fileID uuid.UUID) *lazy.ValueWithError[CacheEntryValue] {
@@ -203,9 +224,12 @@ func (c *Cache) prepare(ctx context.Context, fileID uuid.UUID) *lazy.ValueWithEr
203224
return entry.value
204225
}
205226

206-
// Release decrements the reference count for the given fileID, and frees the
227+
// release decrements the reference count for the given fileID, and frees the
207228
// backing data if there are no further references being held.
208-
func (c *Cache) Release(fileID uuid.UUID) {
229+
//
230+
// release should only be called after a successful call to Acquire using the Release()
231+
// method on the returned *CloseFS.
232+
func (c *Cache) release(fileID uuid.UUID) {
209233
c.lock.Lock()
210234
defer c.lock.Unlock()
211235

‎coderd/files/cache_test.go

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ func TestCacheRBAC(t *testing.T) {
7575
require.Equal(t, 0, cache.Count())
7676

7777
// Read the file with a file reader to put it into the cache.
78-
_, err := cache.Acquire(cacheReader, file.ID)
78+
a, err := cache.Acquire(cacheReader, file.ID)
7979
require.NoError(t, err)
8080
require.Equal(t, 1, cache.Count())
8181

@@ -86,12 +86,12 @@ func TestCacheRBAC(t *testing.T) {
8686
require.Equal(t, 1, cache.Count())
8787

8888
// UserReader can
89-
_, err = cache.Acquire(userReader, file.ID)
89+
b, err := cache.Acquire(userReader, file.ID)
9090
require.NoError(t, err)
9191
require.Equal(t, 1, cache.Count())
9292

93-
cache.Release(file.ID)
94-
cache.Release(file.ID)
93+
a.Close()
94+
b.Close()
9595
require.Equal(t, 0, cache.Count())
9696

9797
rec.AssertActorID(t, nobodyID.String(), rec.Pair(policy.ActionRead, file))
@@ -179,13 +179,15 @@ func TestRelease(t *testing.T) {
179179
ids = append(ids, uuid.New())
180180
}
181181

182+
releases := make(map[uuid.UUID][]func(), 0)
182183
// Acquire a bunch of references
183184
batchSize := 10
184185
for openedIdx, id := range ids {
185186
for batchIdx := range batchSize {
186187
it, err := c.Acquire(ctx, id)
187188
require.NoError(t, err)
188-
require.Equal(t, emptyFS, it)
189+
require.Equal(t, emptyFS, it.FS)
190+
releases[id] = append(releases[id], it.Close)
189191

190192
// Each time a new file is opened, the metrics should be updated as so:
191193
opened := openedIdx + 1
@@ -206,7 +208,8 @@ func TestRelease(t *testing.T) {
206208
for closedIdx, id := range ids {
207209
stillOpen := len(ids) - closedIdx
208210
for closingIdx := range batchSize {
209-
c.Release(id)
211+
releases[id][0]()
212+
releases[id] = releases[id][1:]
210213

211214
// Each time a file is released, the metrics should decrement the file refs
212215
require.Equal(t, (stillOpen*batchSize)-(closingIdx+1), promhelp.GaugeValue(t, reg, cachePromMetricName("open_file_refs_current"), nil))

‎coderd/parameters.go

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"database/sql"
66
"encoding/json"
7+
"io/fs"
78
"net/http"
89
"time"
910

@@ -144,15 +145,19 @@ func (api *API) handleDynamicParameters(listen bool, rw http.ResponseWriter, r *
144145
}
145146

146147
// Add the file first. Calling `Release` if it fails is a no-op, so this is safe.
147-
templateFS, err := api.FileCache.Acquire(fileCtx, fileID)
148+
var templateFS fs.FS
149+
closeableTemplateFS, err := api.FileCache.Acquire(fileCtx, fileID)
148150
if err != nil {
149151
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
150152
Message: "Internal error fetching template version Terraform.",
151153
Detail: err.Error(),
152154
})
153155
return
154156
}
155-
defer api.FileCache.Release(fileID)
157+
defer closeableTemplateFS.Close()
158+
// templateFS does not implement the Close method. For it to be later merged with
159+
// the module files, we need to convert it to an OverlayFS.
160+
templateFS = closeableTemplateFS
156161

157162
// Having the Terraform plan available for the evaluation engine is helpful
158163
// for populating values from data blocks, but isn't strictly required. If
@@ -171,9 +176,9 @@ func (api *API) handleDynamicParameters(listen bool, rw http.ResponseWriter, r *
171176
})
172177
return
173178
}
174-
defer api.FileCache.Release(tf.CachedModuleFiles.UUID)
179+
defer moduleFilesFS.Close()
175180

176-
templateFS = files.NewOverlayFS(templateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}})
181+
templateFS = files.NewOverlayFS(closeableTemplateFS, []files.Overlay{{Path: ".terraform/modules", FS: moduleFilesFS}})
177182
}
178183

179184
owner, err := getWorkspaceOwnerData(ctx, api.Database, apikey.UserID, templateVersion.OrganizationID)

0 commit comments

Comments
 (0)