Skip to content

Commit 7b8156c

Browse files
ola-rozenfeldola-rozenfeld
committed
Addressing Marc Antoine's comments
1 parent 21666d4 commit 7b8156c

File tree

5 files changed

+76
-62
lines changed

5 files changed

+76
-62
lines changed

‎go/pkg/diskcache/diskcache.go‎

Lines changed: 40 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ import (
1616
"time"
1717

1818
"github.com/bazelbuild/remote-apis-sdks/go/pkg/digest"
19+
"golang.org/x/sync/errgroup"
20+
1921
log "github.com/golang/glog"
2022
)
2123

@@ -100,7 +102,7 @@ type DiskCache struct {
100102
testGcTicks chan uint64
101103
}
102104

103-
func New(ctx context.Context, root string, maxCapacityBytes uint64) *DiskCache {
105+
func New(ctx context.Context, root string, maxCapacityBytes uint64) (*DiskCache, error) {
104106
res := &DiskCache{
105107
root: root,
106108
maxCapacityBytes: maxCapacityBytes,
@@ -112,44 +114,45 @@ func New(ctx context.Context, root string, maxCapacityBytes uint64) *DiskCache {
112114
shutdown: make(chan bool),
113115
}
114116
heap.Init(res.queue)
115-
_ = os.MkdirAll(root, os.ModePerm)
117+
if err := os.MkdirAll(root, os.ModePerm); err != nil {
118+
return nil, err
119+
}
116120
// We use Git's directory/file naming structure as inspiration:
117121
// https://git-scm.com/book/en/v2/Git-Internals-Git-Objects#:~:text=The%20subdirectory%20is%20named%20with%20the%20first%202%20characters%20of%20the%20SHA%2D1%2C%20and%20the%20filename%20is%20the%20remaining%2038%20characters.
118-
var wg sync.WaitGroup
119-
wg.Add(256)
122+
eg, eCtx := errgroup.WithContext(ctx)
120123
for i := 0; i < 256; i++ {
121124
prefixDir := filepath.Join(root, fmt.Sprintf("%02x", i))
122-
go func() {
123-
defer wg.Done()
124-
_ = os.MkdirAll(prefixDir, os.ModePerm)
125-
_ = filepath.WalkDir(prefixDir, func(path string, d fs.DirEntry, err error) error {
125+
eg.Go(func() error {
126+
if eCtx.Err() != nil {
127+
return eCtx.Err()
128+
}
129+
if err := os.MkdirAll(prefixDir, os.ModePerm); err != nil {
130+
return err
131+
}
132+
return filepath.WalkDir(prefixDir, func(path string, d fs.DirEntry, err error) error {
126133
// We log and continue on all errors, because cache read errors are not critical.
127134
if err != nil {
128-
log.Errorf("Error reading cache directory: %v", err)
129-
return nil
135+
return fmt.Errorf("error reading cache directory: %v", err)
130136
}
131137
if d.IsDir() {
132138
return nil
133139
}
134140
subdir := filepath.Base(filepath.Dir(path))
135141
k, err := res.getKeyFromFileName(subdir + d.Name())
136142
if err != nil {
137-
log.Errorf("Error parsing cached file name %s: %v", path, err)
138-
return nil
143+
return fmt.Errorf("error parsing cached file name %s: %v", path, err)
139144
}
140-
atime, err := GetLastAccessTime(path)
145+
atime, err := getLastAccessTime(path)
141146
if err != nil {
142-
log.Errorf("Error getting last accessed time of %s: %v", path, err)
143-
return nil
147+
return fmt.Errorf("error getting last accessed time of %s: %v", path, err)
144148
}
145149
it := &qitem{
146150
key: k,
147151
lat: atime,
148152
}
149153
size, err := res.getItemSize(k)
150154
if err != nil {
151-
log.Errorf("Error getting file size of %s: %v", path, err)
152-
return nil
155+
return fmt.Errorf("error getting file size of %s: %v", path, err)
153156
}
154157
res.store.Store(k, it)
155158
atomic.AddInt64(&res.sizeBytes, size)
@@ -158,11 +161,13 @@ func New(ctx context.Context, root string, maxCapacityBytes uint64) *DiskCache {
158161
res.mu.Unlock()
159162
return nil
160163
})
161-
}()
164+
})
165+
}
166+
if err := eg.Wait(); err != nil {
167+
return nil, err
162168
}
163-
wg.Wait()
164169
go res.gc()
165-
return res
170+
return res, nil
166171
}
167172

168173
func (d *DiskCache) getItemSize(k key) (int64, error) {
@@ -284,18 +289,13 @@ func copyFile(src, dst string, size int64) error {
284289
return err
285290
}
286291
defer out.Close()
287-
_, err = io.Copy(out, in)
292+
n, err := io.Copy(out, in)
288293
if err != nil {
289294
return err
290295
}
291-
// Required sanity check: sometimes the copy pretends to succeed, but doesn't, if
292-
// the file is being concurrently deleted.
293-
dstInfo, err := os.Stat(dst)
294-
if err != nil {
295-
return err
296-
}
297-
if dstInfo.Size() != size {
298-
return fmt.Errorf("copy of %s to %s failed: src/dst size mismatch: wanted %d, got %d", src, dst, size, dstInfo.Size())
296+
// Required sanity check: if the file is being concurrently deleted, we may not always copy everything.
297+
if n != size {
298+
return fmt.Errorf("copy of %s to %s failed: src/dst size mismatch: wanted %d, got %d", src, dst, size, n)
299299
}
300300
return nil
301301
}
@@ -309,15 +309,23 @@ func (d *DiskCache) LoadCas(dg digest.Digest, path string) bool {
309309
}
310310
it := iUntyped.(*qitem)
311311
it.mu.RLock()
312-
if err := copyFile(d.getPath(k), path, dg.Size); err != nil {
312+
err := copyFile(d.getPath(k), path, dg.Size)
313+
it.mu.RUnlock()
314+
if err != nil {
313315
// It is not possible to prevent a race with GC; hence, we return false on copy errors.
314-
it.mu.RUnlock()
315316
return false
316317
}
317-
it.mu.RUnlock()
318318

319319
d.mu.Lock()
320320
d.queue.Bump(it)
321321
d.mu.Unlock()
322322
return true
323323
}
324+
325+
func getLastAccessTime(path string) (time.Time, error) {
326+
info, err := os.Stat(path)
327+
if err != nil {
328+
return time.Time{}, err
329+
}
330+
return FileInfoToAccessTime(info), nil
331+
}

‎go/pkg/diskcache/diskcache_test.go‎

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,10 @@ func TestStoreLoadCasPerm(t *testing.T) {
4141
for _, tc := range tests {
4242
t.Run(tc.name, func(t *testing.T) {
4343
root := t.TempDir()
44-
d := New(context.Background(), filepath.Join(root, "cache"), 20)
44+
d, err := New(context.Background(), filepath.Join(root, "cache"), 20)
45+
if err != nil {
46+
t.Errorf("New: %v", err)
47+
}
4548
defer d.Shutdown()
4649
fname, _ := testutil.CreateFile(t, tc.executable, "12345")
4750
srcInfo, err := os.Stat(fname)
@@ -79,7 +82,10 @@ func TestStoreLoadCasPerm(t *testing.T) {
7982

8083
func TestLoadCasNotFound(t *testing.T) {
8184
root := t.TempDir()
82-
d := New(context.Background(), filepath.Join(root, "cache"), 20)
85+
d, err := New(context.Background(), filepath.Join(root, "cache"), 20)
86+
if err != nil {
87+
t.Errorf("New: %v", err)
88+
}
8389
defer d.Shutdown()
8490
newName := filepath.Join(root, "new")
8591
dg := digest.NewFromBlob([]byte("bla"))
@@ -90,7 +96,10 @@ func TestLoadCasNotFound(t *testing.T) {
9096

9197
func TestGcOldestCas(t *testing.T) {
9298
root := t.TempDir()
93-
d := New(context.Background(), filepath.Join(root, "cache"), 20)
99+
d, err := New(context.Background(), filepath.Join(root, "cache"), 20)
100+
if err != nil {
101+
t.Errorf("New: %v", err)
102+
}
94103
defer d.Shutdown()
95104
d.testGcTicks = make(chan uint64, 1)
96105
for i := 0; i < 5; i++ {
@@ -123,11 +132,11 @@ func TestGcOldestCas(t *testing.T) {
123132
func isSystemLastAccessTimeAccurate(t *testing.T) bool {
124133
t.Helper()
125134
fname, _ := testutil.CreateFile(t, false, "foo")
126-
lat, _ := GetLastAccessTime(fname)
135+
lat, _ := getLastAccessTime(fname)
127136
if _, err := os.ReadFile(fname); err != nil {
128137
t.Fatalf("%v", err)
129138
}
130-
newLat, _ := GetLastAccessTime(fname)
139+
newLat, _ := getLastAccessTime(fname)
131140
return lat.Before(newLat)
132141
}
133142

@@ -140,7 +149,10 @@ func TestInitFromExistingCas(t *testing.T) {
140149
return
141150
}
142151
root := t.TempDir()
143-
d := New(context.Background(), filepath.Join(root, "cache"), 20)
152+
d, err := New(context.Background(), filepath.Join(root, "cache"), 20)
153+
if err != nil {
154+
t.Errorf("New: %v", err)
155+
}
144156
for i := 0; i < 4; i++ {
145157
fname, _ := testutil.CreateFile(t, false, fmt.Sprintf("aaa %d", i))
146158
dg, err := digest.NewFromFile(fname)
@@ -159,7 +171,10 @@ func TestInitFromExistingCas(t *testing.T) {
159171
d.Shutdown()
160172

161173
// Re-initialize from existing files.
162-
d = New(context.Background(), filepath.Join(root, "cache"), 20)
174+
d, err = New(context.Background(), filepath.Join(root, "cache"), 20)
175+
if err != nil {
176+
t.Errorf("New: %v", err)
177+
}
163178
defer d.Shutdown()
164179
d.testGcTicks = make(chan uint64, 1)
165180

@@ -169,7 +184,7 @@ func TestInitFromExistingCas(t *testing.T) {
169184
t.Errorf("expected %s to be cached", dg)
170185
}
171186
fname, _ := testutil.CreateFile(t, false, "aaa 4")
172-
dg, err := digest.NewFromFile(fname)
187+
dg, err = digest.NewFromFile(fname)
173188
if err != nil {
174189
t.Fatalf("digest.NewFromFile failed: %v", err)
175190
}
@@ -198,7 +213,10 @@ func TestThreadSafetyCas(t *testing.T) {
198213
nFiles := 10
199214
attempts := 5000
200215
// All blobs are size 5 exactly. We will have half the byte capacity we need.
201-
d := New(context.Background(), filepath.Join(root, "cache"), uint64(nFiles*5)/2)
216+
d, err := New(context.Background(), filepath.Join(root, "cache"), uint64(nFiles*5)/2)
217+
if err != nil {
218+
t.Errorf("New: %v", err)
219+
}
202220
d.testGcTicks = make(chan uint64, attempts)
203221
defer d.Shutdown()
204222
var files []string

‎go/pkg/diskcache/sys_darwin.go‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,11 @@
22
package diskcache
33

44
import (
5-
"os"
5+
"io/fs"
66
"syscall"
77
"time"
88
)
99

10-
func GetLastAccessTime(path string) (time.Time, error) {
11-
info, err := os.Stat(path)
12-
if err != nil {
13-
return time.Time{}, err
14-
}
15-
return time.Unix(info.Sys().(*syscall.Stat_t).Atimespec.Unix()), nil
10+
func FileInfoToAccessTime(info fs.FileInfo) time.Time {
11+
return time.Unix(info.Sys().(*syscall.Stat_t).Atimespec.Unix())
1612
}

‎go/pkg/diskcache/sys_linux.go‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,11 @@
22
package diskcache
33

44
import (
5-
"os"
5+
"io/fs"
66
"syscall"
77
"time"
88
)
99

10-
func GetLastAccessTime(path string) (time.Time, error) {
11-
info, err := os.Stat(path)
12-
if err != nil {
13-
return time.Time{}, err
14-
}
15-
return time.Unix(info.Sys().(*syscall.Stat_t).Atim.Unix()), nil
10+
func FileInfoToAccessTime(info fs.FileInfo) time.Time {
11+
return time.Unix(info.Sys().(*syscall.Stat_t).Atim.Unix())
1612
}

‎go/pkg/diskcache/sys_windows.go‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,13 @@
22
package diskcache
33

44
import (
5-
"os"
5+
"io/fs"
66
"syscall"
77
"time"
88
)
99

1010
// This will return correct values only if `fsutil behavior set disablelastaccess 0` is set.
1111
// Tracking of last access time is disabled by default on Windows.
12-
func GetLastAccessTime(path string) (time.Time, error) {
13-
info, err := os.Stat(path)
14-
if err != nil {
15-
return time.Time{}, err
16-
}
17-
return time.Unix(0, info.Sys().(*syscall.Win32FileAttributeData).LastAccessTime.Nanoseconds()), nil
12+
func FileInfoToAccessTime(info fs.FileInfo) time.Time {
13+
return time.Unix(0, info.Sys().(*syscall.Win32FileAttributeData).LastAccessTime.Nanoseconds())
1814
}

0 commit comments

Comments
 (0)