diff --git a/store/remotestore/remotestore.go b/store/remotestore/remotestore.go index 1779547..e0ceff7 100644 --- a/store/remotestore/remotestore.go +++ b/store/remotestore/remotestore.go @@ -36,9 +36,17 @@ type Store struct { mu sync.Mutex modmaps map[store.ModuleVersion]*remoteModHandle // cached modmap per MV + meta map[metaKey][]byte // cached .info, .mod and .ziphash files tmpDir string // lazy-created temp dir for large files } +// metaKey is the key of a cached metadata file: ext is "info", "mod" or +// "ziphash". +type metaKey struct { + mv store.ModuleVersion + ext string +} + // Close cleans up temporary files. It should be called when the store is no longer needed. func (s *Store) Close() error { s.mu.Lock() @@ -274,7 +282,40 @@ func (s *Store) GetZipHash(ctx context.Context, h store.ModHandle) ([]byte, erro return s.getMetaFile(ctx, rmh.mv, "ziphash") } +// getMetaFile returns the metadata file with extension ext for mv. +// +// Metadata files never change for a module version, and the go command +// reads hundreds of them per invocation (each stat and open of one is a +// call here), so they're cached for the life of the Store. Misses aren't +// cached, as the server may get the module later. func (s *Store) getMetaFile(ctx context.Context, mv store.ModuleVersion, ext string) ([]byte, error) { + k := metaKey{mv, ext} + s.mu.Lock() + v, ok := s.meta[k] + s.mu.Unlock() + if ok { + return v, nil + } + vi, err, _ := s.sf.Do("meta:"+mv.Module+"@"+mv.Version+"."+ext, func() (any, error) { + v, err := s.fetchMetaFile(ctx, mv, ext) + if err != nil { + return nil, err + } + s.mu.Lock() + defer s.mu.Unlock() + if s.meta == nil { + s.meta = make(map[metaKey][]byte) + } + s.meta[k] = v + return v, nil + }) + if err != nil { + return nil, err + } + return vi.([]byte), nil +} + +func (s *Store) fetchMetaFile(ctx context.Context, mv store.ModuleVersion, ext string) ([]byte, error) { escaped, err := mvToEscapedPath(mv) if err != nil { return nil, err diff --git a/store/remotestore/remotestore_test.go b/store/remotestore/remotestore_test.go index d9c6148..960f42b 100644 --- a/store/remotestore/remotestore_test.go +++ b/store/remotestore/remotestore_test.go @@ -5,11 +5,14 @@ package remotestore import ( "context" + "errors" "io/fs" + "maps" "net" "net/http" "net/http/httptest" "slices" + "strings" "sync" "sync/atomic" "testing" @@ -60,6 +63,65 @@ func TestDefaultClientReusesConns(t *testing.T) { } } +// TestMetaFileCache tests that a Store fetches each metadata file from +// the server once, even when asked concurrently, and doesn't cache misses. +func TestMetaFileCache(t *testing.T) { + var mu sync.Mutex + reqs := map[string]int{} // URL path -> requests + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + reqs[r.URL.Path]++ + mu.Unlock() + if strings.Contains(r.URL.Path, "/missing/") { + http.NotFound(w, r) + return + } + w.Write([]byte("contents of " + r.URL.Path)) + })) + defer srv.Close() + + s := &Store{BaseURL: srv.URL} + ctx := context.Background() + mv := store.ModuleVersion{Module: "example.com/m", Version: "v1.0.0"} + var wg sync.WaitGroup + for range 10 { + wg.Go(func() { + for range 5 { + mod, err := s.GetModFile(ctx, mv) + if err != nil { + t.Error(err) + return + } + if want := "contents of " + metaPrefix + "example.com/m/@v/v1.0.0.mod"; string(mod) != want { + t.Errorf("GetModFile = %q; want %q", mod, want) + } + if _, err := s.GetInfoFile(ctx, mv); err != nil { + t.Error(err) + } + } + }) + } + wg.Wait() + + missing := store.ModuleVersion{Module: "example.com/missing", Version: "v1.0.0"} + for range 2 { + if _, err := s.GetModFile(ctx, missing); !errors.Is(err, store.ErrCacheMiss) { + t.Errorf("GetModFile(missing) error = %v; want ErrCacheMiss", err) + } + } + + mu.Lock() + defer mu.Unlock() + want := map[string]int{ + metaPrefix + "example.com/m/@v/v1.0.0.mod": 1, + metaPrefix + "example.com/m/@v/v1.0.0.info": 1, + metaPrefix + "example.com/missing/@v/v1.0.0.mod": 2, + } + if !maps.Equal(reqs, want) { + t.Errorf("server requests = %v; want %v", reqs, want) + } +} + func TestBuildDirMap(t *testing.T) { tests := []struct { name string diff --git a/winfsp.go b/winfsp.go index e2efe75..dba094d 100644 --- a/winfsp.go +++ b/winfsp.go @@ -15,10 +15,10 @@ import ( "path" "path/filepath" "strings" + "sync" "syscall" "time" - "github.com/tailscale/gomodfs/store" "github.com/winfsp/go-winfsp" "github.com/winfsp/go-winfsp/gofs" "golang.org/x/sys/windows" @@ -295,25 +295,30 @@ func (pfs *fspFS) OpenFile(name string, flag int, perm os.FileMode) (retFile gof if err != nil { return nil, err } - spanGF := d.fs.Stats.StartSpan("fsp.OpenFile-GetFile") - contents, err := d.fs.Store.GetFile(ctx, mh, dp.Path) + fi, err := d.fs.Store.Stat(ctx, mh, dp.Path) if err != nil { - if errors.Is(err, store.ErrIsDir) { - spanRD := d.fs.Stats.StartSpan("fsp.OpenFile-Readdir") - ents, err := d.fs.Store.Readdir(ctx, mh, dp.Path) - spanRD.End(err) - if err != nil { - spanGF.End(err) - return nil, err - } - spanGF.End(nil) - return wdDir{pathInZip: dp.Path, baseName: base, ents: ents}, nil - } - spanGF.End(err) return nil, err } - spanGF.End(nil) - return newFWPFileFromContents(name, contents), nil + if fi.IsDir() { + spanRD := d.fs.Stats.StartSpan("fsp.OpenFile-Readdir") + ents, err := d.fs.Store.Readdir(ctx, mh, dp.Path) + spanRD.End(err) + if err != nil { + return nil, err + } + return wdDir{pathInZip: dp.Path, baseName: base, ents: ents}, nil + } + // Get the contents on the first read, not now: Windows opens files for + // many things that don't read them, including every os.Stat. + return &winFSPRegularFile{ + fi: regFileInfo{name: name, size: fi.Size()}, + getContents: func() ([]byte, error) { + sp := d.fs.Stats.StartSpan("fsp.ReadAt-GetFile") + contents, err := d.fs.Store.GetFile(ctx, mh, dp.Path) + sp.End(err) + return contents, err + }, + }, nil } func (s *winFSPRunner) Unmount() error { @@ -350,8 +355,32 @@ func newFWPFileFromContents(baseName string, contents []byte) gofs.File { } type winFSPRegularFile struct { - fi os.FileInfo - contents []byte + fi os.FileInfo + + // getContents, if non-nil, returns the file's contents. It's called on + // the first ReadAt, and again on later ones until it succeeds. + getContents func() ([]byte, error) + + mu sync.Mutex + contents []byte // the contents, once known + haveData bool // contents is valid +} + +// data returns the file's contents, getting them first if needed. +func (f *winFSPRegularFile) data() ([]byte, error) { + if f.getContents == nil { + return f.contents, nil + } + f.mu.Lock() + defer f.mu.Unlock() + if !f.haveData { + contents, err := f.getContents() + if err != nil { + return nil, err + } + f.contents, f.haveData = contents, true + } + return f.contents, nil } func (f *winFSPRegularFile) Close() error { return nil } @@ -374,7 +403,11 @@ func (f *winFSPRegularFile) Read(p []byte) (n int, err error) { } func (f *winFSPRegularFile) ReadAt(p []byte, off int64) (n int, err error) { - n = copy(p, f.contents[min(off, int64(len(f.contents))):]) + contents, err := f.data() + if err != nil { + return 0, err + } + n = copy(p, contents[min(off, int64(len(contents))):]) if n == 0 && len(p) > 0 { return 0, io.EOF }