Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit04d202a

Browse files
authored
chore: file cache Release tied 1:1 with an acquire (#18410)
File cache close made idempotent
1 parent4039327 commit04d202a

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
typefetcherfunc(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+
typeCloseFSstruct {
149+
fs.FS
150+
151+
closefunc()
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 outsideof`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
iferr!=nil {
156-
c.Release(fileID)
169+
c.release(fileID)
157170
returnnil,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
iferr:=c.authz.Authorize(ctx,subject,policy.ActionRead,it.Object);err!=nil {
166-
c.Release(fileID)
179+
c.release(fileID)
167180
returnnil,err
168181
}
169182

170-
returnit.FS,err
183+
varonce 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
returnentry.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
deferc.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
foropenedIdx,id:=rangeids {
185186
forbatchIdx:=rangebatchSize {
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
forclosedIdx,id:=rangeids {
207209
stillOpen:=len(ids)-closedIdx
208210
forclosingIdx:=rangebatchSize {
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+
vartemplateFS fs.FS
149+
closeableTemplateFS,err:=api.FileCache.Acquire(fileCtx,fileID)
148150
iferr!=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-
deferapi.FileCache.Release(fileID)
157+
defercloseableTemplateFS.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-
deferapi.FileCache.Release(tf.CachedModuleFiles.UUID)
179+
defermoduleFilesFS.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)
[8]ページ先頭
©2009-2025 Movatter.jp