Skip to content

Commit 55dfb22

Browse files
EmyrkEmyrk
committed
chore: file cache Release tied 1:1 with an acquire
Made idempotent
1 parent ebc769f commit 55dfb22

File tree

3 files changed

+48
-15
lines changed

3 files changed

+48
-15
lines changed

‎coderd/files/cache.go

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package files
33
import (
44
"bytes"
55
"context"
6+
"io"
67
"io/fs"
78
"sync"
89

@@ -140,20 +141,34 @@ type cacheEntry struct {
140141

141142
type fetcher func(context.Context, uuid.UUID) (CacheEntryValue, error)
142143

144+
var _ fs.FS = (*CloseFS)(nil)
145+
var _ io.Closer = (*CloseFS)(nil)
146+
147+
// CloseFS is a wrapper around fs.FS that implements io.Closer. The Close()
148+
// method tells the cache to release the fileID. Once all open references are
149+
// closed, the file is removed from the cache.
150+
type CloseFS struct {
151+
fs.FS
152+
153+
close func() error
154+
}
155+
156+
func (f *CloseFS) Close() error { return f.close() }
157+
143158
// Acquire will load the fs.FS for the given file. It guarantees that parallel
144159
// calls for the same fileID will only result in one fetch, and that parallel
145160
// calls for distinct fileIDs will fetch in parallel.
146161
//
147162
// Safety: Every call to Acquire that does not return an error must have a
148163
// matching call to Release.
149-
func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (fs.FS, error) {
164+
func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (*CloseFS, error) {
150165
// It's important that this `Load` call occurs outside of `prepare`, after the
151166
// mutex has been released, or we would continue to hold the lock until the
152167
// entire file has been fetched, which may be slow, and would prevent other
153168
// files from being fetched in parallel.
154169
it, err := c.prepare(ctx, fileID).Load()
155170
if err != nil {
156-
c.Release(fileID)
171+
c.release(fileID)
157172
return nil, err
158173
}
159174

@@ -163,11 +178,20 @@ func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (fs.FS, error) {
163178
}
164179
// Always check the caller can actually read the file.
165180
if err := c.authz.Authorize(ctx, subject, policy.ActionRead, it.Object); err != nil {
166-
c.Release(fileID)
181+
c.release(fileID)
167182
return nil, err
168183
}
169184

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

173197
func (c *Cache) prepare(ctx context.Context, fileID uuid.UUID) *lazy.ValueWithError[CacheEntryValue] {
@@ -203,9 +227,12 @@ func (c *Cache) prepare(ctx context.Context, fileID uuid.UUID) *lazy.ValueWithEr
203227
return entry.value
204228
}
205229

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

‎coderd/files/cache_test.go

Lines changed: 8 additions & 5 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() error, 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)
188189
require.Equal(t, emptyFS, it)
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: 7 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,17 @@ 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 = closeableTemplateFS
156159

157160
// Having the Terraform plan available for the evaluation engine is helpful
158161
// for populating values from data blocks, but isn't strictly required. If
@@ -171,9 +174,9 @@ func (api *API) handleDynamicParameters(listen bool, rw http.ResponseWriter, r *
171174
})
172175
return
173176
}
174-
defer api.FileCache.Release(tf.CachedModuleFiles.UUID)
177+
defer moduleFilesFS.Close()
175178

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

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

0 commit comments

Comments
 (0)