diff --git a/handler.go b/handler.go index dc72d43..e9d4b07 100644 --- a/handler.go +++ b/handler.go @@ -44,6 +44,10 @@ type UnixChange interface { // CachingHandler represents the optional caching work that a user may wish to over-ride with // their own implementations, but which can be otherwise provided through defaults. +// +// If your Handler wraps another one (for example to add logging or metrics), give it an +// `Unwrap() Handler` method that returns the inner handler. The server follows Unwrap to find +// a CachingHandler underneath. type CachingHandler interface { VerifierFor(path string, contents []fs.FileInfo) uint64 diff --git a/helpers/cachinghandler.go b/helpers/cachinghandler.go index 73647eb..b876d21 100644 --- a/helpers/cachinghandler.go +++ b/helpers/cachinghandler.go @@ -8,6 +8,7 @@ import ( "sync" "github.com/willscott/go-nfs" + "github.com/willscott/go-nfs/file" "github.com/go-git/go-billy/v5" "github.com/google/uuid" @@ -190,6 +191,12 @@ func hashPathAndContents(path string, contents []fs.FileInfo) uint64 { for _, c := range contents { vHash.Write([]byte(c.Name())) // Never fails according to the docs + // One CachingHandler serves every mounted filesystem, and two of them can have a + // directory with the same path and file names. Mixing in each file's ID gives + // them different verifiers. + if info := file.GetInfo(c); info != nil { + vHash.Write(binary.BigEndian.AppendUint64(nil, info.Fileid)) + } } verify := vHash.Sum(nil)[0:8] @@ -203,7 +210,7 @@ func (c *CachingHandler) VerifierFor(path string, contents []fs.FileInfo) uint64 } func (c *CachingHandler) DataForVerifier(path string, id uint64) []fs.FileInfo { - if cache, ok := c.activeVerifiers.Get(id); ok { + if cache, ok := c.activeVerifiers.Get(id); ok && cache.path == path { return cache.contents } return nil diff --git a/helpers/cachinghandler_verifier_test.go b/helpers/cachinghandler_verifier_test.go new file mode 100644 index 0000000..3966d4a --- /dev/null +++ b/helpers/cachinghandler_verifier_test.go @@ -0,0 +1,52 @@ +package helpers + +import ( + "io/fs" + "testing" + "time" + + "github.com/willscott/go-nfs/file" +) + +// dirEntry is a fake directory entry that carries a file ID, like a real stat result does. +type dirEntry struct { + name string + size int64 + fileid uint64 +} + +func (e dirEntry) Name() string { return e.name } +func (e dirEntry) Size() int64 { return e.size } +func (e dirEntry) Mode() fs.FileMode { return 0o644 } +func (e dirEntry) ModTime() time.Time { return time.Time{} } +func (e dirEntry) IsDir() bool { return false } +func (e dirEntry) Sys() any { return file.FileInfo{Fileid: e.fileid} } + +// Two filesystems can have a directory with the same path and file names. Each must get +// its own verifier, otherwise one would be shown the other's files. +func TestVerifierForTellsApartFilesystemsWithTheSameNames(t *testing.T) { + c := NewCachingHandler(nil, 16).(*CachingHandler) + + onA := []fs.FileInfo{dirEntry{name: "main.go", size: 10, fileid: 5001}} + onB := []fs.FileInfo{dirEntry{name: "main.go", size: 99000, fileid: 7002}} + + verifierA := c.VerifierFor("/src", onA) + verifierB := c.VerifierFor("/src", onB) + if verifierA == verifierB { + t.Fatalf("listings with different file IDs share verifier %x", verifierA) + } + + got := c.DataForVerifier("/src", verifierA) + if len(got) != 1 || got[0].Size() != 10 { + t.Errorf("listing for A = %v, want A's main.go of size 10", got) + } +} + +func TestDataForVerifierChecksThePath(t *testing.T) { + c := NewCachingHandler(nil, 16).(*CachingHandler) + + verifier := c.VerifierFor("/src", []fs.FileInfo{dirEntry{name: "main.go", fileid: 1}}) + if got := c.DataForVerifier("/other", verifier); got != nil { + t.Errorf("verifier for /src returned a listing for /other: %v", got) + } +} diff --git a/nfs_onreaddir.go b/nfs_onreaddir.go index 5992c34..d65f017 100644 --- a/nfs_onreaddir.go +++ b/nfs_onreaddir.go @@ -12,6 +12,8 @@ import ( "sort" "github.com/willscott/go-nfs-client/nfs/xdr" + + "github.com/willscott/go-nfs/file" ) type readDirArgs struct { @@ -45,7 +47,7 @@ func onReadDir(ctx context.Context, w *response, userHandle Handler) error { return &NFSStatusError{NFSStatusStale, err} } - contents, verifier, err := getDirListingWithVerifier(ctx, userHandle, obj.Handle, obj.CookieVerif) + contents, verifier, err := getDirListingWithVerifier(ctx, userHandle, obj.Handle, obj.Cookie, obj.CookieVerif) if err != nil { return err } @@ -137,7 +139,10 @@ func onReadDir(ctx context.Context, w *response, userHandle Handler) error { return nil } -func getDirListingWithVerifier(ctx context.Context, userHandle Handler, fsHandle []byte, verifier uint64) ([]fs.FileInfo, uint64, error) { +// getDirListingWithVerifier returns the entries of the directory a handle points to. +// When the client continues a listing (cookie > 0), the copy cached on the first page is +// used if there is one. A new listing (cookie 0) always reads the directory. +func getDirListingWithVerifier(ctx context.Context, userHandle Handler, fsHandle []byte, cookie uint64, verifier uint64) ([]fs.FileInfo, uint64, error) { // figure out what directory it is. fs, p, err := userHandle.FromHandle(ctx, fsHandle) if err != nil { @@ -145,8 +150,9 @@ func getDirListingWithVerifier(ctx context.Context, userHandle Handler, fsHandle } path := fs.Join(p...) + vh, caching := cachingHandlerOf(userHandle) // see if the verifier has this dir cached: - if vh, ok := userHandle.(CachingHandler); verifier != 0 && ok { + if caching && cookie != 0 && verifier != 0 { entries := vh.DataForVerifier(path, verifier) if entries != nil { return entries, verifier, nil @@ -165,7 +171,7 @@ func getDirListingWithVerifier(ctx context.Context, userHandle Handler, fsHandle return contents[i].Name() < contents[j].Name() }) - if vh, ok := userHandle.(CachingHandler); ok { + if caching { // let the user handler make a verifier if it can. v := vh.VerifierFor(path, contents) return contents, v, nil @@ -175,6 +181,22 @@ func getDirListingWithVerifier(ctx context.Context, userHandle Handler, fsHandle return contents, id, nil } +// cachingHandlerOf returns the CachingHandler inside h, following Unwrap through any +// handlers that wrap it. +func cachingHandlerOf(h Handler) (CachingHandler, bool) { + for h != nil { + if c, ok := h.(CachingHandler); ok { + return c, true + } + wrapper, ok := h.(interface{ Unwrap() Handler }) + if !ok { + break + } + h = wrapper.Unwrap() + } + return nil, false +} + func hashPathAndContents(path string, contents []fs.FileInfo) uint64 { //calculate a cookie-verifier. vHash := sha256.New() @@ -184,6 +206,11 @@ func hashPathAndContents(path string, contents []fs.FileInfo) uint64 { for _, c := range contents { vHash.Write([]byte(c.Name())) // Never fails according to the docs + // Two filesystems can have a directory with the same path and file names. + // Mixing in each file's ID gives them different verifiers. + if info := file.GetInfo(c); info != nil { + vHash.Write(binary.BigEndian.AppendUint64(nil, info.Fileid)) + } } verify := vHash.Sum(nil)[0:8] diff --git a/nfs_onreaddir_test.go b/nfs_onreaddir_test.go new file mode 100644 index 0000000..9bf0f29 --- /dev/null +++ b/nfs_onreaddir_test.go @@ -0,0 +1,101 @@ +package nfs + +import ( + "context" + "io/fs" + "testing" + + "github.com/go-git/go-billy/v5" + "github.com/go-git/go-billy/v5/memfs" +) + +// dirHandler returns the directory "dir" on fs, whatever handle it is given. +type dirHandler struct { + Handler + fs billy.Filesystem +} + +func (h *dirHandler) FromHandle(context.Context, []byte) (billy.Filesystem, []string, error) { + return h.fs, []string{"dir"}, nil +} + +// cachingDirHandler returns the same cached listing for any verifier. +type cachingDirHandler struct { + *dirHandler + cached []fs.FileInfo + lookups int +} + +func (h *cachingDirHandler) VerifierFor(string, []fs.FileInfo) uint64 { return 42 } + +func (h *cachingDirHandler) DataForVerifier(string, uint64) []fs.FileInfo { + h.lookups++ + return h.cached +} + +// wrappingHandler wraps another handler, like logging or metrics middleware does. +type wrappingHandler struct{ Handler } + +func (w wrappingHandler) Unwrap() Handler { return w.Handler } + +func newCachingDirHandler(t *testing.T) *cachingDirHandler { + t.Helper() + + mem := memfs.New() + f, err := mem.Create("dir/on-disk") + if err != nil { + t.Fatal(err) + } + _ = f.Close() + + cachedDir := memfs.New() + f, err = cachedDir.Create("cached") + if err != nil { + t.Fatal(err) + } + _ = f.Close() + cached, err := cachedDir.ReadDir("/") + if err != nil { + t.Fatal(err) + } + + return &cachingDirHandler{dirHandler: &dirHandler{fs: mem}, cached: cached} +} + +func names(entries []fs.FileInfo) []string { + out := make([]string, 0, len(entries)) + for _, e := range entries { + out = append(out, e.Name()) + } + return out +} + +func TestDirListingFindsCachingHandlerThroughWrappers(t *testing.T) { + h := newCachingDirHandler(t) + + entries, verifier, err := getDirListingWithVerifier(context.Background(), wrappingHandler{h}, nil, 5, 42) + if err != nil { + t.Fatal(err) + } + if got := names(entries); len(got) != 1 || got[0] != "cached" { + t.Errorf("continuation page should come from the wrapped handler's cache, got %v", got) + } + if verifier != 42 { + t.Errorf("verifier = %d, want 42", verifier) + } +} + +func TestDirListingFromCookieZeroReadsTheDirectory(t *testing.T) { + h := newCachingDirHandler(t) + + entries, _, err := getDirListingWithVerifier(context.Background(), h, nil, 0, 42) + if err != nil { + t.Fatal(err) + } + if got := names(entries); len(got) != 1 || got[0] != "on-disk" { + t.Errorf("a listing from cookie zero should read the directory, got %v", got) + } + if h.lookups != 0 { + t.Errorf("verifier cache consulted %d times for a cookie-zero listing", h.lookups) + } +} diff --git a/nfs_onreaddirplus.go b/nfs_onreaddirplus.go index de75a15..9d07a53 100644 --- a/nfs_onreaddirplus.go +++ b/nfs_onreaddirplus.go @@ -51,7 +51,7 @@ func onReadDirPlus(ctx context.Context, w *response, userHandle Handler) error { return &NFSStatusError{NFSStatusStale, err} } - contents, verifier, err := getDirListingWithVerifier(ctx, userHandle, obj.Handle, obj.CookieVerif) + contents, verifier, err := getDirListingWithVerifier(ctx, userHandle, obj.Handle, obj.Cookie, obj.CookieVerif) if err != nil { return err }